Maps settings page with a "Base layer" section; offline layers button support integrated with preferences

[zestyping]

Issues: Closes #3129. Closes #3130. Closes #3090.
Scope: Maps settings

User-visible changes

This reorganizes the settings page for Maps to have a "Base layer" section, where the user can select the base layer in terms of an overall "type" (Google, Mapbox, etc.) and then options specific to that type.

Internally, there is now a "BaseLayerType" interface that each of these can use to provide type-specific options and construct the appropriately configured MapFragment.

Guidance for reviewers

I recommend reviewing each commit separately, in order.

Verification performed

I opened maps using each of the three SDKs (Google, Mapbox, OsmDroid) and used the layer button to repeatedly add and remove offline layers, and confirmed that raster tiles appear and disappear as expected in all three, and vector tiles appear in Mapbox.

I used the preferences to change map styles (e.g. Google Terrain vs. Hybrid, or Mapbox Light vs. Dark), and confirmed that these changes still take effect.

Screenshots

See more screenshots below.

[zestyping]

Here's that base layer type menu:

In addition, there is now a "Reference layer" section that shows a list of the supported files in the /odk/layers directory. The set of options there can change depending on the base layer type.

[lognaturel]

Too -> to 😬

[zestyping]

Oops!

[zestyping]

Okay, I believe we are now feature-complete. Here's how the new preference screen looks:

and here's the dialog you get when you tap "Layer data file":

If the dialog has too many items, the items will scroll:

If there are no valid files, the caption at the bottom of the dialog will change:

And we can bring up the same dialog from within an activity:

Yes, the dialog is styled differently. This is because we apply a different theme to the settings than to the rest of the app.

We can also bring up the whole Maps preferences page as a fragment within any activity:

Again, it's styled differently because of the theme.

Now, we have a decision to make about the layers button. It could do either of these two things:

• It could bring up the reference layer dialog in the geo activity. This is more similar to the current behaviour.

• It could open the whole Maps preferences fragment. This offers more capability, e.g. you can also change the base layer settings without exiting the activity, though the base layer source is disabled because it doesn't make sense to change it while you're in an activity..

The current commit (ea1ba21), illustrates both behaviours, so you can try it and see how they feel. In GeoShape, you get the first behaviour and in GeoPoint you get the second behaviour.

Thoughts?

[lognaturel]

Impressive work! This turned out to be much bigger than I anticipated and I'm glad to see some really nice refactoring. Sorry about the dialog woes. :/ It looks really good now, though! The short narrative and including the full paths will hopefully help make these offline layers feel less like magic (CC @russbiggs).

I'm working through a more thorough review but wanted to give an initial reaction to your question about showing just the layer dialog vs the whole preference fragment. I'm conflicted. I was imagining just the dialog for now and rethinking what the layers button does in the context of adding the data (geoJSON) layer and creating a feature-selection widget. But perhaps showing the maps preference fragment is exactly what we'd want our end state to be. Now that I see it, that seems like a great option for the long term. Is that what you're thinking, @zestyping?

@russbiggs, if you have a moment to share your thoughts on the interface, that'd be great!

[russbiggs]

I also really like the full paths, I think it helps make the storage very clear. I also appreciate the dialog for when there are no valid layer files, this could be a very confusing problem for a data collector if invalid or no layers were added. I might try a local build of the app from this branch to poke at it some more. Great work @zestyping!

[zestyping]

I was imagining just the dialog for now and rethinking what the layers button does in the context of adding the data (geoJSON) layer and creating a feature-selection widget. But perhaps showing the maps preference fragment is exactly what we'd want our end state to be. Now that I see it, that seems like a great option for the long term. Is that what you're thinking, @zestyping?

