Repository navigation
Refactor grid notes - #30
Merged
Merged
Conversation
Markel15
commented
Oct 19, 2026
Markel15
commented
Owner
- The ideas and checklists now are showed in a grid-style list
- Potentially in the future I will combine those 2 lists
- This refactor requiered to change the simple lazy list which wrapped all the layout to be a LazyVerticalStaggeredGrid
- I fixed a lot of bugs (especially visual glitches) that I caught refactoring (related to the refactorization)
…d checklists. Purely additive: no UI or behavior changes yet, SectionedFlowView still renders the carousels and the existing reorder methods stay in place. The key here is that I can't simply change the actual carousel with a LazyVerticalStaggeredGrid because that composable doesn't know their height. For that I create an item to put everything with the layout I have in mind. These items also help with the reordering because add a scope within the items would be reordered. - New sealed interface FlowItem: every rendered item (section headers, reminder banner, task rows, checklist cards and idea cards) is one variant, so the index reported by `reorderable` maps 1:1 onto items[index]. That removes the hardcoded `headerOffset` arithmetic SectionedFlowView needs today. - FlowUiState.Success.items: flat, render-ready list declared in the class body so `copy()` always recomputes it. Headers are emitted only for non-empty sections. - FlowItem.stableKey(): unique key per variant; folds the per-task delete version into the task key so an undo recreates the composable with a fresh SwipeToDismissBoxState instead of restoring the stale EndToStart saved state. - FlowUiState.Success.withItems(): projects a reordered item list back onto the three typed lists. - FlowItemTest: 12 unit tests covering item construction, key uniqueness and the withItems round-trip.
Consolidate screen sections into a single LazyVerticalStaggeredGrid inside SectionedFlowView to allow unified scrolling, cross-section reordering, and responsive multi-column card layouts. Key Changes: * SectionedFlowView: - Replaced individual list containers with a single LazyVerticalStaggeredGrid. - Added full-width line spans for Tasks/Headers and single-lane spans for Cards. - Localized draggable item logic using `ReorderableCard` and `TaskRowContent`. * Stable Keys & Task Deletions: - Abstracted key generation via `stableKey(taskDeleteVersions)`. - For Tasks, combines `task.id` with its monotonic `taskDeleteVersions` counter to ensure Compose re-instantiates SwipeToDismiss state upon undo. * Scroll Modifiers: - Added a `LazyStaggeredGridState` overload for `rememberFabVisibilityState`. - Refactored internal scroll tracking to reuse a single `Flow<Pair<Int, Int>>` pipeline across both LazyList and StaggeredGrid states.
Also fixed a bug introduced in the latest commit that caused the reminder permission banner look strange.
Race: the expiry job deleted the task from Room and immediately removed it from _pendingUndoTasks. Room delivers invalidations asynchronously, so the combine often recomputed with the stale pre-deletion task list while the filter no longer excluded the id — the task passed through and flashed on screen for a few frames until the fresh list arrived. Fix: on expiry the id is handed over to a new _committedDeleteIds set (before leaving _pendingUndoTasks, so it is never unfiltered in between) which keeps it excluded from the UI. The combine itself drops committed ids once its DB snapshot no longer contains them — the cleanup runs exactly on the emission that proves the deletion, closing the race for every interleaving. Undo FAB semantics are unchanged: it only tracks the still-undoable map.
The banner row inherited the shared animateItemModifier in the staggered grid refactor, but the pre-grid LazyColumn rendered it as a plain item because the banner owns its enter/exit animation (AnimatedVisibility expand/shrink + fade). The layered animations collide when the tasks section disappears (last task deleted/completed): the item leaves the list while its internal AnimatedVisibility starts collapsing, and animateItem's fadeOut keeps a ghost of the banner — fading and shrinking at the same time — floating over the content that already slides up with the placement spring. The result is the fast upward shift followed by a visible flicker. Without animateItem the banner is removed instantly in the same layout pass, exactly like the old LazyColumn, and its AnimatedVisibility stays the single animation authority while it remains in the list.
Symptom
-------
After the switch from LazyColumn to LazyVerticalStaggeredGrid, completing or
deleting a task made the rows below it move up, but any row that was outside the
viewport and got pulled into view during that animation did not travel with the
rest of the list: it appeared a few frames late, visibly detached from the row
above and then slid into place. With the reminder banner on
screen the artifact was much easier to spot, because the banner reveals several rows at once; that made it look like a second, banner-specific bug, but the mechanism is the same one described here.
Root cause
----------
LazyColumn and LazyVerticalStaggeredGrid do not report the same "end of content" to the item animator (foundation 1.10.x / 1.11.x):
LazyListMeasure.kt
itemAnimator.onMeasured(layoutMaxOffset = currentMainAxisOffset, ...)
LazyStaggeredGridMeasure.kt:933
itemAnimator.onMeasured(
layoutMaxOffset = currentItemOffsets.max() + contentPadding, ...)
`contentPadding` there is the *sum* of the before/after main-axis paddings. This screen used `top = 8.dp, bottom = 96.dp` (room for the FAB), so the staggered grid reported the content end 104 dp further down than it really is.
That offset only matters for rows that are measured for the first time in a given pass and therefore have no previous ItemInfo to animate from. They take the `movingInFromEndBound` branch of LazyLayoutItemAnimator.onMeasured, which initializes their animation at
layoutMaxOffset + accumulatedOffset - mainAxisSizeWithSpacings
i.e. 104 dp (312 px at 3x) *below* the position the row occupied before the removal. The row is born below its real slot and has to travel that extra distance with the placement spring, so it enters the viewport later than the rows that animate from their real previous positions, and visibly disconnected from them.
Fix
---
Do not put main-axis padding on the grid:
* the 8 dp top gap is applied to the first row instead (it is always a header), so it is part of the measured content.
* the 96 dp bottom room for the FAB becomes a real item,
FlowItem.BottomSpacer (see its KDoc for the reasoning).
With both of them inside the content, the animator's end bound equals the real content end and a row entering from below starts exactly at the position it had before the removal, which is the behavior the screen had with LazyColumn.
Symptom
-------
Deleting the task that owns the reminder also hides the "exact alarms" permission
banner. When that happened, the rows below the viewport did not move with the
list: they stayed hidden during the animation and then appeared, already parked
in their final position. The rows that were already composed did move smoothly,
which made the bug look like "only the last two rows pop in late".
Root cause
----------
The banner was a FlowItem, i.e. one more row of the LazyVerticalStaggeredGrid,
so its height was part of the content geometry. A row that has never been
composed has no previous position to animate from: LazyLayoutItemAnimator
synthesizes its start offset from the *end of the measured content* of that pass
(the same bound the previous commit took out of contentPadding, see
onMeasured -> movingInFromEndBound). Two consequences:
* with AnimatedVisibility(shrinkVertically) the banner changed its height on
every frame of the exit, so the content end was a moving target for ~300 ms,
and the collapse lands in the same frame as the task removal (uiState and
showReminderBanner are two StateFlows the ViewModel updates back to back).
Rows entering from below were initialized from the wrong bound: the first
one was born glued to the row above (gap 61 px instead of 137 px) and then
slid into place.
* collapsing the height in a single frame (fading it out and taking it out of
the layout at once) fixes the glue but not the pop: the rows are simply
re-placed at their final offset, which is already inside the viewport, so
they appear planted mid-screen with no motion.
There is no in-grid configuration that solves both at once: any change in the
content geometry either gives the entering rows a wrong synthetic start offset,
or gives them no animation at all.
Fix
---
Take the banner out of the lazy container. ReminderPermissionBanner is rendered
in a Column above the LazyVerticalStaggeredGrid, so the height it animates is the
height of the grid's *viewport* instead of the height of an item:
* the grid's content offsets never change while the banner collapses, so the
grid translates rigidly as a single layer while the parent shrinks the space
above it, and every row keeps its relative position;
* the rows it reveals come in through the bottom edge and travel with the rest
of the list, exactly like during a scroll.
FlowItem.ReminderBanner is removed, together with its entry in FlowUiState.items
and the FlowItemTest expectations. The banner keeps its expand/shrink + fade
animation, which is safe again outside the lazy layout.
Behavior note
-------------
The banner no longer scrolls away with the list: it stays pinned above it while
the permission is missing. That is the usual pattern for a permission banner, and
it is what makes the collapse free of item animations.
Alternatives considered
-----------------------
1. Keeping the banner as a plain list item and letting the list animate it (no
AnimatedVisibility at all): the approach that fixed issue #24. Measured in the
same harness it does not work here: the rows that enter from below are still
born at a synthetic offset (the gap above task12 is 125 px instead of 50 px,
and the first checklist is initialized at 1075 px instead of 788 px), and they
need ~350 ms to converge, so the "detached tail" artifact stays.
The difference from #24: there the item that traveled wrongly had been
composed before, so it had a previous offset and animateItem() only had to use
it. Here the rows that misbehave are precisely the ones that were never
composed, and the only thing the animator knows about them is the end of the
measured content - which the banner moves when it leaves the list.
2. Fading the banner and collapsing its height in a single frame: no glue, but
the entering rows are re-placed straight at their final offset and appear
mid-screen without any motion (788 px in the harness, exactly where they end).
3. Keeping the height animation (the state before this commit): rows are born
glued to the row above (gap 61 px instead of 137 px) and slide into place.
Only taking the banner out of the lazy content removes the cause, because then
the layout it changes is the viewport's, not the content's.
Symptom
-------
Long-pressing a checklist / idea card and dragging it over a row of another
section (the first idea while holding the last checklist card, or any task row)
made the held card jump to a different spot and then follow the finger from
there, staying displaced until the drop. Reordering inside a single section was
fine, and the case did not exist before the grid refactor.
Root cause
----------
onItemReorder vetoes cross-section moves:
val section = from.section ?: return
if (section != to.section) return
but reorderable cannot be told "no": onMove returns Unit and the library
commits to the swap on the very next lines (3.0.0, ReorderableLazyCollection.kt):
:641 oldDraggingItemIndex = draggingItem.index
:643 onMove(from, to) -> our veto, nothing moves
:645 predictedDraggingItemOffset = ... -> where the row *would* land
:316 draggingItemOffset = draggedDelta + (initialOffset - predictedOffset)
so the held row is translated by the delta of a swap that never happened. That is
the jump. Nothing corrects it afterwards: the library then waits, at :651, for a
layout change a vetoed move cannot produce; the timeout is swallowed by the
catch (CancellationException) at :660, so the resets at :657-658 never run and
the phantom offset survives until onDragStop(). The move mutex stays held for
that second, onDrag() short-circuits on tryLock(), and while the row keeps
accumulating drag deltas no further crossing or auto-scroll is processed.
Section headers are never involved because they are not ReorderableItems, so
they are absent from the library's reorderableKeys. Leaving a section always
means overlapping a foreign *card*, which is precisely the row the guard rejects.
This was unreachable before the refactor: ReorderableCarousel built its own
rememberReorderableLazyListState per section, so one carousel never saw the
rows of another. A single grid for every section puts them all in the same
reorderableKeys set, and that is what makes the veto reachable.
Fix
---
Veto before the library picks its target, not inside onMove. The only lever
the library offers is ReorderableItem(enabled = …): a disabled key is dropped
from reorderableKeys (ReorderableLazyCollection.kt:832-837) so it can never
be chosen as a swap target, while the row stays draggable — enabled gates being
*swapped with*, not being *dragged*.
* FlowItem.kt: isReorderTargetFor(draggedSection) holds the rule "a row is a
swap target only while the dragged row comes from its own section", so the
view's gate and the ViewModel's guard cannot drift apart.
* SectionedFlowView.kt: draggedSection state, published by each handle's
onDragStarted / onDragStopped and consumed as enabled by every row. It
is also cleared from reorderState.isAnyItemDragging, so an onDragStarted
that never turned into a real drag cannot leave the other sections locked.
* FlowViewModel.kt: the guard stays — it is what protects the data — but its
KDoc now records that it must be unreachable, and that any new rejection
needs a matching gate on the view side.
Behavior note
-------------
Dragging a card over a foreign row now does nothing: the card follows the finger
and animates back to its slot on drop. Swapping inside Tasks, CheckLists or Ideas
is untouched.
A one-frame window remains: the gate lands with the recomposition that follows
onDragStarted, so a first onDrag in the same frame could in principle still
pick a foreign target. In practice long-press detection and the drag deltas come
in different pointer events. The data is safe either way (the guard still
rejects), and a next crossing realigns the row.
This has a trade-off, but I think is logical, if the app process ends mid-drag, i.e. by closing the app, the new order won't be persisted, but I think that is better than persisting the last saved drag, which may not be the goal position.
Also reordered the layout, showing ideas first and then checklists. Bumped version name to 3.5.3
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
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.