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

Use theme attribute to update MapView map style URL #1018

Merged
merged 2 commits into from
Jun 21, 2018

Conversation

danesfeder
Copy link
Contributor

Closes #994

@mapbox/maps-android the mapbox_styleUrl is taken into account every time the MapView is rendered for the first time correct?

If we set the style URL from navigationViewMapStyle there shouldn't be any possible issues with caching and we can remove that method from ThemeSwitcher.

FWIW, I created an example that used the cancel btn to change the theme and recreate (simulating if this we to happen mid-drive) and I wasn't able to reproduce. @Guardiola31337 if you can provide a reliable way to reproduce, that would be great.

@danesfeder danesfeder added bug Defect to be fixed. navigation-ui labels Jun 11, 2018
@danesfeder danesfeder added this to the 0.15.0 milestone Jun 11, 2018
@danesfeder danesfeder self-assigned this Jun 11, 2018
@LukasPaczos
Copy link

@danesfeder correct, it's taken into account every time the MapView is inflated.

@Guardiola31337
Copy link
Contributor

@danesfeder

FWIW, I created an example that used the cancel btn to change the theme and recreate (simulating if this we to happen mid-drive) and I wasn't able to reproduce.

Could you add the example you wrote to try to reproduce #994 as part of this PR? I think that would be great to have it anyway so we can test different scenarios or any regressions that might appear.

@Guardiola31337
Copy link
Contributor

I can confirm that the changes implemented in this PR don't solve #994 👀

night_style_map_issue

When bringing app into the foreground after backgrounding nothing happened either, the map style didn't change 😓

Even after rotating the screen 👀

night_style_map_issue_landscape

My hunch either a regression from #939 or the MapView is caching somehow. Any hints @mapbox/maps-android?

@LukasPaczos
Copy link

When recreating an Activity previously used style is loaded as it's stored with onSaveInstace. The style passed with options is loaded only if there is no saved state available (each inflation, unless it's recreation process). I hope this helps.

@Guardiola31337
Copy link
Contributor

Guardiola31337 commented Jun 18, 2018

@danesfeder latest changes seem to fix the issue of changing the map style automatically between day/night mode (I still have to check if it changes automatically from night to day mode, day to night works 👌) but now during night it doesn't load the proper style at startup time (first time) 👀

night_mode_startup_issue

Also noticed that if you rotate the screen loads the proper map style (night in the example above) but the upcoming maneuver arrow disappears.

if (savedInstanceState != null) {
savedInstanceState.putString(MapboxConstants.STATE_STYLE_URL, mapStyleUrl);
} else {
mapView.setStyleUrl(mapStyleUrl);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Guardiola31337 found the issue - we were previously only updated when the saved instance state was not null, thus it was always day mode style.

@danesfeder
Copy link
Contributor Author

@Guardiola31337 this is ready for 👀

@Guardiola31337
Copy link
Contributor

@danesfeder

found the issue - we were previously only updated when the saved instance state was not null, thus it was always day mode style.

Nah, that doesn't solve it (at least completely) 👀

theme_switching_issue

It fails when closing and then reopening. It seems something with initialization

@danesfeder danesfeder force-pushed the dan-style-update branch 2 times, most recently from 50c475b to 6ced5fc Compare June 20, 2018 21:55
@danesfeder
Copy link
Contributor Author

I was able to reproduce #1018 (comment) and fixed it by setting the style URL again in the XML, but keeping the saved instance state check in NavigationView.

Currently looking into a public API to toggle the style (day vs. night) on the fly. Not sure if it's totally in scope of this PR but would make it easy to create an example.

@danesfeder
Copy link
Contributor Author

Added a FAB to the EmbeddedNavigationActivity that toggles the day / night mode:
ezgif com-video-to-gif

@danesfeder
Copy link
Contributor Author

@Guardiola31337 @devotaaabel this PR is ready for 👀

@danesfeder danesfeder force-pushed the dan-style-update branch 3 times, most recently from 0bcb27c to 10f7a5a Compare June 21, 2018 20:00
@Guardiola31337
Copy link
Contributor

It's working now 🎉

theme_night_mode_working

I'd need to test if the theme changes from night to day but this is definitely good to go 🚀

Well done @danesfeder 👏

@danesfeder danesfeder merged commit bf5b2f1 into master Jun 21, 2018
@danesfeder danesfeder deleted the dan-style-update branch June 21, 2018 21:01
@danesfeder danesfeder mentioned this pull request Jun 21, 2018
11 tasks
@Guardiola31337 Guardiola31337 mentioned this pull request Oct 4, 2018
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Defect to be fixed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants