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

Improve image caching #1105

Merged
merged 4 commits into from
Feb 12, 2024

Conversation

micahmo
Copy link
Member

@micahmo micahmo commented Jan 31, 2024

Pull Request Description

This PR is an attempt to improve image caching. This has been a long-running discussion (see a few of the linked PRs below).

The most recent solution was to cache images with a timeout. This means that scrolling through the feed is nice, but navigating into a post for a while and then navigating back can still cause all the images to reload. In fact, this behavior recently prompted a new issue, #1101.

Since it seems that there's no perfect solution which will both keep images cached and improve performance while also maintaining a low memory profile, I've added a new advanced setting which lets the user pick their preferred behavior.

The new default behavior will be like prior to #745, meaning that images may reload as you scroll up and down the feed, but should not reload when navigating back from a post (clearMemoryCacheWhenDispose is true).

The new optional behavior is to have more aggressive caching than even in #745, more like after #463, where clearMemoryCacheWhenDispose is false and cacheMaxAge is not set.

Important

The important thing is that neither mode should cause the flashing described in #1101, because that was purely due to the cacheMaxAge. It was forcibly invalidating cache, even before the widget was disposed.

Issue Being Fixed

Issue Number: #1101

Screenshots / Recordings

This demo just shows the new setting. You need to run the app for a while to experience the difference in behavior.

qemu-system-x86_64_7XJ47ndbLU.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member Author

micahmo commented Feb 7, 2024

Since this one is a bit difficult to test, I think the best feedback we can get is from regular usage in the nightlies. Therefore, I'm opening this up for review.

@micahmo micahmo marked this pull request as ready for review February 7, 2024 17:34
Copy link
Member

@hjiangsu hjiangsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a look at the changes and it LGTM (with a minor comment)!

One thought - since this seems to only be happening on Android, should we limit the setting to Android only for now?

lib/settings/pages/general_settings_page.dart Show resolved Hide resolved
@micahmo
Copy link
Member Author

micahmo commented Feb 12, 2024

since this seems to only be happening on Android, should we limit the setting to Android only for now?

Sure, I will add that restriction. It would be easy enough to allow the setting on iOS later if needed. The default should also be fine for memory utilization on iOS since it defaults to clearing cache on dispose.

@micahmo
Copy link
Member Author

micahmo commented Feb 12, 2024

Should be all set now!

@hjiangsu hjiangsu merged commit f8d76f4 into thunder-app:develop Feb 12, 2024
1 check passed
@micahmo micahmo deleted the feature/improve-image-caching branch February 12, 2024 20:17
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.

2 participants