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

[BUG] Images flash white when post is interacted with #1352

Open
1 task done
fizwidget opened this issue Oct 29, 2024 · 2 comments
Open
1 task done

[BUG] Images flash white when post is interacted with #1352

fizwidget opened this issue Oct 29, 2024 · 2 comments
Labels
Bug Something isn't working Needs Triage
Milestone

Comments

@fizwidget
Copy link

Is there an existing issue for this?

  • I have searched the issues (both open and closed)

Current Behavior

When I favourite a post that contains an image, the image briefly flashes white then reappears. This is especially annoying for GIFs because it also resets the play position to the beginning. For static images it feels a bit glitchy/buggy, but isn’t a big problem.

Expected Behavior

Favouriting a post should not make images flash or reset the play position of GIFs.

Steps To Reproduce

  1. Tap the star/favourite button on a post that contains an image.
  2. Observe how it flashes white.

Environment

  • Device: iPhone XS Max
  • OS: iOS 18.0.1
  • Version: 2024.10
  • Build: 6834

Anything else?

No response

@fizwidget fizwidget added Bug Something isn't working Needs Triage labels Oct 29, 2024
@whattherestimefor whattherestimefor modified the milestones: 2024.11, To Do Nov 20, 2024
@tvrrp
Copy link
Contributor

tvrrp commented Jan 17, 2025

@whattherestimefor I would like to take on this issue. But first, I want to discuss a few things.

This bug occurs because, after applying a new snapshot to UITableView, DiffableDataSource reloads the cell instead of reconfiguring it. We could potentially use the new reloadItems API, but this would require significant changes across multiple screens. As an external contributor, I don’t want to introduce a large number of changes, as it could potentially break some functionality.

However, I’m considering a different approach. Inside MediaView in prepareForReuse(), we clear everything — removing images, stopping videos, removing AVPlayer, etc. It doesn’t make sense to do this preemptively. Instead, we could handle it in setup(configuration:) since this method is called for every cell before it’s displayed. Additionally, as Configuration is Hashable, we could compare the previous model with the new one and update the cell content only if the model has changed.

I also dislike the idea of removing views from the view hierarchy. We could simply toggle their isHidden property. A better solution would be to use different views for different types of media content, but that would also require substantial changes.

What do you think about the proposed solution? Or are there other methods I might have overlooked or am unaware of?

@whattherestimefor
Copy link
Contributor

@tvrrp Thanks so much. I think the simpler approach is worth a try, but let me provide some background/additional info:

The current state of things is kind of in-between. The app was originally developed with everything in CoreData and the cells updating their own state whenever they observed changes. CoreData has been mostly removed (too much overhead and we really didn't need to save most of that info across app launches at all), but the system of mutable data objects updating the cells remained. That system was (is) very hard to read and very brittle when attempting to make new changes.

What you're seeing now is the first effort at moving towards the cells being much simpler, displaying immutable data that gets replaced when changes happen. That transition is definitely introducing some bugs, like this one.

Additionally, I'm currently overhauling the notifications view. We will be using SwiftUI at least for the individual cells, and might end up using SwiftUI List rather than UITableView overall. The data model is also changing (or at least starting to), with the Mastodon objects being cached based on their id instead of wrapped up together with all their related objects and view state (like whether a particular content warning should be blurred out or not).

Which is all to say, I hope that almost none of the code involved in this bug will still be around by the end of this year. However, it would be great to fix it in the meantime, and your plan sounds reasonable to me if you are still willing to take it on.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Needs Triage
Projects
None yet
Development

No branches or pull requests

3 participants