Skip to content
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

Update favIconHandler's context on RssItemViewHolder bind #1405

Closed
wants to merge 2 commits into from

Conversation

DoHe
Copy link

@DoHe DoHe commented Mar 30, 2024

On changing the theme the context the favIconHandler is holding seems to become invalid. You can see the resulting issue here:

Screen_recording_20240329_120130.mp4

This was recorded on a virtual android device (pixel 4 API 33) and coud also be reproduced on my physical pixel 7a running android 14.
I'm sadly not much of an android dev, so I'm not sure if this is the best solution but it does fix the issue. What I could tell is that the request glide (which also holds a reference to this context) is making is neither successful nor fails so it just leaves the placeholder in place (I added RequestListeners and neither onLoadFailed nor onResourceReady ever gets called in these cases).

I also wonder if this issue has a similar root cause, but I haven't been able to reliably reproduce it.

@DoHe DoHe force-pushed the dh-fix-favicons branch from 761c2e4 to 7041044 Compare March 30, 2024 10:58
Signed-off-by: Dominik Henter <[email protected]>
@DoHe DoHe force-pushed the dh-fix-favicons branch from 7041044 to cd55d71 Compare March 30, 2024 10:59
@David-Development
Copy link
Member

Great catch! Thank you for opening this MR and for including a video that displays the issue.

I see two issues:

  • everytime bind is called the context will be changed and Glide will be recreated (so basically for every rss item in the list) (So either we need to move the setContext somewhere else or check inside the setContext if it's the same context that it was before - if so, we don't need to change it.)
  • I think that it makes sense to have a global Glide Provider that every class that needs Glide can request the current Glide instance at runtime. Which - when the theme is changed will be switched. Even the RssItemViewHolder has a reference to a Glide instance which is not updated when the theme changes. Thus I believe there are quite a few of these bugs spread across the code base..

I believe in theory the onConfigurationChanged function should be called when the Theme is changed.

@Unpublished What are your thoughts on this?

Signed-off-by: Dominik Henter <[email protected]>
@DoHe
Copy link
Author

DoHe commented Apr 1, 2024

@David-Development thanks for the feedback. I added the equality check for what it's worth but I'd also agree that a more global approach might make more sense. I sadly don't think I'm familiar enough with the code base to really judge that or know the best way to implement it.

@Unpublished
Copy link
Collaborator

Unpublished commented Apr 1, 2024

An implementation is coming up on noStaticFavIconHandler branch.

I think the main culprit is having a static FavIconHandler which leaks context.

Using a per fragment instance approach for Glide will still benefit from Glide's lifecycle awareness, pausing requests of not visible fragments. Also fragments context should be aware of configuration changes like theme AFAIK.

You can already try above branch, I'll polish a bit and open a PR soon.

@DoHe
Copy link
Author

DoHe commented Apr 2, 2024

Closing this in favor of #1410 which fixes this problem more holistically.

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.

3 participants