Handle unsupported files when importing .mbtiles #6230
Conversation
grzesiek2010
changed the title
| @@ -74,10 +76,6 @@ class OfflineMapLayersPicker( | |||
| private val getLayers = registerForActivityResult(ActivityResultContracts.GetMultipleContents(), registry) { uris -> | |||
| if (uris.isNotEmpty()) { | |||
| sharedViewModel.loadLayersToImport(uris, requireContext()) | |||
| DialogFragmentUtils.showIfNotShowing( | |||
There was a problem hiding this comment.
I feel like this would all be a bit more contained, if OfflineMapLayersImporter handles the warning cases? layersToImport could still be a Consumable, but it wouldn't care about if it had consumed or not for setting the adapter. In that world, there's no changes in OfflineMapLayersPicker and in OfflineMapLayersImporter we just get something like:
viewModel.layersToImport.observe(this) { layersToImport ->
val adapter = OfflineMapLayersImporterAdapter(layersToImport.value.layers)
binding.layers.setAdapter(adapter)
if (layersToImport.isConsumed()) {
// Show warning dialog if it make sense
layersToImport.consume()
}
}For me that keeps the navigation simpler. What do you think?
There was a problem hiding this comment.
But OfflineMapLayersImporter should not be displayed at all if all the selected files are not supported right?
There was a problem hiding this comment.
True! In the case where we don't want to offer import (there's no supported layers), we can just dismiss the imported. So to expand my previous example:
viewModel.layersToImport.observe(this) { layersToImport ->
val adapter = OfflineMapLayersImporterAdapter(layersToImport.value.layers)
binding.layers.setAdapter(adapter)
if (layersToImport.isConsumed()) {
if (layersToImport.value.numberOfUnsupportedLayers == layersToImport.value.numberOfSelectedLayers) {
dismiss()
showNoSupportedLayersWarning()
} else if (layersToImport.value.numberOfUnsupportedLayers > 0) {
showSomeUnsuppotedLayersWarning()
}
layersToImport.consume()
}
}That prevents us from having to deal with multiple dialogs opening in the picker (maybe that's OK, but it feels off to me) and I still prefer that handling the layers to import all happens on the importer side.
grzesiek2010
force-pushed
the
COLLECT-6199
branch
from
7c76aba to
eccd18d
Compare
|
|
||
| val context = ApplicationProvider.getApplicationContext<Application>() | ||
| onView(withText(context.getLocalizedQuantityString(R.plurals.non_mbtiles_files_selected_title, 1, 1))).inRoot(isDialog()).check(matches(isDisplayed())) | ||
| onView(withText(R.string.all_non_mbtiles_files_selected_message)).inRoot(isDialog()).check(matches(isDisplayed())) |
There was a problem hiding this comment.
It'd be cleaner for these tests to use the helpers available in Assertions/Interactions, but I'm pretty sure that's something you were going to deal with in #6201 anyway.
|
@getodk/testers the warning messages shown here will not be maintained when rotating the device - that's OK and not something we'd look to change right now. |
|
Tested with Success! Verified on a device with Android 10 Verified Cases:
|
|
Tested with Success! Verified on a device with Android 13 |
Closes #6199
Why is this the best possible solution? Were any other approaches considered?
The way we should inform users about unsupported files they selected has been discussed in the issue. When it comes to the implementation there is nothing I would like to discuss here.
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?
According to the issue, a warning should be displayed if some/all the selected files are not supported.
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