-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add automated resource loading of icon-images #1000
Conversation
@tobrun, this is awesome! @julianrex @fabian-guerra, is this also possible on the iOS side? |
Yep, totally. |
@chloekraw my answer would be yes and no 😅 Unlike Android listeners approach. iOS uses a delegate architecture where the map can "listen" for events and pass this to its single delegate. This means if I implement the delegate in the annotations controller then the developer won't be able to use the map's delegate in his own classes. This is what I mean with "no": I won't be using the same approach. "Yes" means we can support it with a different approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add a withIconImage
option that accepts a resources ID, convert that to a String
when passing to the map and back to an int
in the ImageLoader
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, could you include a test?
plugin-annotation/src/main/java/com/mapbox/mapboxsdk/plugins/annotation/SymbolManager.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Could you add a line with an example to the SymbolOptions#withIconImage(String)
javadoc explaining how a drawable name can be used as well?
@@ -146,5 +148,20 @@ static Bitmap getBitmapFromDrawable(Drawable drawable) { | |||
return bitmap; | |||
} | |||
} | |||
|
|||
@VisibleForTesting |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can changes here be reverted? They seem unused.
@@ -74,12 +81,15 @@ public class <%- camelize(type) %>Manager extends AnnotationManager<<%- camelize | |||
*/ | |||
@UiThread | |||
public <%- camelize(type) %>Manager(@NonNull MapView mapView, @NonNull MapboxMap mapboxMap, @NonNull Style style, @Nullable String belowLayerId, @Nullable GeoJsonOptions geoJsonOptions) { | |||
this(mapView, mapboxMap, style, new <%- camelize(type) %>ElementProvider(), belowLayerId, geoJsonOptions, new DraggableAnnotationController<<%- camelize(type) %>, On<%- camelize(type) %>DragListener>(mapView, mapboxMap)); | |||
this(mapView, mapboxMap, style, new <%- camelize(type) %>ElementProvider(), belowLayerId, geoJsonOptions, new DraggableAnnotationController<<%- camelize(type) %>, On<%- camelize(type) %>DragListener>(mapView, mapboxMap), new ImageLoader(mapView)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ImageLoader
argument addition should be if-checked for symbol manager.
} | ||
|
||
@VisibleForTesting | ||
<%- camelize(type) %>Manager(@NonNull MapView mapView, @NonNull MapboxMap mapboxMap, @NonNull Style style, @NonNull CoreElementProvider<<%- camelize(type) %>Layer> coreElementProvider, @Nullable String belowLayerId, @Nullable GeoJsonOptions geoJsonOptions, DraggableAnnotationController<<%- camelize(type) %>, On<%- camelize(type) %>DragListener> draggableAnnotationController) { | ||
<%- camelize(type) %>Manager(@NonNull MapView mapView, @NonNull MapboxMap mapboxMap, @NonNull Style style, @NonNull CoreElementProvider<<%- camelize(type) %>Layer> coreElementProvider, @Nullable String belowLayerId, @Nullable GeoJsonOptions geoJsonOptions, DraggableAnnotationController<<%- camelize(type) %>, On<%- camelize(type) %>DragListener> draggableAnnotationController, ImageLoader imageLoader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions. |
@tobrun @LukasPaczos any updates on this? 😄 It's currently a blocker, since the update to the latest mapbox sdk version. |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions. |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions. |
Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions. |
This PR is a proposal to automate loading of icon-image property of symbols. If core misses an style image, it will emit the image missing event. The symbol manager will pick this up and will attempt to load a drawable from resources with this id. This way users don't need to call addImage themselves, they can just pass in the id of the drawable resource as a String (see here).
Closes #742
@mapbox/maps-android