Convert reference layer selection to bottom sheet

[grzesiek2010]

Closes #5845

Why is this the best possible solution? Were any other approaches considered?

Generally, everything has been implemented according to what we had discussed earlier. The only difference might be related to this acceptance criteria:

Given I'm filling out a form with a select one from map question
And I've copied reference layers to both the global and the current project's directory
When I open the select one from map view
And I press the layers button
And I swipe the bottom sheet up above the middle of the screen
Then the bottom sheet should fill the screen

The bottom sheet can be expanded but only if it contains a long list of layers to choose. Otherwise, it's not possible because everything is visible and there is no need to do that. This is the default behavior and I think it doesn't make sense to change it.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There should be a new map layers picker used everywhere the old one was used:

• Settings
• Select one from the map
• Geo widget

Please test the new picker with a long list of offline layers to make sure it behaves well. Please keep in mind that different map providers support different layer types: Google and OSM support only raster layers but Mapbox supports both raster and vector layers. It's important because unsupported layers should not be visible when you switch between those map providers and additionally, when a vector layer is set in Mapbox it should be set to None once you switch to Google or OSM since as I already said it's not supported there.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

Before submitting this PR, please make sure you have:

• added or modified tests for any new or changed behavior
• run ./gradlew connectedAndroidTest (or ./gradlew testLab) and confirmed all checks still pass
• added a comment above any new strings describing it for translators
• verified that any code or assets from external sources are properly credited in comments and/or in the about file.
• verified that any new UI elements use theme colors. UI Components Style guidelines

[grzesiek2010]

@seadowg could you take a look at the test that is failing here please?

collect/geo/src/test/java/org/odk/collect/geo/selection/SelectionMapFragmentTest.kt

Line 407 in b3cd4a0

