Skip to content

Conversation

@danesfeder
Copy link
Contributor

Closes #486

This PR introduces options to use String names for icons that have already been added to the MapboxMap with MapboxMap#addImage. Developers may also provide names for Maki icons that are already loaded in their given style.

device-2018-07-05-155217

@danesfeder danesfeder added the location-layer-plugin Issues that deal with the location layer module label Jul 5, 2018
@danesfeder danesfeder self-assigned this Jul 5, 2018
@danesfeder danesfeder requested review from LukasPaczos and tobrun July 5, 2018 20:58
@danesfeder danesfeder added this to the location-layer-0.6.0 milestone Jul 5, 2018
String foregroundStaleIconString = buildIconString(options.foregroundStaleName(), FOREGROUND_STALE_ICON);
String backgroundIconString = buildIconString(options.backgroundName(), BACKGROUND_ICON);
String backgroundStaleIconString = buildIconString(options.backgroundStaleName(), BACKGROUND_STALE_ICON);
String bearingIconString = buildIconString(options.bearingName(), BEARING_ICON);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this values would have to be moved to the location feature, otherwise, you'd not be able to revert this custom image names when resetting the style on an already initialized plugin.

We could also document this behavior and leave as is, thoughts?

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! Questions/comments below.


private void styleForeground(@NonNull LocationLayerOptions options) {
boolean isGpsRenderMode = renderMode == RenderMode.GPS;
if (options.foregroundName() == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have separate cases for foregroundName and foregroundGpsName? This would allow for providing an id for both normal foreground icon, as well as a GPS one.


private void styleShadow(LocationLayerOptions options) {
Drawable shadowDrawable = ContextCompat.getDrawable(context, R.drawable.mapbox_user_icon_shadow);
if (shadowDrawable == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This drawable is provided by the plugin, can it be null?


private void addSymbolLayer(String layerId, String beforeLayerId) {
private void addSymbolLayer(String layerId, String beforeLayerId, LocationLayerOptions options) {
String foregroundString = options.gpsName() != null ? options.gpsName() : options.foregroundName();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LukasPaczos ran into a possible issue here - the GPS / Foreground drawables both share the foreground icon layer. At the time we add the symbol layer here, we don't know what render mode we are in yet. So a decision needs to be made as far as what String is used. Here I prioritized the GPS name. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the resolution here would be keeping the string IDs of the used images in the locationFeature, this way we would be able to update them in the runtime, without reloading the map.

@danesfeder
Copy link
Contributor Author

@LukasPaczos thanks for the feedback! Any ideas on how to alternate between String icon names and Drawables from the options? Are we forced to reload the map here?

@danesfeder
Copy link
Contributor Author

@LukasPaczos the Feature changes / tests you added look 💯 ✅

Copy link
Contributor

@LukasPaczos LukasPaczos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@LukasPaczos LukasPaczos merged commit 44ee2b6 into master Jul 9, 2018
@LukasPaczos LukasPaczos deleted the dan-name-icon branch July 9, 2018 15:51
@danesfeder danesfeder mentioned this pull request Jul 9, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

location-layer-plugin Issues that deal with the location layer module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants