Rework answering select one from map questions#6973
Rework answering select one from map questions#6973grzesiek2010 merged 6 commits intogetodk:masterfrom
Conversation
a1ffb1e to
70c9389
Compare
|
@getodk/testers marking this as "needs testing" early to gain confidence that this doesn't break anything before moving on! |
|
I've noticed something new (I haven't seen it on the master version or the store version). After moving from the map back to the question answers "appear" - I can see the moment when they appear. Answers used to be displayed the moment I was moved back to the question (from the map). Has this thing changed? XRecorder_27112025_134431.mp4 |
Yeah I'd expect the answer to take milliseconds longer to show with the new approach as we re-render the question rather than just updating it. This looks fine in the video, maybe it would be good to test on the slowest device you have and with field lists? |
You're right. I can see it on a field list, too. After re-rendering the question the field list is scrolled to the top. XRecorder_28112025_074228.mp4 |
Am I right in thinking the only problem here is the jump back to the top in the field list? Or does the delay feel long enough to be a regression? |
That it reloads questions so I guess that's why the field-list is scrolled to the top. Scrolling the field list is a regression. |
On the slowest device I could find the delay looks like a feature - if I change the answer I can see values changing so it feels ok to me. |
|
@dbemke thanks! Ok so it looks like the key thing to fix here before we can move ahead is the field list jumping back problem. |
Yes. We finished testing. |
11298e1 to
442db6e
Compare
@dbemke this should be fixed now. It also should do it's best to scroll back to the right place if the field list questions change after answering the select one from map question (making others not relevant for example). There's a slight flash when returning from the map to the form in field lists, but I think we can live with that for now. If that's the only problem right now, I think filing an issue is better than blocking this change at this stage so we can move on to the rest of the |
I filed a separate issue ##6982 but I'm mentioning it also here as this issue is a potential "data loss" one (the geometry in geopoint, geotrace and geoshape isn't saved after using select one form map on a field list). |
|
@dbemke oh wow! That would be a blocker as it'd be a big regression. I'll copy that in here and close the issue. |
|
Issue from @dbemke: Unable to save geopoint/trace/shape after saving select one from map on a field list XRecorder_04122025_074724.mp4Steps to reproduce the problem
|
|
Filling question after select one from map questions doubles questions on a field list. XRecorder_04122025_100337.mp4Steps to reproduce:
|
|
@dbemke I'm going to just revert to the version that scrolls back to the top when returning from the selection map. As far as I can tell, that version didn't exhibit either of those two (pretty bad) bugs. We can file the scroll issue separately after merging this and that lets us move on to getting the rest of the |
442db6e to
d5acd9e
Compare
|
Tested with Success! Verified on devices with Androids: 7.1.1, 8.1, 10 Verified cases:
|
|
Tested with success Verified on Android 16, 11 |
collect_app/src/main/java/org/odk/collect/android/formentry/FormEntryViewModel.java
Outdated
Show resolved
Hide resolved
| formController.stepToNextScreenEvent(); | ||
| } | ||
|
|
||
| updateIndex(true, null); |
There was a problem hiding this comment.
This refreshes the entire screen instead of comparing all the questions and updating only when necessary. I think it has performance implications and isn’t what we want to keep, right?
There was a problem hiding this comment.
That's correct. There most likely will be two changes to performance:
- We "recompute" (commit an answer, recalculate dependent calculations, fire events etc) on a background thread instead of a foreground thread which blocks the UI (with a loading animation)
- Instead of the smarter re-rendering done by field lists on every change (on every keystroke for example) we re-render the whole "page" (as you point out)
I think 2 will have some impact on larger field lists, but personally it feels like an allowable trade-off for now? It will only affect "select one from map" and (eventually) geotrace/shape questions in the next release and I think the context of returning from one screen to another will most likely make this hard to notice. I initially did try to reuse the code field list re-render code here, but ran into multiple problems, and it felt like fixes would introduce a lot of risk to solve a pretty isolated problem. We could look to improve performance as part of our fix for #6984 (which I think we can do in parallel to other invalid polygon stuff once this merged), and it might actually be the best route to a fix.
Either way, I still feel comfortable with the performance regression for just those two question types (and just in field lists).
There was a problem hiding this comment.
I’m not happy with how this works at the moment, and it’s not how it should be in the long run. However, if it’s preventing you from making progress on the polygons, we can leave it as is for now, just let’s make sure we revisit it shortly afterward
There was a problem hiding this comment.
Understood! I'll add a note to #6984 to make sure the potential performance regression is captured as well.
Work towards #6969
Why is this the best possible solution? Were any other approaches considered?
This reworks/simplifies how the answer from a "select one from map" question is saved. The idea here was to make it as simple as possible to use the same approach in the
geoshape/geotracequestions withincremental=true.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 shouldn't be any observable change in behaviour. The change is isolated to answering select one from map questions so that's the only thing that needs to be tested. Things that come to mind that there could be problematic interactions with are audit logs, appearances and savepoints. There were no changes to any of the map code so no need to test with different map settings.
There's an issue which we'll need to come back and fix. For the moment, I think merging this is higher priority as it unblocks further
incremental=truework.Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest