Add info button to trace/shape questions#7021
Conversation
376d119 to
ff5d41b
Compare
|
Looking great - this will be so helpful beyond the invalid polygon work too! Explaining to data collectors how to use the different map actions has come up on the forum before, and it came up again just the other day. Small thing - I noticed we reuse the same icon for tapping and moving. I was think we should use the location marker for tapping instead so we don't repeat the icon and they look more distinct. |
ff5d41b to
fe54103
Compare
seadowg
left a comment
There was a problem hiding this comment.
These needs tests. It might be awkward to mix and match Espresso/Compose tests, but I think at least having tests for which items are shown in which "mode" would be enough.
If GeoPolyFragment stored the recordingAutomatic and recordingEnabled state in GeoPolyViewModel we could have a single Composable that gets passed that view model (and is contained in the ComposeView). That would make testing the bulk of the logic here pretty simple.
c7ec087 to
73b35b8
Compare
test-shared/src/main/java/org/odk/collect/testshared/DummyActivity.kt
Outdated
Show resolved
Hide resolved
73b35b8 to
090a449
Compare
090a449 to
800e0a2
Compare
|
|
||
| @Before | ||
| public void setup() { | ||
| ApplicationProvider.getApplicationContext().setTheme(com.google.android.material.R.style.Theme_MaterialComponents); |
There was a problem hiding this comment.
Why is this change needed? If I take it out, then there's a failure, but it seems completely unrelated to everything else here.
There was a problem hiding this comment.
This happens because androidx.compose.ui:ui-test-manifest provides its own test manifest, which changes the default app theme used during tests. Robolectric picks up that manifest even for non-Compose tests, and the new default theme is not AppCompat/Material.
|
|
||
| private fun clear() { | ||
| inputActive = false | ||
| viewModel.disableInput() |
There was a problem hiding this comment.
Do we need this? The tests pass without it and I think whenever we call clear we've already had to call GeoPolyViewModel#stopRecording right?
There was a problem hiding this comment.
I think whenever we call clear we've already had to call GeoPolyViewModel#stopRecording right?
No, it’s not guaranteed. I didn’t think much about whether it’s mandatory here or not I just tried to mirror the old implementation. So if we used to set the flag to false here, I’m doing the same now.
There was a problem hiding this comment.
Gotcha. I can maybe just pull it out in the next PR if it's not needed.
geo/src/test/java/org/odk/collect/geo/geopoly/InfoDialogTest.kt
Outdated
Show resolved
Hide resolved
|
Tested with success Verified on Android 16 Verified cases:
|
|
Tested with success Verified on Android 14 |
|
Tested with success Verified on Android 10 |

Closes #6961
Why is this the best possible solution? Were any other approaches considered?
The main thing worth mentioning is that I built the dialog’s UI using
Jetpack Compose, but I wrapped it inside aComposeViewso it can be used from a regularDialogFragment. The rest of the screen where this “info” dialog is opened is still based on the legacy XML view system, so embeddingComposethis way keeps the integration simple and avoids a bigger refactor.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 is a fairly isolated change. It should be enough to ensure that the correct content is displayed depending on the circumstances described in the issue.
Do we need any specific form for testing your changes? If so, please attach one.
Any form with
geotrace/geoshapequestions.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