Skip to content
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

Maps SDK 7.0.0 and required breaking changes refactoring #789

Merged
merged 14 commits into from
Jan 8, 2019

Conversation

langsmith
Copy link
Contributor

This pr bumps the plugins' Maps SDK dependency to 7.0.0-beta.1 in preparation for the stable 7.0.0 release. This pr also refactors all of the plugins to fix breaking changes.

mapbox/mapbox-android-demo#917 is related.

After commenting out the test app activities which are related to the LocationLayerPlugin, I got the test app running again. All of the non-LocationLayerPlugin activities are running and behaving fine for me. I encourage you to switch to this branch, comment out LocationLayerPlugin .kt files, and check out the app on your own device.

@tobrun & @LukasPaczos :

  • In some files, I declare private Style style;, set this.style = mapboxMap.getStyle(); in the initialization of the class, and then use if (style!=null) in various places throughout the class before running whatever code. Didn't know if that's preferred or if it's fine for us to just do mapboxMap.getStyle.XXXX instead.

  • Wasn't sure how to handle the mix of breaking changes and the deprecation of the location layer plugin. How should we proceed with it?

@langsmith langsmith added ⚠️ DO NOT MERGE traffic-plugin Issues that deal with the traffic module building-plugin offline-plugin Issues that deal with the offline plugin module places-plugin Issues that deal with the places plugin module localization-plugin annotation-plugin markerview-plugin mapbox-ktx labels Dec 15, 2018
@langsmith langsmith self-assigned this Dec 15, 2018
@langsmith langsmith changed the title Maps SDK 7.0.0 and required breaking changes factoring Maps SDK 7.0.0 and required breaking changes factoring [WIP] Dec 15, 2018
@langsmith
Copy link
Contributor Author

Also, other than the LocationLayerPlugin tests, I've checked/fixed the plugins' unit/instrumentation tests

@langsmith langsmith changed the title Maps SDK 7.0.0 and required breaking changes factoring [WIP] Maps SDK 7.0.0 and required breaking changes refactoring [WIP] Dec 15, 2018
@LukasPaczos
Copy link
Member

Thanks for kicking this off @langsmith!

Wasn't sure how to handle the mix of breaking changes and the deprecation of the location layer plugin. How should we proceed with it?

Let's remove the source code of the LocationLayerPlugin, we shouldn't need to maintain that code any longer. I think we can leave the module and README.md not to break existing links that are out there.

In some files, I declare private Style style;, set this.style = mapboxMap.getStyle(); in the initialization of the class, and then use if (style!=null) in various places throughout the class before running whatever code.

We thought about a different strategy wrt to plugins initialization - let's require a valid and fully loaded Style to be passed in the constructor, similarly as we've done with the LocationComponent. This removes one edge case where a user creates the plugin's instance before the style has been loaded.

Let me run with the above and generally let's push any required changes to this branch without rebasing, we can squash all the commits when merging to master.

@langsmith
Copy link
Contributor Author

Let's remove the source code of the LocationLayerPlugin, we shouldn't need to maintain that code any longer. I think we can leave the module and README.md not to break existing links that are out there.

@LukasPaczos , this means that the latest/current version of the plugin ( 0.11.0 ) is the final version, right? Asking because I'm thinking about whether any additional warning(s) about this need to go on https://www.mapbox.com/android-docs/plugins/overview/location-layer (cc @colleenmcginnis )

@LukasPaczos
Copy link
Member

@langsmith correct, 0.11.0 is the final version of the LocationLayerPlugin.

@LukasPaczos LukasPaczos force-pushed the ls-maps-sdk-7.0.0-bump-and-required-refactoring branch from 75938ea to 58a414a Compare January 8, 2019 11:39
Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

Looks great! :shipit:

loadingArgumentCaptor.getValue().onWillStartLoadingMap();

ArgumentCaptor<Style.OnStyleLoaded> styleLoadedArgumentCaptor = ArgumentCaptor.forClass(Style.OnStyleLoaded.class);
verify(mapboxMap).getStyle(styleLoadedArgumentCaptor.capture());
Copy link
Member

Choose a reason for hiding this comment

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

interesting construct this ArgumentCaptor

@LukasPaczos LukasPaczos changed the title Maps SDK 7.0.0 and required breaking changes refactoring [WIP] Maps SDK 7.0.0 and required breaking changes refactoring Jan 8, 2019
@LukasPaczos LukasPaczos merged commit 7ba51ad into master Jan 8, 2019
@LukasPaczos LukasPaczos deleted the ls-maps-sdk-7.0.0-bump-and-required-refactoring branch January 8, 2019 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annotation-plugin building-plugin localization-plugin mapbox-ktx markerview-plugin offline-plugin Issues that deal with the offline plugin module places-plugin Issues that deal with the places plugin module traffic-plugin Issues that deal with the traffic module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants