Convert reference layer selection to bottom sheet in select one from map

[grzesiek2010]

Closes #5845
Closes #5851
Closes #5852

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:

From #5845

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. 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?

I think it will be better to these the whole feature at once when all the issues are addressed.

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!

[grzesiek2010]

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!

This is a thing I haven't had to touch even. There is a listener that is called whenever the settings are updated and then we update the map in the map fragment that we use (Google, Mapbox or OSM).

[seadowg]

Sorry I really think we should split this up. I've started going through it commit but commit, but because the PR builds things up iteratively (like d2517fe for example), I'd really like to be able to use "Files changed" to also keep track of the final state of things so I'm not commenting on intermediarey states. Just to be clear, I really like the step by step approach, it just hard to combine that with a partial review given the tooling we have.

This should be split after d6c9705 - another branch should be created there and this one should be git reset to a194524 and then pushed. I know managing stacked PRs is a pain, but I'm happy to help with the branch management here. I tend to use git rebase --onto to deal with stacked branch updates, and I've also been looking at Git Town's [stacked changes] tooling which I think we might want to start adopting as I think we most likely want to be doing this more to cut the size of reviews down.

[grzesiek2010]

@seadowg ok the first part is ready.

[seadowg]

@seadowg ok the first part is ready.

Thanks so much! That's going to be really helpful.

[seadowg]

Just something to consider: I'd be tempted to pass in the ViewModel or ViewModelProvider.Factory here instead of the ViewModel's dependencies. That would let you have more concentrated tests of the Fragment and ViewModel.

I don't think that would be very advantageous in this PR (OfflineMapsLayerPickerTest is pretty simple and easy to read), but it might be useful down the line if you wanted to test something like the ordering of layers (which would be easier to test at the ViewModel level).

This is definitely related to our earlier discussion on the injection setup, and I think I'm again suggesting the approach I put forward in #6118 (comment).

[grzesiek2010]

Let's keep this in mind but do not change now. I'm not against but I also think it's not something we definitely need to change.
When it comes to testing I already verify the order of displayed layers in OfflineMapLayersPickerTest.
Even if after merging this pr and creating a new one I change my mind and start thinking that yeah we should have it, it would be better to do that later so that I can cherry-pick the commits I've removed easier.

[seadowg]

It looks like the recycling here isn't tested.

It could be worth extracting a helper adapter for single select lists like this (like we already have for multi select in MultiSelectAdapter), but that potentially a follow up change.

[grzesiek2010]

Right, I wanted to avoid creating a custom view for list elements but that would make testing easier. Let's wait until it has its final form (expandable with the delete button).

[seadowg]

It's probably worth creating a specific data class to represent the view data here (rather than using Pair) just so it's a little more readable.

[grzesiek2010]

I agree and I came to the same conclusion but at a later stage (while working on deleting layers when I need even more details like for example if the layer item is expanded or collapsed). If it's ok, let's maybe keep it unchanged for now so that I can cherry-pick commits that will improve this code without any conflicts (in one of the next prs).

[seadowg]

Why is this here?

[grzesiek2010]

The RadioButton has a margin that I remove using this. I don't want it. I want the button to start from the edge of the layout without any extra margin. The TriggerWIdget uses the same.

[seadowg]

Ah I see now: offline_map_layers_picker.xml defines a guideline that the RecyclerView starts at so we don't need the built in margin in here. The problem there is that we end up with the start edge of the screen not being tappable, even though I'd expect the list item to be selectable across the full horizontal space. I think we should make the RecyclerView full width and remove this (although we might want to adjus the start padding to one of our standard margins).

[grzesiek2010]

I mean not so long ago we decided not to make the RecyclerView responsible for displaying choices in select_one/select_multiple questions full-width and I think that one is much more important so I thought it's ok. Do you really want it to be the way you described it? If so I'm ok to do that but to me it's not an issue.

[seadowg]

I think that was more to do with how we controlled widget margins. I don't think we need to do anything special here and can just let the RecyclerView be full width.

[grzesiek2010]

Ok, done.

[seadowg]

#6118 (comment)

[seadowg]

#6118 (comment)

[seadowg]

It's super confusing that you have to do this and can't just set some kind of padding on the MaterialRadioButton. Could you file an issue with this example at https://github.com/material-components/material-components-android? That really needs to get fixed.

[grzesiek2010]

Ok material-components/material-components-android#4198

[seadowg]

It might be a little more readable (for each test) to write a stub outside of Mockito here. For example:

private class StubMapConfigurator : MapConfigurator {

    private val files = mutableMapOf<File, Pair<Boolean, String>>()

    override fun supportsLayer(file: File?): Boolean {
        return files[file]!!.first
    }

    override fun getDisplayName(file: File?): String {
        return files[file]!!.second
    }

    fun addFile(fike: File, displayName: String, isSupported: Boolean) {

    }

    override fun isAvailable(context: Context?): Boolean {
        TODO("Not yet implemented")
    }

    override fun showUnavailableMessage(context: Context?) {
        TODO("Not yet implemented")
    }

    override fun createPrefs(context: Context?, settings: Settings?): MutableList<Preference> {
        TODO("Not yet implemented")
    }

    override fun getPrefKeys(): MutableCollection<String> {
        TODO("Not yet implemented")
    }

    override fun buildConfig(prefs: Settings?): Bundle {
        TODO("Not yet implemented")
    }
}

[grzesiek2010]

Done.

[seadowg]

@getodk/testers going to skip "needs testing" here so you can test the whole feature once managing tiles is added.