Possibly! It depends what the use cases really are for enumerators changing their layers while collecting data. I honestly don't know how the feature gets used. It's not a big deal, though—I've gotten it to a point where it's a small and easy change to do it either way (as long as we're willing to tolerate the style inconsistencies between the Settings theme and the rest of the app).

[zestyping]

Oh, also:

I'm working through a more thorough review

I should warn you that the commits are not organized nicely yet! I did explore a few garden paths on the way here, and I don't want to waste your time reviewing experiments that I ultimately reverted.

Also, today I've been trying to clean up the way that listening to preferences is done. Firstly, the way the listeners are attached in the current commit, they never get detached, and there's a risk of not only a memory leak but maybe a crash if a listener wakes up and tries to poke at a map that has long since been discarded. Secondly, I've been conflicted because the easiest place to listen for preference changes is in the MapFragment implementations themselves—but doing that duplicates some code among the three MapFragments, and moreover ties the MapFragment intimately with the preferences, which is the opposite of what we want for the future! The layer settings and so on need to be able to come from the preferences (in which case they should update when the preferences update) or from something packaged with the form, so I don't want to go too far tying them in with preferences.

I've arrived at a plan that I like: the MapFragments will take a configuration Bundle that tells them what map type, layers, etc. to show, and commit to being able to update themselves when provided with a new Bundle. This sacrifices some efficiency (we pass a whole new configuration each time a single preference changes), but it simplifies things and I think it's worth it. The individual BaseLayerSources can decide how to form their Bundles, so the preference-specific stuff is kept out of the MapFragments, and the MapConfigurator can do all the listening in one place: all it needs to know is which preference keys to care about, and if any relevant preference changes, it asks the BaseLayerSource to make a new configuration Bundle, and hands it off to the MapFragment.

This will be visible soon on this branch!

[zestyping]

One note, with this change "BaseLayerSource" is feeling increasingly awkward as a name. These things are more like MapFragment-specific configurators, e.g. "a thing that knows how to configure a GoogleMapFragment", "a thing that knows how to configure a MapboxMapFragment", where the MapConfigurator is the top-level thing that calls on these sub-configurators. But I haven't been able to think of a good name that also has consistency with something we can show in the UI that makes sense.

[lognaturel]

Possibly! It depends what the use cases really are for enumerators changing their layers while collecting data. I honestly don't know how the feature gets used.

Originally it was the only way to set offline layers. Let's go for the dialog for consistency with the current approach, then. We can gather more information and decide what to do. My hunch is that allowing form creators to specify offline layers in the form will mostly make the button unnecessary. If an enumerator really needs to change something about the map display, they could go to settings from the question before launching the geo widget.

the MapFragments will take a configuration Bundle that tells them what map type, layers, etc. to show

That sounds good.

"a thing that knows how to configure a GoogleMapFragment", "a thing that knows how to configure a MapboxMapFragment"

And the Web Map Service one doesn't fit that pattern because it could (in principle) configure any, right?

[lognaturel]

Quick thing for you to consider, @zestyping.

Given that these source names are all organization names, I'm not sure they should be translated. I think they should go in untranslated.xml so that they're guaranteed consistent across languages. I imagine some languages might have translations for common brand like Google but I'm sure they also understand the official (English) name.

Then could the map style text be represented as a single parameterized resource? That is, in English it would be %s map style and in French it might be Style de carte %s.

[zestyping]

Oh, interesting. Yeah, that's a fine point. I'll move them there, and make the parameterized string as you suggest.

[zestyping]

This is done now, in the commit with the description "Avoid translating basemap source labels...". The code is slightly more awkward, but it saves a bunch of translations so it's probably worth it.

[lognaturel]

Another one that was kicking around my brain -- since the style strings are being changed and will need to be re-translated anyway, I think we should take a moment to confirm they make as much sense as possible. I'm happy to do a follow-on issue/PR if it feels like a distraction.

Here's what I'm thinking:

• pull out "generic" style names that can be reused. That is, "Streets", "Terrain", "Satellite", "Hybrid" are the same regardless of the source. They should always be represented by the same string. They can be map_style_streets, etc.
• for the USGS maps, I would drop the "National" and expand "Topo" to "Topographic" as a generic style name. I'd also use the standard "Satellite" instead of "Imagery." So USGS would have three standard style options: Topographic, Satellite and Hybrid.
• Mapbox, Carto, Stamen would continue to have custom names.

[zestyping]

All makes sense! I'm trying to prioritize getting you readable commits for now (as I'll be offline for the next couple of days), and then maybe I can do this after (on this PR or another)?

