#731: GTK: 22 Relm4-era widget handles are permanently None — the second orphan pocket #672 could not see (and why #723's fix cannot run) - #744
Merged
JDonaghy merged 2 commits intoSep 2, 2026
Conversation
22 `Rc<RefCell<Option<gtk4::...>>>` fields on `App` were initialised `None` at construction and never assigned anywhere under the ShellApp runner (the quadraui runner owns the single real DrawingArea; these Relm4-era handles never got reconnected after #540). They guarded ~103 `if let Some`/`match` arms that could never execute. Deletes the fields, their initialisers, and every dead arm — plus 2 more of the same species found during the sweep (`sidebar_revealer`, `explorer_ctx_menu_layout`), and the now-fully-dead helpers `sync_scrollbar`, `create_window_scrollbars`, `sync_scrollbar_positions`, `native_scrollbar_margin_start`, and `struct WindowScrollbars`. Re-derives #723 against reality: that fix (`e02a824`) inset the native `gtk4::Scrollbar` overlay past the minimap strip, but the only code that ever created that widget was `sync_scrollbar`/`create_window_scrollbars` — both gated on the same permanently-`None` handles, so the fix was never live on screen. quadraui's `gtk::editor::draw_editor` documents that it deliberately skips scrollbars on GTK and defers to that dead host path (unlike its TUI twin, which paints an inline scrollbar column as part of the shared `Editor` primitive). GTK's editor currently paints no scrollbar at all, vertical or horizontal. The inset math and the quadraui-side gap are recorded in a doc comment at the `Surface::Editor` push site in `render_content` — closing it is quadraui-side work per CLAUDE.md's Platform-Neutrality Rule, not something to patch here with new GTK-specific widget plumbing. The largest single finding: a ~135-line block in `App::tick` polled `self.mouse_pos_cell` at 20Hz to drive four passive-hover features — h-scrollbar hover, tab-close (×) hover + tooltip, debug toolbar button hover, and LSP hover-on-dwell popups. All four were gated on the same dead `drawing_area`-derived size and have not worked since #540; nothing else in the file writes those hover fields or calls `Engine::editor_hover_mouse_move`. Documented in place rather than "fixed" — the removed code's `(0, 0)`-origin math was itself already flagged by a neighboring comment as the #582/#646 coordinate-frame bug, so restoring it needs a rewrite, not a re-wire. Also found and documented in place (all pre-existing, confirmed unaffected by this diff): `terminal_cols`/`terminal_target_maximize_rows` pinned at fallback constants (terminal PTY sizing never tracks the real DA size), `focus_editor_if_needed` a no-op at ~10 call sites (focus never returns to the editor DA when leaving a sidebar panel), and the Settings sidebar's click routing permanently seeing a zero-size panel (every click at/below the search row falls into "open settings.json" instead of reaching the per-row edit path). Two small, deliberate exceptions to "no observable behavior change": debug-output-panel scroll and ext-panel hover now call `self.draw_needed.set(true)` where they previously called `queue_draw()` on an always-`None` widget (i.e. never redrew) — using the mechanism that actually works under ShellApp instead of the one that never did. `Msg::TerminalToggleSplit`'s `cols` is preserved as its literal computed value (`0`, not a `terminal_cols()`-style `80` fallback) to avoid silently changing that arm's behavior. Net: -1288 production lines in src/gtk/mod.rs. `grep -rn 'gtk4::\(Box\|Overlay\|ScrolledWindow\)' src/gtk/` now returns nothing. No field in `App` is Option-typed, initialised `None`, and never assigned. Testing: added `gtk::testing::scrollbar_paint`, a `GtkDriver` black-box test proving no scrollbar-colored pixels paint for an overflowing editor pane — the executable evidence for the #723 re-diagnosis above. It is not a red→green bug-fix proof (painting is unchanged by this diff, so it passes identically before and after); it pins today's confirmed-absent behavior so a future quadraui-side fix must update it deliberately, not silently. `cargo build && cargo test --bin vimcode && cargo clippy -- -D warnings && cargo clippy --no-default-features -- -D warnings && cargo fmt --check` all pass. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ert the unreachable ext-panel-hover "fix" Addresses the blocking review finding: two self-acknowledged behavior changes shipped with zero black-box coverage. 1. debug-output-panel scroll (`"debug_output" =>` arm): this one is real and live. Adds `wheel_scroll_up_on_debug_output_panel_repaints_the_ unpinned_view` (src/gtk/testing.rs), a GtkDriver test that snapshots the panel's painted pixels, scrolls, and asserts the repainted pixels differ — not just that `debug_output_scroll` (engine state) moved, which is exactly the #587/#592 anti-pattern CLAUDE.md calls out. Verified RED against the pre-#731 code (temporarily reverted the `draw_needed.set(true)` back to the dead `queue_draw()`-on-`None` shape and reran just this test: fails byte-for-byte identical, as expected) and GREEN with the fix restored. Getting a real pixel diff needed re-deriving the actual mechanism: scrolling *down* (the existing state-only test's direction) never moves painted content, because `dap_output_lines` starts `auto_scroll: true` (pinned to the tail) and `handle_debug_output_scroll`'s `delta_y > 0.0` branch never turns auto_scroll off. Scrolling *up* hits the `else` branch, which unconditionally sets `auto_scroll = false`, switching `quadraui::TextDisplay`'s painted view from "pinned to the tail" to "pinned to scroll_offset" — a real, visible repaint even when `debug_output_scroll` itself stays 0. 2. ext-panel hover (`Msg::ExtPanelMouseMove` handler): re-investigation found this arm is not just guarded by a dead widget handle — the `Msg::ExtPanelMouseMove` variant itself is never constructed anywhere in the crate (confirmed via full-repo grep), both before and after this issue's changes. The `EventControllerMotion` that used to send it was removed by the #540 ShellApp cutover and nothing replaced it. With no UI event path able to reach this arm, "add a driver test" per the reviewer's suggested remedy isn't available, so this takes the reviewer's other suggested remedy: revert to a strict no-op (drop the `draw_needed.set(true)`, matching what the dead `queue_draw()` on the always-`None` handle already was), with a comment explaining why and what restoring the feature for real would need. This also resolves the reviewer's non-blocking note about the sibling early-return branch lacking the same call — both branches are now equally no-op, so there's no more inconsistency to flag. Net effect: exactly one behavior change ships in this PR (debug-output scroll now visibly repaints), and it has driver coverage proving it. cargo build / cargo clippy -- -D warnings / cargo fmt --check all pass. `cargo clippy --all-targets -- -D warnings` still fails, but identically on the unmodified branch tip (6b0deaf) before this commit — ~90 pre-existing non_snake_case-named vim-motion tests (src/core/engine/tests.rs, tests/new_vim_features.rs) plus one write_with_newline lint (tests/breadcrumbs.rs), none of them in files this issue touches. This is the toolchain-drift gotcha CLAUDE.md documents ("CI's Test (Linux, headless) is red but everything passes locally") — out of scope for this fix iteration. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Closes #731
Automated PR opened by coordinator for review of issue #731.