Skip to content

refactor: GTK action menu engine-drawn; drop dead right-click bodies (#395) - #427

Merged
JDonaghy merged 1 commit into
developfrom
issue-395-popovermenu-to-quadraui
May 16, 2026
Merged

refactor: GTK action menu engine-drawn; drop dead right-click bodies (#395)#427
JDonaghy merged 1 commit into
developfrom
issue-395-popovermenu-to-quadraui

Conversation

@JDonaghy

Copy link
Copy Markdown
Owner

Summary

  • Action menu migrated to engine-drawn path. The editor `…` action menu was the last popover-on-editor-DA still building a native `gtk4::PopoverMenu`. The engine-drawn ctx-menu path (paint at `draw.rs::draw_context_menu_popup`, click dispatch + hover via cached `ContextMenuLayout` at `gtk/mod.rs:6022`, keyboard via `engine::handle_context_menu_key`) was already fully wired by earlier work (feat: GTK tab/editor context menus use engine-drawn rendering #137, ContextMenu: motion handlers should use primitive's hit_test, not hand-rolled row math #210/fix: context menu click/hover uses cached ContextMenuLayout hit_test (#210) #425). Two changes hook it up: `click.rs` derives `(col, row)` from the click's pixel coords when opening the menu (was hardcoded `(0, 0)`) so the anchor lands under the `…` button, and the `show_action_menu_popover` call in `mod.rs:7113` is dropped — engine state + existing render + click dispatch take over.
  • 3 unused functions removed (~497 lines): `show_action_menu_popover` (now unused), `handle_tab_right_click` and `handle_editor_right_click` (both already replaced by `Msg::TabRightClick` / `Msg::EditorRightClick` in earlier PRs but left behind under `#[allow(dead_code)]`).
  • Explorer right-click NOT migrated here — scope split to GTK explorer context menu: migrate to engine-drawn ctx menu (cross-DA coord handling) #426. Engine-drawn ctx menu paints on the editor DA, but explorer right-clicks happen on the separate explorer DA with its own coord system; the migration needs cross-DA coordinate handling that warrants its own PR. `show_explorer_context_menu` + its 3 helpers (`build_gio_menu_from_engine_items`, `swap_ctx_popover`, `menu_row_count`) + the `active_ctx_popover` field stay in place for now.

Refs #395 (does not close — explorer follow-up is #426).

Files

  • `src/gtk/click.rs` — `+3 / -1`: pass real `(col, row)` to `open_editor_action_menu`.
  • `src/gtk/mod.rs` — `+4 / -507`: drop `show_action_menu_popover` call + 3 unused functions.
  • `SUMMARIES/gtk_mod.md` — line-count refresh.

Test plan

Smoke checklist added as a PR comment so the validating agent has it inline.

🤖 Generated with Claude Code

@JDonaghy

Copy link
Copy Markdown
Owner Author

Smoke test checklist (for the desktop agent picking this up)

1. Quality gate

cargo fmt
cargo clippy -- -D warnings
cargo test --no-default-features --lib
cargo build

All four must be green before merging. The clippy run is the critical one — this PR removed ~500 lines and changed the action-menu code path; clippy will catch any orphan imports, unused fields, or dead code that I might have missed.

2. Action menu smoke (the actual migrated piece)

This is the only behavior change in the PR. Everything else is dead-code removal.

  • Open a file in vimcode (GTK). Click the action menu button on the right side of the tab bar.
  • Menu appears under the button, not at top-left of the editor. (Before the click.rs anchor fix, it would have rendered at (0, 0).)
  • Click each item — verify the action fires:
    • Close All → all tabs close
    • Close Others / Close Saved / Close to the Right / Close to the Left
    • Word Wrap: On / Off → toggles :set wrap
    • Change Language → opens language picker
    • Reveal in Files → opens file manager at the file's directory
  • Click outside the menu → menu dismisses.
  • Press Esc → menu dismisses.
  • Press j / k → selection moves up/down inside the menu.
  • Press Enter on a selected item → action fires.

3. Regression checks — context menus that were already engine-drawn

These weren't touched by this PR but exercise the same engine-drawn path; verify nothing drifted:

  • Tab right-click: right-click on a tab → menu appears under the cursor. Click items (Close, Close Others, Close to the Right, Copy Path, Split). Each fires.
  • Editor right-click: right-click inside the editor → context menu (Cut/Copy/Paste/Go to Definition/etc.). Items fire.

4. NOT changed in this PR — explorer right-click

  • Right-click a file or folder in the explorer sidebar → native GTK PopoverMenu still appears (as before). Menu items (New File, Rename, Delete, etc.) still fire normally.

If the explorer menu looks visually different or items don't fire, that's a regression. The explorer migration is deferred to #426.

5. Branch ready-to-merge state

If everything above passes, this PR is ready for merge to develop. The CLAUDE.md workflow step 5 then says: when merged, run gh issue close <N> and unassign — but this PR only refs #395 (doesn't close it), since the explorer follow-up at #426 is what completes the original issue's scope. Keep #395 open until #426 also lands.

…#395)

The engine-drawn context-menu path (paint at draw.rs::draw_context_menu_popup,
click dispatch + hover via cached ContextMenuLayout at gtk/mod.rs:6022,
keyboard via engine::handle_context_menu_key) was fully wired in earlier
work (#137, #210/#425) but the editor "..." action menu kept building a
native gtk4::PopoverMenu on top. Two right-click handlers
(handle_tab_right_click, handle_editor_right_click) had already been
replaced by Msg::TabRightClick (mod.rs:4014) / Msg::EditorRightClick
(mod.rs:4042) but their bodies were left behind with #[allow(dead_code)].

- click.rs::handle_mouse_click derives (col, row) from the click's
  pixel coords when opening the editor action menu, so the engine's
  ContextMenuState anchor lands under the "..." button instead of (0, 0).
- gtk/mod.rs drops the show_action_menu_popover call branch; the
  engine-drawn renderer + click dispatch take over from there.
- Deletes 3 unused functions (~497 lines): show_action_menu_popover,
  handle_tab_right_click, handle_editor_right_click.

Explorer context menu (show_explorer_context_menu) stays on native
PopoverMenu in this PR — engine-drawn ctx menu paints on the editor
DA, but explorer right-clicks happen on the explorer DA (separate
GTK widget, independent coord system). Cross-DA handling tracked in

Refs #395.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JDonaghy
JDonaghy force-pushed the issue-395-popovermenu-to-quadraui branch from 3b3c9ef to c476b67 Compare May 16, 2026 22:52
@JDonaghy
JDonaghy merged commit e87d853 into develop May 16, 2026
JDonaghy added a commit that referenced this pull request May 17, 2026
PR #427 landed; ~497 lines of native-popover dead code removed.
Filed quadraui#205 (selection bg obscures top/bottom border),
vimcode#434 (menu opens AT button row, not below),
vimcode#435 (keyboard nav dead until click — same class as #273)
as follow-ups. #395 stays open until #426 (explorer right-click).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@JDonaghy
JDonaghy deleted the issue-395-popovermenu-to-quadraui branch May 17, 2026 00:37
JDonaghy added a commit that referenced this pull request May 17, 2026
The editor tab bar `…` action menu opened at the row of the button
itself (overlapping the tab row) instead of below it. After PR #427
migrated the action menu from gtk4::PopoverMenu to the engine-drawn
quadraui::ContextMenu, the click coords were anchored as-is and the
adapter never set a placement, so quadraui defaulted to AnchorPoint
(menu top-left == click point). TUI had been masking the same bug with
a `row + 1` hack at the call site.

Take option 1 from the issue: thread the trigger element's height
through the engine and use ContextMenuPlacement::Below.

- ContextMenuState gains `trigger_height: u16` (cells). 0 = right-click
  flow → AnchorPoint; non-zero = dropdown-style → Below.
- open_editor_action_menu(group_id, x, y, trigger_height) — new arg.
  The four right-click open_* functions default trigger_height to 0.
- ContextMenuPanel + render builder propagate the field.
- context_menu_panel_to_quadraui_context_menu sets placement based on
  trigger_height.
- Both backends switch from `.layout()` to `.layout_at()` with an
  anchor Rect carrying the trigger height in their native unit
  (line_height px for GTK, 1 cell for TUI).
- GTK click handler computes trigger height from tab_row_height_px /
  line_height (ceiled) — about 2 cells. The sub-cell rounding leaves a
  small gap below the button rather than overlap; cleaning that up is
  #168's job.
- TUI call sites drop the `row + 1` hack and pass row + height=1.

Auto-flip to Above when there's no room below is handled by quadraui's
layout_at; vimcode gets it for free.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JDonaghy added a commit that referenced this pull request Jul 17, 2026
…mServices

Msg::OpenFileDialog and Msg::SaveWorkspaceAsDialog now call
backend.services().show_file_open_dialog()/show_file_save_dialog()
(quadraui#427, now shipped) instead of constructing gtk4::FileDialog
in-line. dispatch() has no `backend` in scope, so the request is
stashed in a new PendingFileDialog Cell and drained in tick(), which
does receive the runner-owned `backend: &mut dyn quadraui::Backend` —
the same instance whose PlatformServices pump_depth is wired to the
runner's own event controllers, unlike App's separate self.backend
field (used for modal-stack/drag-state), which must NOT be used for
this (unshared pump_depth would reintroduce the #427 double-borrow
panic).

Msg::OpenFolderDialog stays on direct gtk4::FileDialog for now:
quadraui::PlatformServices has no folder-select dialog mode yet (only
file-open/file-save). That gap needs a new quadraui issue before the
third call site can migrate.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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