-
Notifications
You must be signed in to change notification settings - Fork 613
Fix #5959 Profiles Flicker issue #6063
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
base: develop
Are you sure you want to change the base?
Conversation
|
Thanks for submitting this pull request! Some main reviewers have taken time off for the next few weeks, so it may take a little while before we can look at this PR. We appreciate your patience while some of our team members recharge. We'll be fully returning on 05 January 2026. |
|
Hi @itsadityaaaaa, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
|
@BenHenning, I understand that there have been previous attempts to use Diff Utils that caused some issues, i.e. linked PRs in #171 and #1627. Should we explore other possible solutions to this issue? |
|
Apologies--I didn't realize this was assigned to me. We discussed this briefly in the team meeting this week @itsadityaaaaa. As @adhiamboperes mentioned we have tried diff utils multiple times in the past--it's very hard to get right particularly for the core lesson player. I'm not sure we should have suggested this as the main fix for #5959 actually, which in turn means that's not quite a starter issue. Beyond that, I think the main problem here is we need understand why the flickering is happening. The profile colors are supposed to be fixed within a user's
I think we need to figure out what's actually wrong at the data level, then work backwards to understand why it's wrong. This is what I recommend you do:
From there, one of us can probably provide further advice or specific suggestions. Note that it may be the case that diff util is still the best way to fix the issue, but we need to understand exactly why it's broken before we can be confident in that solution. Also if that does happen, we'll want to update the binding adapter to support enabling diff utils since we'll really only want it enabled for the profile list for now (to avoid the bugs elsewhere in the app that it will cause). |
1a283d1 to
45e250c
Compare
|
@BenHenning @adhiamboperes Thank you for your suggestions, I tried to use stable ID's to fix the flickering. It reuse view holders for items with the same stable ID, preventing full rebinds that cause UI flicker. I build and investigate and found It patches the flickering, Is it the correct approach or should I have to investigate further? |
Coverage ReportResultsNumber of files assessed: 1 Exempted coverageFiles exempted from coverage
|
@itsadityaaaaa please take a closer look at my earlier message. Finding a fix without understanding the solution is an inherently risky strategy (see https://en.wikipedia.org/wiki/Root-cause_analysis for why root-cause analysis is a preferred methodology). We actually need to understand what the root cause is to be sure the diff utils solution not only is actually correct (logically, not just observationally) and that it's the best solution we can take (considering it has fairly significant potential drawbacks without a fully thought-out solution). To that end, your next steps should be to work through the items in my previous comment and write up a debugging document: https://github.com/oppia/oppia-android/wiki/Debugging-Docs. |
|
@BenHenning Thanks for your guidance I tried to find the root cause and it seems like it causes due to full rebind. Please take a look at this doc https://drive.google.com/file/d/1G9Tyi7odQenHtqqQ5DPsrjUeRYW0p2AL/view?usp=sharing, I tried to use DiffUtil as a potential solution earlier which leads to many pre-written UI tests failure. |
|
Unassigning @itsadityaaaaa since a re-review was requested. @itsadityaaaaa, please make sure you have addressed all review comments. Thanks! |
Thanks @itsadityaaaaa. Can you create a debugging document, start filling that out, and share it? It's important to also collaborate on the steps to debug, not just findings. In this case I think we actually know already that this is a full rebind situation since that's how |
|
@BenHenning Thanks again for your guidance. I’ve put together a debugging document and wanted to confirm the approach https://docs.google.com/document/d/1CkVmUMNSJeCQ0mXIPxIedZRqGKKeBSwx3FERdsn2iy0/edit?usp=sharing I found out BindingAdapter in ImageViewBindingAdapters.java was not resetting ImageView state, so recycled views could briefly retain stale color filters until the async Glide load completed, causing the flicker. I implemented a minimal fix that makes the adapter idempotent seems like it solves the issue. Does this seem like the right direction, or would I have to prefer a different approach? |
|
@BenHenning Apologise for my earlier comment, I have tested it further and found out the flickering doesn't happen until we have more than 5 ID's(with or without changes). I had tested it with 5 or lesser ID's so it seems the approach work but now I think I have to find other possible causes. |
|
Thanks @itsadityaaaaa this is a really helpful investigation, and thanks especially for the doc. Just to check: does the state resetting fix not work for cases when there are more than 5 profiles? I wasn't completely clear on your most recent comment. I think this is definitely the right direction to continue the investigation, however. It's curious if 5 is exactly the threshold where different behavior kicks in, perhaps triggering some sort of recycling logic in the |
Explanation
Essential Checklist
For UI-specific PRs only
mobile_flick.mp4
-Tablet
Tab_flick.mp4
If your PR includes UI-related changes, then: