-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce amount of index updates on UI thread #5916
Conversation
Other repeat actions need to stay sync so that they work in FormHierarchyActivity where we immediately call finish() after them.
This is an example of filling in a Kobo test form on Redmi 9T Android 10. I'm not sure if after reverting changes and adding this PR this is the expected result? XRecorder_19012024_093910.mp4 |
The fact that there is a progress bar is expected. Did it also take so much time on v2023.3.1 to navigate between the questions? |
I checked the same form on v2023.3.1 and it felt a bit faster (around 1 second faster when swiping to the next select question). I think a bigger change between 2003.3.1 and the PR is on Android 13. On 2023.3.1 swiping went smoothly and in the PR it feels just like Android 10 in the uploaded video above. |
As in the PR description:
Could you record the same flow using v2023.3.1 on that device so we can compare? My guess is that it's still just that it appears faster than it actually is, but it'd be interesting to see why. I think we're probably doing a bit more work while that loading screen is up than would happen in one "freeze" before so it's likely we end up rendering bits of the screen earlier in v2023.3.1. |
I created an automated test for Kobo Test form measuring time from clicking the form to open till going to the second question (on Android 10). RepeatoKobotestmaster.mp4 |
This is the store version test. RepeatoKoboTeststore.mp4 |
@dbemke could you try this with the latest v2024.1 beta as well? |
v2024.1.0 Beta 1 |
Interesting! So with or without showing the progress UI (I'm pretty sure we're not in v2024.1 Beta 1) it does seem like preloading choices (or some other change in v2024.1) results in slightly slower runs through the form on that device. @lognaturel do you have any other forms that would be good to test with? |
We didn't find any issues related to form entry in general and:
|
Closes #5229
Blocked by #5918This doesn't fix the problem for all cases in form entry, but should catch the majority of them. We'll now show progress when loading selects when:
Why is this the best possible solution? Were any other approaches considered?
There will still be many cases where we load select choices on the UI thread and don't show progress that are riskier to tackle like returning to a select question from an external app or when adding a repeat that starts with a select. These cases all require much more rework as they often involve navigation between activities which makes an async operation more tricky.
We (@lognaturel and I) previously discussed this and decided just targeting the highest traffic paths (like moving forward) was a good first step to reducing our ANRs and smoothing out performance in this area.
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 really be any obvious changes to the behaviour of the app so validating form entry (especially for forms with selects) is the best approach here. As we found before reverting the first attempt at these changes, it's likely that form entry might appear a little slower on fast devices: this is because we show that something is happening rather than letting the app freeze which can ironically feel "faster". We'll likely want to revise exactly how we display progress to improve this.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest
(or./gradlew testLab
) and confirmed all checks still pass