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

[sdk] Use cached resourceOptions if it's initialized. #77

Merged
merged 1 commit into from
Feb 4, 2021

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Feb 4, 2021

PRs must be submitted under the terms of our Contributor License Agreement CLA.
Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Add example if relevant.
  • Document any changes to public APIs.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog>[sdk] Use cached resourceOptions if it's initialized.</changelog>.

Summary of changes

This pr add the function that checks and use the cached resourceOptions if it's initialized instead of recreate it from ResourcesOptions

User impact (optional)

@Chaoba Chaoba self-assigned this Feb 4, 2021
@Chaoba Chaoba added the bug 🪲 Something isn't working label Feb 4, 2021
Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

@Chaoba let's imagine we have 2 MapViews in one layout and we want to set different tokens to each of it programatically (and also different from <string name="mapbox_access_token"></string>) . Can it be done with current setup? If yes - guess we should provide an example in app module.

@Chaoba Chaoba requested a review from a team February 4, 2021 09:02
assertEquals(defaultOptions.assetPath, currentOptions.assetPath)
assertEquals(defaultOptions.cachePath, currentOptions.cachePath)
assertEquals(defaultOptions.cacheSize, currentOptions.cacheSize)
assertEquals(defaultOptions, currentOptions)
Copy link
Member

Choose a reason for hiding this comment

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

nice catch!

@pengdev
Copy link
Member

pengdev commented Feb 4, 2021

@Chaoba let's imagine we have 2 MapViews in one layout and we want to set different tokens to each of it programatically (and also different from <string name="mapbox_access_token"></string>) . Can it be done with current setup? If yes - guess we should provide an example in app module.

I guess it's hard to showcase this example without exposing the access token in the code.

@Chaoba
Copy link
Contributor Author

Chaoba commented Feb 4, 2021

@Chaoba let's imagine we have 2 MapViews in one layout and we want to set different tokens to each of it programatically (and also different from <string name="mapbox_access_token"></string>) . Can it be done with current setup? If yes - guess we should provide an example in app module.

@kiryldz Even without the changes in this pr, set different tokens to 2 MapView is available. This pr resolves the scenario that if users use interface MapboxOptions#setDefaultResourceOptions updated the ResourceOptions and create a new MapView, the updated ResourceOptions will not work on the new MapView.

@kiryldz
Copy link
Contributor

kiryldz commented Feb 4, 2021

@Chaoba let's imagine we have 2 MapViews in one layout and we want to set different tokens to each of it programatically (and also different from <string name="mapbox_access_token"></string>) . Can it be done with current setup? If yes - guess we should provide an example in app module.

I guess it's hard to showcase this example without exposing the access token in the code.

We could use any string and not make MapView show data (because token is invalid).
However that use-case is pretty real, I faced it myself in our Preview App, when valid token comes through web and gets saved to shared preferences and then must be used with MapView inflated from xml.

@kiryldz
Copy link
Contributor

kiryldz commented Feb 4, 2021

@Chaoba let's imagine we have 2 MapViews in one layout and we want to set different tokens to each of it programatically (and also different from <string name="mapbox_access_token"></string>) . Can it be done with current setup? If yes - guess we should provide an example in app module.

@kiryldz Even without the changes in this pr, set different tokens to 2 MapView is available. This pr resolves the scenario that if users use interface MapboxOptions#setDefaultResourceOptions updated the ResourceOptions and create a new MapView, the updated ResourceOptions will not work on the new MapView.

My concern here is that our current logic will work fine with 1 MapView in xml - then we could MapboxOptions#setDefaultResourceOptions before setContentView in activity. But if we have 2 MapViews in the layout and each of it should have it's own token - that will not work I guess.
Generally such scenario should not be pretty common but anyway.

Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

LGTM, created a task to track revisiting approach in general.

@Chaoba Chaoba merged commit 66253b3 into main Feb 4, 2021
@Chaoba Chaoba deleted the kl-resource-option branch February 4, 2021 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants