Unify GTK (absolute) vs TUI (content-relative) window-rect coordinate conventions

[JDonaghy]

Summary

Follow-up from #547. GTK and TUI use two different coordinate conventions for the window_rects fed into Engine::calculate_group_window_rects / render::build_screen_layout, and every shared render helper that touches those rects (e.g. the new render::breadcrumb_draw_targets() from #547) has to carry an origin_offset parameter purely to paper over the difference. Unifying the convention would let that parameter — and the offset math at every call site — disappear.

The asymmetry

• GTK (src/gtk/mod.rs::render_content): editor_bounds = WindowRect::new(x, y, w, editor_area_h) where (x, y) comes from layout.main_content_bounds (i.e. absolute screen/pixel coordinates). Window rects, and everything derived from them (e.g. BreadcrumbBar::bounds), are already in final screen space.
• TUI (src/tui_main/render_impl.rs::build_screen_for_tui): content_bounds = WindowRect::new(0.0, 0.0, content_cols, content_rows) — always origin (0, 0), i.e. content-area-relative. Every draw call site (tab bars, breadcrumbs, dividers, windows) has to manually add editor_area.x / editor_area.y to translate back into terminal screen space.

This is presumably historical (TUI's content_bounds predates the shared render::build_screen_layout path), not a deliberate design choice — nothing in either backend's rendering model requires window rects to be in one convention over the other.

Proposed direction

Make TUI's window_rects (and therefore every WindowRect/bounds field derived from them — BreadcrumbBar::bounds, tab bar bounds, divider positions, etc.) absolute in terminal-screen space from the start, matching GTK. Candidates:

• Pass content_bounds with origin (editor_area.x, editor_area.y) instead of (0, 0) in build_screen_for_tui.
• Remove the per-call-site editor_area.x/.y offset additions this uncovers (breadcrumbs already went through this in GTK regressions from #515: editor breadcrumbs broken + explorer treeview icons don't use Nerd Fonts (converge on quadraui; fix quadraui gaps first) #547 — grep for origin_offset and editor_area.x as f64 in render_impl.rs and mouse.rs for the rest).
• Update/verify TUI's click and hit-test code (tui_main/mouse.rs) doesn't double-apply an offset that's now baked into the rects it reads.

Why this wasn't done in #547

This touches window-rect computation and every click/hit-test path in both backends — a much bigger, more architecturally invasive change than a bugfix PR should carry. It's exactly the kind of cross-cutting normalization the Platform-Neutral milestone (#7) exists for, but needs its own dedicated pass with full regression coverage on both backends' mouse/click paths, not a drive-by.

Acceptance criteria

• TUI's window rects (and everything derived from them) are absolute screen-space coordinates, matching GTK's convention.
• origin_offset-style parameters (e.g. on render::breadcrumb_draw_targets()) can be dropped/hardcoded to (0.0, 0.0) for both backends.
• All existing TUI click/hit-test tests (tui_main::mouse and friends) continue to pass, plus new coverage for the specific offset removal if any test previously encoded the old relative convention.

Parent context: Platform-Neutral milestone (#7), surfaced while working #547.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: —

Summary

Implemented #550: unified TUI's window-rect coordinate convention to absolute (matching GTK), eliminating the origin_offset param and ~40 editor_area.x/.y / editor_left / menu_rows offset-addition sites across render.rs, render_impl.rs, mouse.rs, and gtk/mod.rs+click.rs call sites. Added 2 regression tests proving click-hit-test now matches paint geometry with sidebar+menu bar visible. Full test suite (44 binaries), both cargo build variants, cargo clippy -D warnings (both variants), and cargo fmt all clean. One quadraui cross-repo constraint recorded in coord context (draw_find_replace still expects content-relative input internally; vimcode passes editor_left=0 as a workaround since quadraui can't be edited from here). Branch pushed, no PR opened per workflow.

[JDonaghy]

Coordinator: Assignment Complete

Machine: elitebook
Status: done
Exit code: 0
Duration: —

Summary

Smoke FAILED: TUI split-divider invisible/non-functional in both :vsplit and :split, drags fall through to editor text-selection — offset-fix regression, likely mouse.rs/render_impl.rs divider path

[JDonaghy]

Coordinator: Assignment Complete

Machine: elitebook
Status: done
Exit code: 0
Duration: —

Summary

Smoke FAILED: split-divider drag broken in BOTH TUI and GTK (drag falls through to editor text-selection) — scope wider than TUI-only offset fix, may be shared-code regression; full repro + suspected files in test reason

[JDonaghy]

Coordinator: Assignment Complete

Machine: elitebook
Status: done
Exit code: 0
Duration: —

Summary

Smoke PASSED for #550: binary/CLI/TUI render/breadcrumbs all good, no regression from this branch. Note: found a real split-divider drag bug (drag falls through to editor text-selection, both TUI+GTK) but operator confirmed it's pre-existing on develop, not caused by #550 — needs its own separate GitHub issue, not blocking this merge.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: —

Summary

Coordinate-unification diff is clean & well-tested, but the pinned smoke-test divider-drag failure (both backends) is unresolved — likely pre-existing per static analysis, needs confirmation before merge

Verdict: request-changes

Review findings

Review: issue-550 — Unify GTK/TUI window-rect coordinate convention

Summary of what I checked

• Fetched origin/issue-550-unify-gtk-absolute-vs-tui-content-relati and diffed against origin/develop (1237-line diff across render.rs, gtk/mod.rs, gtk/click.rs, tui_main/mouse.rs, tui_main/quadraui_tui.rs, tui_main/render_impl.rs).
• Did not run cargo build/cargo test/the app, per this review agent's standing rules (no build/test/run commands) — verification below is by static reading, cross-file offset arithmetic, and comparing call-site signatures.
• Read CLAUDE.md's Platform-Neutrality Rule and cross-checked the GTK/TUI backend split.

Diff quality: solid

• render::breadcrumb_draw_targets / render::tab_bar_draw_targets cleanly drop the origin_offset param; every call site (GTK mod.rs/click.rs, TUI render_impl.rs) was updated consistently, and I verified there are no orphaned callers left with the old 5/6-arg signature (grep across the full branch tree).
• build_screen_for_tui (render_impl.rs:49-63) now seeds content_bounds with editor_origin_x = area.x + ab_width + sidebar_cols, editor_origin_y = area.y + menu_height. I traced draw_frame's actual Layout split (menu row → activity/sidebar columns → editor_col) and confirmed the two formulas agree term-for-term — this is the crux of the change and it's correct.
• tui_main/mouse.rs: every hit-test site that used to do col.saturating_sub(editor_left) / row.saturating_sub(menu_rows) now compares the raw event col/row directly against absolute rects, consistently across divider-drag start, divider-drag move, tab bar click/hover/right-click, breadcrumb click, window/gutter/scrollbar hit-test, and text-column resolution. No case where a subtraction was correctly removed on one side but left in on a sibling code path that I could find. No underflow risk introduced — every col - x/row - y remaining is guarded by a preceding col >= x/row >= y bounds check.
• tui_main/quadraui_tui.rs::draw_find_replace now always passes editor_left=0 into quadraui::tui::draw_find_replace. I read the actual quadraui source (~/src/quadraui/quadraui/src/tui/find_replace.rs:49-51): with editor_left=0, gb_right = gb.x + gb.width and x = gb_right.saturating_sub(panel_w+1).max(0) — this matches vimcode's mirrored hit-test math in mouse.rs exactly (which also dropped the editor_left +/.max(editor_left)). This is the documented, unavoidable workaround since quadraui's signature can't be changed from this repo (per CLAUDE.md's "never edit quadraui directly" rule) — correctly called out in both the code comment and the pinned issue context. Good adherence to the Platform-Neutrality Rule: no new backend-specific logic was invented to route around this, just a constant-0 passthrough with a clear comment.
• New tests are real, not degenerate: test_breadcrumb_draw_targets_... and test_tab_bar_draw_targets_single_and_split were changed to use non-zero content_bounds origins ((10,20), (5,7)) specifically to prove pass-through-untouched behavior rather than accidentally-correct zero-origin arithmetic. Two new integration-style tests in mouse.rs (group_divider_click_matches_painted_divider_position, split_group_tab_bar_right_click_matches_painted_position) build a real ScreenLayout via the actual production build_screen_for_tui with sidebar+menu bar visible (maximal non-zero origin) and dispatch through the real handle_mouse, asserting both a hit and a 1-column miss. This is good regression coverage for exactly the offset-double-counting class of bug the issue calls out.
• Scope: stayed within the issue — no unrelated refactors, no core/ changes needed (as expected, since calculate_group_window_rects/dividers() were already coordinate-convention-agnostic; only what TUI feeds in changed).
• No security concerns (no I/O, no external input parsing beyond existing mouse-event handling).

Open concern: the pinned smoke-test failure (divider drag)

The carried-forward issue context states a smoke test on this exact branch found split-pane divider drag-to-resize completely non-functional in both TUI and GTK — clicks fall through to text-selection instead of grabbing the divider.

I looked hard for whether this diff causes or fails to fix that, specifically:

• GTK: grep -n "divider" src/gtk/click.rs src/gtk/mod.rs shows zero diff footprint in this PR — the divider press/drag code at gtk/mod.rs:7015-7064 (hit-test) and 7416-7442 (drag update) is completely untouched. GTK's window rects were already absolute pre-Unify GTK (absolute) vs TUI (content-relative) window-rect coordinate conventions #550 (per the issue's own problem statement), so this PR could not plausibly be the cause of a GTK-side divider regression.
• TUI: I traced both halves of the divider interaction in the new code:
  • Press/start-drag (mouse.rs "Group divider click — start drag", ~line 2825): col == div_col, (row as f64) >= div.cross_start && < div.cross_start + div.cross_size — correctly uses raw absolute col/row against div.position/.cross_start, which are now absolute since they derive from the now-absolute window_rects.
  • Move/resize (mouse.rs ~line 980-997): mouse_pos = col as f64 (or row), new_ratio = (mouse_pos - div.axis_start) / div.axis_size — also correctly using raw absolute coordinates.
  • Both are directly exercised by the new group_divider_click_matches_painted_divider_position test, which reads div.position/div.cross_start straight off a real painted ScreenLayout (not a hand-derived formula) and asserts a click there starts the drag and a click 1 column off does not.
  • Divider rendering (render_impl.rs ~line 387-416, the #481 duplicate-scrollbar-skip logic) is unmodified by this PR aside from the div_x/y_start/y_end no-longer-offset assignments, which are consistent with the rest of the change.

This is strong evidence for the issue context's own hypothesis (a): the divider-drag failure is a pre-existing bug unrelated to the TUI offset-unification, not something this branch introduced (GTK has zero footprint here; TUI's divider math in this diff is internally consistent and covered by a targeted new test).

However, I could not run the app to confirm this directly (build/run is out of scope for this review agent), and the smoke test is pinned as authoritative — it found the bug present on this branch's built artifact, not (yet) confirmed absent on develop HEAD. The issue's own acceptance criteria explicitly include "verify TUI's click/hit-test code doesn't double-apply an offset," which is precisely the class of bug the smoke test's symptom (drag falls through to text-selection) describes.

Verdict

Requesting changes — not because I found a defect in the diff itself (I didn't; the coordinate-unification work is clean, consistent, and well-tested), but because there's an unresolved, testing-confirmed regression in core interactive functionality (divider drag, both backends) on this exact branch that hasn't been triaged to a conclusion. Before merge:

1. Confirm whether the divider-drag failure reproduces on develop HEAD (pre-branch) — e.g. git stash/build develop directly and repeat the GTK repro steps from the smoke test. Given GTK's divider code has zero diff footprint here, I'd expect it does reproduce.
2. If confirmed pre-existing: note that explicitly (e.g. a PR comment or issue cross-link) so it isn't silently conflated with Unify GTK (absolute) vs TUI (content-relative) window-rect coordinate conventions #550, file/link a separate tracking issue for it, and this PR can proceed.
3. If NOT confirmed pre-existing (i.e. something about this branch specifically breaks it beyond what I could find by reading the diff): that needs an actual fix before merge, since it's exactly the class of regression Unify GTK (absolute) vs TUI (content-relative) window-rect coordinate conventions #550's acceptance criteria was meant to guard against.

Once (1)/(2) are done (or a fix lands per (3)), I'd expect to approve — the underlying coordinate-unification diff is solid.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: —

Summary

Investigated the reviewer's blocking concern (pinned divider-drag smoke-test failure) and confirmed it is pre-existing and unrelated to #550 -- no code change made, none needed. Evidence: (1) git diff origin/develop...HEAD -- src/gtk/mod.rs src/gtk/click.rs shows the GTK group-divider hit-test/drag code (mod.rs ~4319-4369, ~4726-4751) is BYTE-IDENTICAL to origin/develop -- zero footprint from this branch. (2) TUI's divider math in this diff is internally consistent (absolute coords throughout) and covered by the new group_divider_click_matches_painted_divider_position regression test. (3) Root cause found: the smoke test used :vsplit/:split, which create WindowLayout (Vim-style) window splits -- NOT GroupLayout (VSCode-style editor-group tab) splits. Mouse divider-drag-to-resize (group_divider_dragging/dragging_group_divider) is wired up ONLY for GroupLayout dividers in both backends; WindowLayout splits have zero drag-resize hit-test code and zero tests in either backend, completely untouched by #550. (4) Empirically reproduced with the built binary: launched TUI in tmux, ran :vsplit, confirmed the divider renders correctly and visibly (refuting 'no divider rendered at all'), then sent raw SGR mouse press+drag+release at the divider column -- cursor landed inside the LEFT window's text (Ln1,Col8) and the pane ratio never changed, exactly matching the smoke report's 'falls through to text-selection' symptom. This is a genuine, separate, pre-existing feature gap (window-split mouse-resize was never implemented in either backend) that needs its own tracking issue -- not a #550 regression. Recorded as a pinned finding via coord context add. Build/test/clippy/fmt all clean: cargo build (no warnings), cargo test --no-default-features (44/44 binaries, 0 failed), cargo clippy -- -D warnings (clean), cargo fmt --check (clean).

[JDonaghy]

Coordinator: Assignment Complete

Machine: dellserver
Status: done
Exit code: 0
Duration: —

Summary

issue-550 fix correctly removes all window-rect offset math, adds real regression tests; no regressions found

Verdict: approve

Review findings

Review: issue-550 — re-review iteration 1 (fix commit 5b21d32)

Scope: this branch is a single commit (5b21d32, presumably an amend/rebase over the
previously-reviewed state rather than a stacked fix commit). Since there's no separate
"before" commit to diff against, I diffed the full branch against origin/develop and
checked it against the two things the prior review round required:

1. the full origin_offset/editor_left/menu_rows removal across every window-rects-derived
   site, and
2. new regression coverage for divider-click and tab-bar-right-click hit-testing.

Both are the "iteration 1" work I re-reviewed. I did not re-litigate the divider-drag
smoke-test failure — an operator finding is already pinned on this issue confirming that's a
pre-existing, out-of-scope gap (WindowLayout splits have no mouse-resize in either backend;
gtk/mod.rs/gtk/click.rs are confirmed byte-identical to develop except the unrelated
origin_offset signature simplification).

What I checked

• render.rs: origin_offset param dropped entirely from breadcrumb_draw_targets and
  tab_bar_draw_targets. Tests updated to use non-zero content_bounds origins (10,20 and
  5,7) to actually exercise pass-through-of-absolute-bounds behavior instead of the old
  translate-by-offset behavior. Good — this is a real behavioral test, not just a signature
  update.
• gtk/mod.rs / gtk/click.rs: only the (0.0, 0.0) argument removed from both call
  sites, consistent with GTK's convention already being absolute. No GTK behavior change.
• render_impl.rs: build_screen_for_tui's new editor_origin_x/editor_origin_y is
  computed with the exact same formula (ab_width, sidebar_cols, menu_height, same
  engine.app_shell.sidebar_visible()/engine.settings.autohide_panels conditions) as
  draw_frame's real ratatui::Layout split produces for editor_area. I traced both
  independently line-by-line — they agree, which is the load-bearing invariant for this whole
  change (two independently-computed origins that must land on the same absolute point).
  Every divider/window/separator draw site that used to add editor_area.x/.y now compares
  raw absolute coordinates directly (div.position, div.cross_start, window.rect, etc.),
  and the bound checks (div_x < editor_area.x + editor_area.width, div_x > editor_area.x)
  are still correct against the absolute values.
• quadraui_tui.rs: draw_find_replace wrapper permanently passes editor_left=0 into
  quadraui's still-content-relative-assuming rasteriser, since group_bounds is now absolute
  and quadraui's signature can't be changed from this repo. Matches the pinned finding
  exactly, well documented, correctly scoped as a workaround rather than attempted as a "fix"
  in the wrong repo.
• mouse.rs (the bulk of the diff, ~40 sites): I checked every remaining
  rel_col/editor_row usage after the patch (grepped the whole file) and confirmed the
  split is exactly right — split-group tab-bar/divider/window/scrollbar hit-tests now compare
  raw col/row against absolute gtb.bounds/rw.rect/div.* fields with no offset, while
  the single-group tab-bar hit-test (bar-local, not window-rects-derived) correctly keeps
  rel_col = col - editor_left. Also verified the find/replace hit-test's dropped
  .max(editor_left) clamp is correct, not a regression: quadraui's paint-side clamp is now
  called with editor_left=0, making it a no-op, so removing the vimcode-side clamp keeps
  click math mirroring paint math exactly (restoring it would have reintroduced a paint/click
  drift for narrow groups).
• Confirmed via full-repo grep that no stale call sites remain anywhere for
  tab_bar_draw_targets/breadcrumb_draw_targets/render_all_windows/render_separators
  with the old signatures — all 4 files that reference them were updated consistently.
• New tests group_divider_click_matches_painted_divider_position and
  split_group_tab_bar_right_click_matches_painted_position build a real ScreenLayout via
  build_screen_for_tui (menu bar + sidebar visible, non-trivial origin) and dispatch clicks
  at coordinates read directly off that painted layout — not hand-derived formulas — which is
  exactly the right test shape to catch a future paint/click drift regression. Both also assert
  the "one column off" / miss case, not just the hit case.

Verdict

The offset-removal is thorough and internally consistent, the two backends' behavior is
unchanged where it should be (GTK) and corrected where it needed to be (TUI), the
previously-flagged quadraui workaround is documented and appropriately out of scope, and the
new tests exercise the change through production code paths rather than reimplementing the
math by hand. I found no regressions in the diff.

Non-blocking notes (do not require another fix round):

• The branch is a single commit rather than an original commit + a separate fix commit — fine
  for landing, just means there's no isolated "here's the delta since last review" diff to
  point at; I diffed the whole branch against develop instead.
• Worth opening a follow-up tracking issue (per the already-pinned operator finding) for
  WindowLayout (:vsplit/:split) mouse divider-drag-to-resize being unimplemented in both
  backends — unrelated to Unify GTK (absolute) vs TUI (content-relative) window-rect coordinate conventions #550 but a real gap the smoke test surfaced.

REVIEW_VERDICT: approve