feat(windows-ui): embed widgets in table cells - #7075
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughWindows table cells now cache text or embedded widget handles. Widget HWNDs are clipped, positioned over ListView subitems, realigned during table changes, detached during cache invalidation, and handled through display notifications and cleanup tests. ChangesWin32 table widget cells
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RenderClosure
participant TableEntry
participant ListView
participant TableSubclass
participant WidgetHWND
RenderClosure->>TableEntry: return text or widget
TableEntry->>ListView: provide cached text or empty cell text
ListView->>TableSubclass: emit paint, scroll, resize, or notify message
TableSubclass->>WidgetHWND: position or hide widget HWND
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/perry-ui-windows/src/widgets/table.rs (1)
312-326: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a reentrancy guard around
layout_widget_cells.
WM_PAINT/WM_NOTIFY→layout_widget_cells→SetWindowPos/ShowWindow→ invalidation →WM_PAINT/LVN_GETDISPINFO→layout_widget_cellsagain.handle_dispinfo(Line 751) also calls it per widget cell, so a single repaint can re-run full layout many times. A thread-local in-progress flag (and skipping layout forWM_PAINTwhen nothing moved) would bound the work.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-ui-windows/src/widgets/table.rs` around lines 312 - 326, In the window procedure flow around DefSubclassProc and layout_widget_cells, add a thread-local reentrancy guard so nested WM_PAINT/WM_NOTIFY-triggered layout calls are skipped while layout is already in progress. Ensure the guard is reliably cleared after layout completes, and avoid running WM_PAINT layout when no widget movement requires it, while preserving layout_widget_cells calls from handle_dispinfo for legitimate outer layouts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-ui-windows/src/widgets/table.rs`:
- Around line 228-260: Update the table widget visibility and bounds logic
around cell_rect and the visible check to exclude the column-header band: obtain
the header with LVM_GETHEADER, convert its GetWindowRect coordinates into table
client coordinates, and clip or skip cells intersecting that region. Preserve
hiding for non-visible cells and ensure repositioned child widgets cannot paint
over the header even when rows are partially scrolled.
- Around line 299-310: Update the WM_NCDESTROY handling in table_subclass_proc
to remove the table entry from TABLES and drain its cell_cache rather than
cloning values while retaining the entry. Pass the drained widget contents to
detach_widget_cells, ensuring is_registered and layout_widget_cells can no
longer observe the destroyed table.
---
Nitpick comments:
In `@crates/perry-ui-windows/src/widgets/table.rs`:
- Around line 312-326: In the window procedure flow around DefSubclassProc and
layout_widget_cells, add a thread-local reentrancy guard so nested
WM_PAINT/WM_NOTIFY-triggered layout calls are skipped while layout is already in
progress. Ensure the guard is reliably cleared after layout completes, and avoid
running WM_PAINT layout when no widget movement requires it, while preserving
layout_widget_cells calls from handle_dispinfo for legitimate outer layouts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c582c3e-09d3-45d9-bb44-6d0fd9bf6b2b
📒 Files selected for processing (2)
changelog.d/7075-windows-table-widget-cells.mdcrates/perry-ui-windows/src/widgets/table.rs
|
Also addressed the review nit in a739dbd: embedded-cell layout now has a thread-local RAII reentrancy guard, preventing synchronous SetWindowPos/paint notifications from recursively rerunning a full table layout. |
Summary
Verification
cargo check --tests --target x86_64-pc-windows-msvc -p perry-ui-windowscargo test -p perry-ui-testcargo fmt --all -- --checkCloses #6616
Summary by CodeRabbit