-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fallback to OSM if other engines aren't available #5702
Conversation
google-maps/src/main/java/org/odk/collect/googlemaps/GoogleMapFragment.java
Outdated
Show resolved
Hide resolved
google-maps/src/main/java/org/odk/collect/googlemaps/GoogleMapFragment.java
Outdated
Show resolved
Hide resolved
google-maps/src/main/java/org/odk/collect/googlemaps/GoogleMapFragment.java
Outdated
Show resolved
Hide resolved
@@ -97,6 +99,18 @@ class ApplicationInitializer( | |||
} | |||
|
|||
private fun initializeMapFrameworks() { | |||
// Reset basemap setting if the currently selected is not available |
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.
If we install the app this code will be executed but what if we scan a QR code with maps that are not available?
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.
Ah good point. The QR code map would be set and used (even if it's not available), but it would be then be reset the next time the app is launched (as a fresh process). This could cause a crash I'd imagine if you used a QR code with Mapbox on a build without Mapbox and would give you the blank screen case for Google Maps (if there's no Google Maps key). I'm not sure whether this is worth fixing seeing as we expect all maps to be available in full release builds. What do you think?
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't we just run that code or a part of it after loading new settings to update maps?
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.
Yeah I guess we can pull this code to something shared and call it on settings change as well.
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.
So we can have a separate issue for that improvement? We can ask the QA team to verify this case to make sure it works in the way we assume and then file an issue.
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.
Yeah it's actually a little awkward to get that working right now as we'd need to rework things so that CollectSettingsChangeHandler
is able to actually change settings. I reckon we leave it for now and come back for it it gets annoying.
I'll add a note for QA.
dd2be86
to
ffb862f
Compare
The default map shown is Google (when a map is opened) but in the settings OSM is set. The maps change to OSM only after going to Settings -> Maps. Steps to reproduce:
|
Ah! It looks like it doesn't work for a fresh install when there are no projects as there's none to reset back to OSM. I'll fix that. |
Fallback to available ones on app start
This is starting to look and function like FormsDataService, so it makes sense to make it official.
2645150
to
7a058f5
Compare
@getodk/testers this is good for another look! |
Tested with Success Verified on device with Android 13 Verified cases:
|
Tested with Success Verified on device with Android 10 |
This will make development and testing with debug builds (that might not include Mapbox/Google Maps) easier. The app will now fallback to whatever mapping engine is available on start up.
I also moved all Google Maps into a dedicated module (like we already had for OSM and Mapbox), to make it easier for me to keep track of the various components in play.
For @getodk/testers: You should now be able to do test CI builds without Google Maps/Mapbox configured and not have to change to OSM (which should be selected automatically) so that's worth verifying. It's also worth verifying that local builds with Google Maps/Mapbox configured still work as expected. As @grzesiek2010 pointed out, scanning a QR code with Google Maps/Mapbox configured on a build that doesn't have it available will still select that map engine and probably cause problems.