Skip to content

Conversation

@itsadityaaaaa
Copy link
Collaborator

@itsadityaaaaa itsadityaaaaa commented Jan 2, 2026

Explanation

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

  • Mobile
mobile_flick.mp4

-Tablet

Tab_flick.mp4
  • BindableAdapterTest Result
Screenshot from 2026-01-02 22-09-49

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@itsadityaaaaa itsadityaaaaa requested a review from a team as a code owner January 2, 2026 16:41
@itsadityaaaaa itsadityaaaaa requested a review from manas-yu January 2, 2026 16:41
@github-actions
Copy link

github-actions bot commented Jan 2, 2026

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.

@itsadityaaaaa itsadityaaaaa marked this pull request as draft January 3, 2026 06:27
@oppiabot
Copy link

oppiabot bot commented Jan 10, 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.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 10, 2026
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label Jan 16, 2026
@adhiamboperes
Copy link
Contributor

@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?

@BenHenning
Copy link
Member

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 Profile proto--we only generate colors upon profile creation. At a high-level this suggests a few possibilities (though I'm sure there are many more) for why the bug may be happening:

  • There's some sort of race condition between the list updating, the recycler view partially re-rendering, and the page changing.
  • There's some sort of corruption or mismanagement happening for the list of profiles caused by the late update during screen transition.
  • Recycler view is actually binding the wrong view data (i.e. an issue in the recycler view pipeline or how we bind it rather than the actual data we're binding to it).

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:

  1. Either step debug or use 'print/logcat debugging' to print out the exact list of profile data being bound at each step. See what the list is before selecting a profile, then compare that to the list after selecting (that ends up showing up wrong). What are the differences? This will narrow down if it's a data representation, binding, or rendering problem.
  2. Once we understand which category of problem it is, we need to ask if the data is correct at that stage. If it is, we have to work to do in how we're managing it (i.e. rendering or binding). If it isn't, we need to go backwards and figure out how the data came to be that way (data management). This involves stepping or more logcat logs earlier in the dataflow for the profile list to try and figure out at which point is the wrong data being created and propagated.
  3. Follow up with your findings. If you are unable to determine the why then document as much of the what as you can (i.e. what steps you took and the observed outcomes, and what seems wrong or right about them).

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).

@itsadityaaaaa
Copy link
Collaborator Author

itsadityaaaaa commented Jan 23, 2026

@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?

@itsadityaaaaa itsadityaaaaa marked this pull request as ready for review January 23, 2026 18:38
@github-actions
Copy link

Coverage Report

Results

Number of files assessed: 1
Overall Coverage: 0.00%
Coverage Analysis: PASS

Exempted coverage

Files exempted from coverage
File Exemption Reason
BindableAdapter.ktapp/src/main/java/org/oppia/android/app/recyclerview/BindableAdapter.kt
This file is incompatible with code coverage tooling; skipping coverage check.

Refer test_file_exemptions.textproto for the comprehensive list of file exemptions and their required coverage percentages.

To learn more, visit the Oppia Android Code Coverage wiki page

@BenHenning
Copy link
Member

@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?

@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.

@itsadityaaaaa itsadityaaaaa marked this pull request as draft January 29, 2026 16:29
@itsadityaaaaa
Copy link
Collaborator Author

@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.

@oppiabot oppiabot bot assigned BenHenning and unassigned itsadityaaaaa Jan 29, 2026
@oppiabot
Copy link

oppiabot bot commented Jan 29, 2026

Unassigning @itsadityaaaaa since a re-review was requested. @itsadityaaaaa, please make sure you have addressed all review comments. Thanks!

@BenHenning
Copy link
Member

@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.

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 BindableAdapter works today. What's unclear is why the full rebind causes the specific issue with the colors changing--that's what needs to be figured out. It may well be the case that diff utils, by avoiding a full rebind, are just sidestepping the issue rather than fixing its real root cause (which we won't be able to know without understanding why exactly the full rebind is causing problems).

@itsadityaaaaa
Copy link
Collaborator Author

@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?

@itsadityaaaaa
Copy link
Collaborator Author

@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.

@BenHenning
Copy link
Member

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 RecyclerView? That hypothesis could be tested by temporarily disabling recycling so that the RecyclerView just acts like a ListView to see if it changes the behavior at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: Profile colors and positions change when selecting a profile in the Profile screen

3 participants