Media view / Quick Look tweaks and improvements - #6072
Draft
stefanceriu wants to merge 12 commits into
Draft
Conversation
A megolm event whose failure cause is unknown is usually waiting on a key that is still on its way from backup. Every other cause is final. `mayStillDecrypt` exposes that distinction. Nothing reads it yet; the media viewer picks it up in a later commit, to decide whether an event sitting between two media items might yet become media itself or can be stepped over.
The badge drawn over video thumbnails was inline in VideoRoomTimelineView. The media viewer needs the same one over the posters it shows while a video downloads, so pull it out as VideoPlayBadge. Same icon, padding and background, so no visual change.
Tapping a media item showed the room screen's loading overlay unconditionally until the viewer was ready. Building the media timeline takes around 100ms from the event cache, so on almost every tap the spinner appeared and vanished as a blink. It is now shown from a task that waits 300ms first and is cancelled as soon as the tap has been processed, so only a tap that has to fetch the event shows it.
Fetching the media either side of the one on show is what lets a swipe land on the media rather than an empty page, but it spends data on files the user may never reach. Put it behind a toggle, defaulting to on. Only the settings screen reads it so far; the viewer picks it up in a later commit. Preview snapshots re-recorded for the new row.
The data source reports a fixed block of phantom padding either side of the loaded items so indices stay stable while the timeline paginates underneath. That padding was also what the user swiped into, and it never went away, so reaching either end of the timeline meant paging onto placeholder after placeholder. A side's padding now collapses to zero once that side reaches the end of the timeline, making the last real item QuickLook's own content edge so its scroll view rubber-bands there instead. The reported count changes only at those two transitions, so the stable-index invariant the padding exists for still holds throughout ordinary pagination. The collapse waits for a pagination state other than `.initial`, which reports `endReached` both ways as a "don't paginate yet" sentinel and is otherwise indistinguishable from a genuinely exhausted small room.
Opening the viewer on an upload in flight and waiting for it to send rebuilt the viewer around the "new" item: sending replaces the local echo's transaction ID with an event ID, and the data source matches items by that ID alone. The timeline recycles the item's unique ID across the swap, so an item whose unique ID matches and whose transaction ID has become an event ID is now recognised as the same media. It keeps its page, its file (already in the local media cache) and its download state, and its ID is updated in place, which is the only thing that ever changes it.
An update whose items are not a contiguous run of the new list was ignored wholesale, to avoid wiping out the initial item while the timeline was still loading it from the store. The media timeline's de-duplication and backfill churn triggers the same case constantly, so the viewer froze on the handful of items it had while the timeline went on to the room's start. Such an update is now taken whenever the two lists share an item. The current item, or any shared item, is held at its index and the padding either side absorbs the difference, so the page on display does not move while the rest of the list is replaced underneath. An update with nothing in common is still ignored, so the initial item is still safe.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #6072 +/- ##
===========================================
- Coverage 74.91% 73.70% -1.22%
===========================================
Files 829 830 +1
Lines 58734 60419 +1685
===========================================
+ Hits 44003 44531 +528
- Misses 14731 15888 +1157
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
QuickLook builds a page from whatever previewItemURL its item answers with at build time and never asks again, so an item whose file has not arrived yet is built black and stays that way. Two mechanisms close that gap. Neighbouring media is fetched ahead of the swipe, and any item still without its file answers with a poster drawn from the timeline's cached thumbnail, so its page builds from that and sharpens in place when the media lands. The initial item starts loading with the view model, so cached media opens straight to the file. Two decisions worth checking. Preloading is thumbnails-only, so a speculative neighbour never pulls a whole file, except for an image sent without a thumbnail (MSC4409), which is fetched whole because asking for a size would route it to the thumbnail endpoint and that cannot serve encrypted media. And preloading is skipped entirely when a content scanner is configured, since a neighbour would have to be scanned as the current item is. The reach, the file size limit and the placeholder grace period are documented at their definitions.
QuickLook re-reads the item count only on reloadData, and the index it reports comes from a callback it partly ignores, so the viewer has to track the dataset itself. Three things follow. The current item's index is re-derived from the data source on every count change rather than shifted by the change in the first index, which was opening the viewer on an item roughly N older than the one tapped, or on the paginating placeholder. Landing on a "Loading more" page clamps the viewer to it, so the swipe stops on one placeholder instead of running through a hundred identical ones. And the phantom padding is treated as a budget a long browse spends, restored under a reload before a spent side's edge hardens into a false end of the timeline. Every one of those transitions is deferred out of QuickLook's index callback: an index written from inside it is dropped, which used to toggle clamp and release forever, and one written in the same turn as a reload wedges the page queue. Nothing is reloaded while the pages are moving. The timeline ends bounce natively now, so the hard block on the swipe and the "no more media to show" toast behind it both go away.
Stepping off a "Loading more" page onto the items that arrived beyond it skipped around twenty items that filled in a moment later. A backfill lands older items first, leaving a timeline gap, or messages that have not decrypted yet, between them and the edge the user came from, and those messages usually turn out to be media once their key arrives from backup. The data source now reads the shape of the timeline around the media: what sits between each item and the next newer one, and whether anything unresolved sits older than the oldest media. The controller waits on the placeholder while something does, and the view model holds the backwards pagination, which would otherwise request keys for older pages ahead of the nearest ones. Both waits are bounded and both bounds matter. An event counts as maybe-media for five seconds, comfortably longer than a key takes to arrive. The controller's hold runs from when the user landed on the placeholder rather than restarting per event, which in a room full of undecryptable messages kept them there for 13 seconds while media that had already decrypted sat unreachable behind the clamp.
QuickLook caches what each page was built from and never asks again, so a neighbour whose file or poster landed after the build stays blank until the user reaches it. There is no per-page refresh: only reloadData for everything, and refreshCurrentPreviewItem for the page on display. So the controller models which items were built without a file, since a blank offscreen page is not in the view hierarchy and cannot be found by inspecting subviews. When content arrives while the user is resting, a debounced reloadData rebuilds the built pages, so a page heals before it is reached rather than after. A reshuffle invalidates the built pages the same way, and the data source flags it. Two constraints on that reload. It never runs on an active touch, which wedges QuickLook into accepting every pan without moving a page. And its flash is hidden behind a snapshot of the page scroll view, dropped once the rebuilt page has content again; the handover between overlapping covers is documented at `reloadCoverTask`. Pages the user does land on blank are cleared with reloadData rather than refreshCurrentPreviewItem, which reliably failed to clear QuickLook's "content unavailable" placeholder on device.
QuickLook swaps its navigation item on every refresh that re-installs its list button, and the sender and timestamp header was only installed from viewWillLayoutSubviews, so the item's filename showed until the next layout pass. The header is now reapplied from the same KVO and timer that swap the button back. A video's caption sits over the scrubber, and the tap to dismiss it went to QuickLook, hiding all the chrome and pausing playback with it. A viewer-level recognizer that every other single-tap recognizer defers to claims the touch, but only while a caption is on show and never over the navigation bar, the caption's own links or visible player controls. Any page change brings the caption back. The header reads "Loading..." over a thumbnail placeholder, and appends the attachment's place in its gallery. A video's poster gets the timeline's play badge as a vector overlay, crisp whatever the thumbnail's resolution rather than baked into the poster. QuickLook's text selection and caret take the view's tint, and the window's primary-text tint reads as a faint grey on dark media, so the composer's is used instead.
stefanceriu
force-pushed
the
stefan/fableQuickLookResplit
branch
from
September 2, 2026 14:19
f8903cf to
8960da1
Compare
PR Checks
|
pixlwave
reviewed
Sep 4, 2026
Member
There was a problem hiding this comment.
Ok after an initial look (I haven't tested anything yet) I think we should split this PR into 3 (in this order):
- Bug fixes
- Proactive loading/padding fixes (the big one, should be clearly split between loading and then all the padding adjustments).
- Thumbnail support.
This will be easier to review and test the changes in isolation of each other.
| context.send(viewAction: .optimizeMediaUploadsChanged) | ||
| } | ||
|
|
||
| ListRow(label: .plain(title: UntranslatedL10n.screenAdvancedSettingsPreloadMediaTitle, |
Member
There was a problem hiding this comment.
I would think this should go in Developer Options for some time to test/bug-hunt in Nightly first right?
| private var backwardPadding: Int | ||
| private var forwardPadding: Int | ||
| /// Whether a real (post-load) pagination state has been seen. `.initial` is `endReached` on | ||
| /// both sides as a "don't paginate yet" sentinel, indistinguishable by value from a genuinely |
Member
There was a problem hiding this comment.
Hmmm, I wonder if we should separately fix this with an initial case on PaginationState instead.
| /// both sides as a "don't paginate yet" sentinel, indistinguishable by value from a genuinely | ||
| /// exhausted small room, so we only collapse the phantom padding (below) once a real state has | ||
| /// arrived; until then the padding stays, keeping room to paginate. | ||
| private var hasReceivedRealPaginationState: Bool |
Member
There was a problem hiding this comment.
Noting to myself that I'm surprised this doesn't need to do something to make sure swiping goes to the correct index.
| fileprivate(set) var id: MediaPreviewItemID | ||
|
|
||
| /// Whether this is the local echo that `sentItem` is the remote echo of: sending swaps the | ||
| /// transaction ID for an event ID (so `id` changes) but the timeline recycles the unique ID. |
Member
There was a problem hiding this comment.
but the timeline recycles the unique ID
Noting that I'm curious why we don't use the uniqueID alone as the MediaPreviewItemID instead… There's probably a reason but it certainly stands out as being wrong to me now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replacement for #6062 in which the per commit split was off.