getodk
Convert reference layer selection to bottom sheet
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:
- [x] added or modified tests for any new or changed behavior
- [x] run
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still pass - [x] added a comment above any new strings describing it for translators
- [x] verified that any code or assets from external sources are properly credited in comments and/or in the about file.
- [x] verified that any new UI elements use theme colors. UI Components Style guidelines
@seadowg could you take a look at the test that is failing here please?https://github.com/getodk/collect/blob/b3cd4a057832c48ab04c0c380274e74e4ee28fe2/geo/src/test/java/org/odk/collect/geo/selection/SelectionMapFragmentTest.kt#L407 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?
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!
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).
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 d2517feea771f732e23fd651b7790d04716ec7a9 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 d6c9705e12639dbc3a1bc09dc61fde08188a919d - another branch should be created there and this one should be git reset to a1945243e69f927c2a39c67bdc357065984ad8c3 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.
@seadowg ok the first part is ready.
Thanks so much! That's going to be really helpful.
@getodk/testers going to skip "needs testing" here so you can test the whole feature once managing tiles is added.