Dismiss the offline layers confirmation dialog on activity recreation #6243
Conversation
| @@ -116,6 +116,13 @@ class OfflineMapLayersPicker( | |||
| dismiss() | |||
| } | |||
|
|
|||
| if (sharedViewModel.layersToImport.value?.value == null) { | |||
There was a problem hiding this comment.
This is probably the right way to solve this, but it does feel like we're working against ourselves here a little to have to write specific code for this case.
Considering it more, I'm realizing an alternative would be to fully support this case with Fragment args - pass the list of URIs as an arg to OfflineMapLayersImporter (and then have it call loadExistingLayers) instead of communicating them via the ViewModel. I don't like this nearly as much, but it does feel like where Android is pushing us given we have state we want to this Fragment to be able between full recreations.
Another alternative (which wouldn't support full recreation) would be to rework the UI a little to not show OfflineMapLayersImporter until there layersToImport is available. This would prevent us from having to deal with the lifecycle problem at all.
What do you think? Perhaps we could look at one of those as part of #6198 and go with this approach (just avoiding the crash) for now?
There was a problem hiding this comment.
Another alternative (which wouldn't support full recreation) would be to rework the UI a little to not show OfflineMapLayersImporter until there layersToImport is available. This would prevent us from having to deal with the lifecycle problem at all.
This would require dismissing the confirmation dialog programmatically either way (that's the thing we don't like the most I guess). Plus I think it's better to display that dialog immediately and have the loading indicator there not in the first one (at least it makes more sense with the current UI).
I will take a look at the first option which was my first implementation and I liked it. If it doesn't require a lot of changes I will do that otherwise we can think about it later.
There was a problem hiding this comment.
I will take a look at the first option which was my first implementation and I liked it. If it doesn't require a lot of changes I will do that otherwise we can think about it later.
Sounds good!
There was a problem hiding this comment.
Ok I still like option 1 but I think we should leave it as is for now (merge this pr) and see if it's still the best solution once we figure out how we want to rework the UI in #6198
|
Tested with Success! Verified on a device with Android 14 Verified Cases:
|
|
Tested with Success! Verified on a device with Android 10 |
Closes #6219
Why is this the best possible solution? Were any other approaches considered?
We agreed in the issue that dismissing the dialog is the solution we want for now.
I wanted to handle that case in
OfflineMapLayersImporterbut it didn't seem to be easy so I did it inOfflineMapLayersPicker(the dialog that shows the confirmation dialog).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?
This requires ensuring that the dialog is being dismissed on activity recreation (the case described in the issue). This should not happen on configuration changes (for example when you rotate your device).
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:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest