fix(mobile): Timeline grid not resizing properly on device rotation#24573
fix(mobile): Timeline grid not resizing properly on device rotation#24573timonrieger wants to merge 12 commits intoimmich-app:mainfrom
Conversation
|
Label error. Requires exactly 1 of: changelog:.*. Found: 📱mobile. A maintainer will add the required label. |
|
Perhaps you can gain some ideas from the old timeline; this issue wasn’t present there. |
Do you now until which version the old timeline was in-place? |
@timonrieger The latest version should still include the old timeline codes as you can disable the new timeline via Settings -> Advanced. |
true that, thanks for the bump 😄 |
| startOffset += headerExtent + spacing + (tileHeight * numberOfRows) + spacing * (numberOfRows - 1); | ||
| } else { | ||
| startOffset += headerExtent; | ||
| } |
There was a problem hiding this comment.
self-review: When numberOfRows == 0, there are no asset rows, so we only add the header extent and skip spacing/tile heights that don't apply.
| DriftMemoryPage.setMemory(ref, memories[index]); | ||
| } | ||
| context.pushRoute(DriftMemoryRoute(memories: memories, memoryIndex: index)); | ||
| child: ListView.separated( |
There was a problem hiding this comment.
self-review: CarouselView triggered a framework assertion during rotation ('haveDimensions == (_lastMetrics != null)')
There was a problem hiding this comment.
A ListView is different from CarouselView. We should be fixing the issue and not replace it with a different widget
Implement continuous row anchor tracking and automatic scroll adjustment when segments change, matching web timeline behavior. This ensures stable scroll position during orientation changes by using (rowIndex, deltaPx) anchors instead of unreliable pixel offsets. - Add _onScroll() listener to continuously update row anchor - Auto-adjust scroll when segments regenerate (width/columnCount change) - Remove pixel-based fallback restoration logic
6ca4969 to
b1ea976
Compare
|
a merge conflict introduced by #24999 must be resolved, don't know if that addition was intentional there |
|
@shenlong-tanwen Can you help with reviewing this PR? Thank you |
| DriftMemoryPage.setMemory(ref, memories[index]); | ||
| } | ||
| context.pushRoute(DriftMemoryRoute(memories: memories, memoryIndex: index)); | ||
| child: ListView.separated( |
There was a problem hiding this comment.
A ListView is different from CarouselView. We should be fixing the issue and not replace it with a different widget
| final segmentStartOffset = startOffset; | ||
| startOffset += headerExtent + (tileHeight * numberOfRows) + spacing * (numberOfRows - 1); | ||
| if (numberOfRows > 0) { | ||
| startOffset += headerExtent + spacing + (tileHeight * numberOfRows) + spacing * (numberOfRows - 1); |
There was a problem hiding this comment.
Was the duplicate spacing intentional?
| startOffset += headerExtent + spacing + (tileHeight * numberOfRows) + spacing * (numberOfRows - 1); | |
| startOffset += headerExtent + (tileHeight * numberOfRows) + spacing * (numberOfRows - 1); |
| child: Consumer( | ||
| builder: (context, ref, _) { | ||
| final columnCount = ref.watch(settingsProvider.select((s) => s.get(Setting.tilesPerRow))); | ||
| final desired = TimelineArgs( | ||
| maxWidth: constraints.maxWidth, | ||
| maxHeight: constraints.maxHeight, | ||
| columnCount: columnCount, | ||
| showStorageIndicator: showStorageIndicator, | ||
| withStack: withStack, | ||
| groupBy: groupBy, | ||
| ); | ||
| final current = ref.watch(_runtimeTimelineArgsProvider); | ||
|
|
||
| if (current != desired) { | ||
| final rowAnchor = ref.read(_timelineAnchorRowProvider); | ||
| WidgetsBinding.instance.addPostFrameCallback((_) { | ||
| final latest = ref.read(_runtimeTimelineArgsProvider); | ||
| if (latest != desired) { | ||
| ref.read(_runtimeTimelineArgsProvider.notifier).state = desired; | ||
| } | ||
| if (rowAnchor != null) { | ||
| ref.read(_timelinePendingRestoreRowAnchorProvider.notifier).state = rowAnchor; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Why'd we need all these complex rebuild flows? All we need to do is to rebuild the widget on orientation state while retaining the anchor. It should actually be possible with just a single provider that stores the required anchor before rebuild and restores it after the change. Can you look into simplifying all the provider logic? Or can you explain why we need each of the providers?
|
closing this in favor of #25088 as discussed |
Tries to fix #24446
This is not trivial, unfortunately. Pixel-based preservation fails because tile dimensions change on rotation (portrait vs landscape have different tile heights), causing the same pixel offset to map to different visual content.
EDIT: i got it working, thanks to @kao-byte in #24573 (comment) 🎉
Description
The timeline now correctly resizes the layout when rotating the device. The scroll position is being restored when rotating the device, similar to how the old timeline handles scroll position.
Most of the implementation got bridged from the old timeline, where this issue was a not present.
Fixes #24446
Fixes #22540
How Has This Been Tested?
Screenshots
demo-24573.mov
Checklist:
src/services/uses repositories implementations for database calls, filesystem operations, etc.src/repositories/is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/)Please describe to which degree, if any, an LLM was used in creating this pull request.
LLM assistance was used to bridge the anchor logic from the old timeline to the new one.