[lognaturel]

That sounds perfect.

[zestyping]

This is now done. I took the liberty of labelling "Mapbox Satellite Streets" as "Hybrid" as well, since that's what the "Hybrid" option does for all the other basemap sources.

This means Mapbox doesn't use any custom names any more—"Streets", "Light", "Dark", "Satellite", "Hybrid", and "Outdoors" are all generic, translatable words. None of them do, except Carto; the only remaining custom names are "Positron" and "Dark Matter".

[zestyping]

Yikes! I just discovered this:

mapbox/mapbox-gl-native#15182

Not certain what the workaround should be. The effect of this problem is that it isn't safe to remove a reference layer while the Mapbox map is running—in other words, if we want the user to be able to change layers from the layers button, they can safely switch from "None" to a reference layer, but then switching from that to any other layer (or back to "None") triggers the segmentation violation. I suppose we could throw away and restart the map in that case, though it would mean cutting an awkward path through these nice map configuration interfaces. Let's see how they respond.

[zestyping]

Fixed these:

• undelete jacoco config (deleted in 1c6396a)
• remove https://github.com/opendatakit/collect/pull/3164/files#diff-103454ae2451bc94954ffd6ac2409bc7
• bring back FormMetadataMigrator / AutoSendPreferenceMigrator -- it looks like those still use the new pref migration framework

[lognaturel]

Hurray! I've done another sweep through and the code is looking great to me. As far as I know, the remaining action items are:

• Decide what to do with mbtiles files that don't specify a format (e.g. http://geoodk.com/downloads/mbtiles/MapBox_Demo_Layer.zip)
• Explore reducing spacing in layers dialog

[zestyping]

Thanks for noticing this, @mmarciniak90!

the area which is scrolled is very small but the area below with text is quite big.

That's a good point. @zestyping would it be possible to make the spacing above the "Above are all the..." text the same as the spacing below? I think that would help a lot.

I've done this, though it's only a small improvement. In the theming shown in @mmarciniak90's screenshot, there's a blue line under the dialog title so it would make sense to remove the space between that blue line and the top of the scroll region. But unfortunately, on a newer device, there is no blue line (or the padding around it), so removing that space would cause the scroll region to bump right up against the title of the dialog, which doesn't look good.

I removed the bottom margin between the caption and the "CANCEL" button as well, which helps a bit.

I'm not sure what else to do. I could specify TextAppearance.Small for the caption, but it's also visible from the screenshot that TextAppearance.Small is the same size as TextAppearance.Medium on @mmarciniak90's device. (I've found that sometimes it's the same and sometimes it's smaller, depending on the Android version.)

[zestyping]

Okay! I've added autodetection, and tried it with several files including Mapbox_Demo.

[lognaturel]

The spacing change looks great to me. I thought target API was supposed to save us from headaches around different looks across Android versions? Regardless, I think it's enough.

Auto-detection is fantastic. 🙏

Couple of checkstyle and lint complaints and then this looks good to go.

[zestyping]

Couple of checkstyle and lint complaints and then this looks good to go.

👍

"Unicode escape(s) usage should be avoided."

Oh, come on! 😝

[zestyping]

Phew. This has been a long road. Taking a break for a bit.

[mmarciniak90]

Tested with success

Verified cases:

• light/dark mode
• offline layers with Mapbox are supported
• clicking on the radio buttons in Layer data file dialog
• set layer in Geo question with map appearance
• set layer in Settings
• offline map from http://geoodk.com/downloads/mbtiles/MapBox_Demo_Layer.zip
• put mbtiles files directly in /layers directory and in a subdirectory
• the file path is visible on Layer data file dialog but it is not visible under Reference layer
• USGS map style contains: "Topographic", "Satellite" and "Hybrid"

Verified on Androids: 4.2, 4.4, 5.1, 6.0, 7.0, 8.1

@opendatakit-bot unlabel "needs testing"
@opendatakit-bot label "behavior verified"