fun `clicking layers button navigates to layers settings`() {

Here is the reason:

Caused by: java.lang.ClassCastException: class org.odk.collect.geo.support.RobolectricApplication cannot be cast to class org.odk.collect.maps.MapsDependencyComponentProvider

The test is in the geo module and it opens the list of layers (before it was a dialog now it's a bottom sheet I called OfflineMapLayersPicker). OfflineMapLayersPicker is in the maps module though and I use dagger to inject dependencies there https://github.com/getodk/collect/pull/6118/files#diff-9e6c9d8d539aa94749d2b7fef8bb4c48090de8859f4484a80945ebb71fdff5ffR37 this line causes the problem because I can't cast org.odk.collect.geo.support.RobolectricApplication to org.odk.collect.maps.MapsDependencyComponentProvider. Do you think solving this is possible? How would you fix this?
You might ask why I didn't use a fragment factory (or a ViewModel) to pass dependencies via constructor so that injecting with dagger wouldn't be needed. The answer is that the new bottom sheet is used in at least three different places so I would need to pass dependencies, I need (there are a few of them) to all those three places and there, use a fragment factory to build my fragment. This doesn't sound easy to maintain so I decided to use dagger. Alternatively, I could have built one fragment factory (with all the dependencies I need) and inject it into those three separate classes which would be easier than injecting all the dependencies I need (3 or 4) separately. But still, that would mean injecting the factory into those three places plus this is not how we have used fragment factories so far.
So basically I still think using dagger to inject dependencies to OfflineMapLayersPicker is the best way but can we solve that problem with tests?

[seadowg]

The test is in the geo module and it opens the list of layers (before it was a dialog now it's a bottom sheet I called OfflineMapLayersPicker). OfflineMapLayersPicker is in the maps module though and I use dagger to inject dependencies there https://github.com/getodk/collect/pull/6118/files#diff-9e6c9d8d539aa94749d2b7fef8bb4c48090de8859f4484a80945ebb71fdff5ffR37 this line causes the problem because I can't cast org.odk.collect.geo.support.RobolectricApplication to org.odk.collect.maps.MapsDependencyComponentProvider. Do you think solving this is possible? How would you fix this?

The RobolectricApplication in geo needs to implement MapsDependencyComponentProvider as well as GeoDependencyComponentProvider (just like how Collect implements both of them).

I do think that having Dagger dependencies go down multiple layers (geo and then maps) in the dependency hierarchy feels awkward though (although we've probably already done it somewhere else). One point to make here is that we should really be using a ViewModel in OfflineMapLayersPicker instead of talking to Scheduler directly as otherwise you'll run into problems with device rotation.

I think a setup like MultiSelectListFragment would be best here: OfflineMapLayersPicker takes a ViewModel (or View Model Factory) and ExternalWebPageHelper in as constructor args and then the parent Fragment SelectionMapFragment passes them via a FragmentFactory on its child FragmentManager. SelectionMapFragment can either construct these (with injected dependencies) or just pass them along from its own constructor. This would be similar to how MultiSelectListFragment is used in DeleteBlankFormFragment. I think my preference overall is to avoid Dagger in components that don't need them, and that's thankfully now true for Fragments.

Happy to discuss more here on Slack if that's not clear!

[grzesiek2010]

@seadowg please take a look at this button. The styling does not work well. If it was an activity everything would be fine (for example we use the same styling in the crash-handler module and there it works well). There is something wrong with using it in fragments in separate modules I guess. Do you maybe know what is wrong?

[seadowg]

Oh wow that's weird! It looks to me like bottom sheets might get their own theme overlay set on them (although I haven't looked into it deeply). The hint for that is that you can set style to @style/Widget.Material3.Button and get what you'd expect, but using ?materialButtonStyle is just giving us a bolder text button. My guess is that bottom sheets have an overlay theme that overrides ?materialButtonStyle and that we might need to customise and set our own theme for them (like we do with dialogs). The Material Components repo on Github is probably a good place to look into.

[grzesiek2010]

Yeah, it looks like we need to set our own theme for bottom sheets.

[seadowg]

#6118 (comment)

[seadowg]

I've just done a quick overview of the general structure and had one comment.

Also, I don't have an any mbtiles to try out, but how does the map reload with the new layer? It seems like the underlying setting changes when the user selects a tile, but it's not clear to me what then triggers that being used. I'm probably missing something obvious!

[seadowg]

I think I'd prefer it if we just added providers for ReferenceLayerRepository and SettingsProvider here. We could then avoid a specific implementation of ViewModelProvider.Factory for OfflineMapLayersPickerViewModel and just have a shared view model factory for geo components that can construct one (like we do with form entry). This approach keeps a more specific set of components "injectable" (the "building blocks") and should reduce the amount of view model factories we need.

[grzesiek2010]

I think I'd prefer it if we just added providers for ReferenceLayerRepository and SettingsProvider here. We could then avoid a specific implementation of ViewModelProvider.Factory for OfflineMapLayersPickerViewModel and just have a shared view model factory for geo components that can construct one (like we do with form entry).

I see what you mean. I think the reason I did this in that way was that I wanted to have one class to wrap the three dependencies I need (ReferenceLayerRepository, Scheduler, SettingsProvider and potentially more when I add importing/deleting layers) so that I don't have to inject all of them separately in GeoPolyActivity, GeoPointMapActivity, SelectionMapFragment and MapsPreferencesFragment (every place where we use the new maps picker). Do you think converting OfflineMapLayersPickerViewModel.Factory to a more general one (but still injecting it in the same way) that would in the future as you said work like FormEntryViewModelFactory would be a good trade off here?

[seadowg]

I think the reason I did this in that way was that I wanted to have one class to wrap the three dependencies I need (ReferenceLayerRepository, Scheduler, SettingsProvider and potentially more when I add importing/deleting layers) so that I don't have to inject all of them separately in GeoPolyActivity, GeoPointMapActivity, SelectionMapFragment and MapsPreferencesFragment (every place where we use the new maps picker).

I think it's ok to have all those injections. I like the model of dependency injection just handling the edges of the system. The hope there is you end up with more being injected into entry point "black boxes" (like Activity objects), but less actual depdencies being managed by the framework with plain ol' constructors doing most of the work.

Maybe a nice way of having our cake and eating it here is to create two Factory implementations - one for geo (GeoViewModelFactory) and one for MapPreferenecesFragment in collect_app (MapPreferencesViewModelFactory). Then we could just use @Inject on their constructors. You still only have to deal with one @Inject in both places, but we still don't have declare the factory as a dependency of geo anywhere. I'd thought about doing this for FormEntryViewModelFactory before, but hadn't got round to it. I like this approach as it gives us the convenience of Dagger in the Activity/Fragment, but doesn't mean managing dependencies outside (in the module etc) and you still have a simple ViewModel factory constructor.