Skip to content

Reuse list rows safely during scrolling - #122

Open
k-kostin wants to merge 2 commits into
omacom:mainfrom
k-kostin:codex/reuse-list-rows
Open

k-kostin wants to merge 2 commits into
omacom:mainfrom
k-kostin:codex/reuse-list-rows

Conversation

@k-kostin

Copy link
Copy Markdown

Summary

Reuse FolderList delegates during scrolling, rather than rebuilding the same row components for each newly visible file.

row leaves the live area → pool → update roles → reuse
                            ├─ clear drag MIME/image and invalidate pending grabs
                            └─ discard the previous file's tap history

Pooled rows release their thumbnail source and stop handling input. A late drag-image callback is accepted only for the current request and path. While a drag is active, the list uses its existing non-reusing behavior, so the source cannot be recycled into another file. Grid and column views do not enable reuse; timers and cacheBuffer are unchanged.

Evidence

  • Before: 64 large jumps through 10,000 text files constructed 1,034 rows and used a median 217 ms of GUI-thread CPU.
    After: 42 rows constructed and 35 ms of GUI-thread CPU. With alternating synthetic PNG/text files, GUI CPU fell from 242 to 76 ms and all-thread CPU from 386 to 217 ms. Five interleaved measured pairs per scenario, after discarded warm-ups, Qt 6.12 Release/offscreen software rendering on the same machine. These measure cumulative scroll work, not FPS; first display after model preload and peak live row count were approximately unchanged.
  • Added a separate rowreuse_tests target: state cleanup in LTR/RTL, different-file clicks within the double-click interval, release after scrolling, drag/reuse restoration, delayed grabs across A→B→A, and window destruction. The double-click case caught and prevented a pilot regression: a click on a reused row could otherwise activate the new file immediately.
  • Full local suite: folio_tests 196 passed, rowreuse_tests 9 passed; the new target also passed 10 consecutive runs.
  • Native Wayland acceptance tests in an isolated Weston/Xvfb session, with server-delivered pointer input: a selected file survives programmatic list jumps during drag, is moved into a real temporary folder by FileOps, and a subsequent Escape cancels without moving the next file. Passed in LTR/RTL. Destruction of an unselected drag source during a large jump cancels the drag on both main and this branch; this change does not pin the source.

Merge Danger

Door: Two-way. No stored data or format changes.

Blast Radius: List. FileRow is also used by column views, which were smoke-tested in LTR/RTL with enter/back navigation. This targets main independently; overlapping pending thumbnail/column changes will need a fresh check when combined.

@greptile-apps

greptile-apps Bot commented Nov 13, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 4/5

[Medium impact] The effects are limited to the folder-list browsing workflow.

The PR appears safe to merge, with a non-blocking gap in thumbnail test coverage.

Findings

  1. P2 Thumbnail reuse is untested ▶

Summary

Enables row reuse in FolderList while keeping drag and click state tied to the correct file.

  • FolderList reuses rows during scrolling without carrying old file state.

Known behavior acknowledged by k-kostin: scrolling can destroy an unselected drag source and cancel the drag on both branches. Pinning that source is intentionally outside this change.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    A[Live row] --> B[Pooled row]
    B --> C[Clear drag assets and thumbnail source]
    C --> D[Destroy tap handler and disable input]
    D --> E[Update file roles]
    E --> F[Reuse row with fresh tap handler]
    G[Active drag] --> H[Disable row reuse]
    H --> I[Drag ends]
    I --> A
Loading

Reviews (1) · Last reviewed commit: "Reset tap history on row reuse and cover..." · Reviewed by Greptile

Comment thread tests/rowreuse_tests.cpp

ListFixture() {
QDir().mkpath(folder());
for (int i = 0; i < 400; ++i) writeFile(folder() + QString("/file%1.txt").arg(i, 3, 10, QChar('0')));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Thumbnail reuse is untested

All fixtures are text files, so these tests never exercise the changed thumbnail binding. The suite can pass even if pooled rows keep thumbnails or reused rows show the wrong picture. Add an image fixture, check that pooling clears its thumbnail source, then reuse that row for another image and a text file.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

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.

1 participant