Skip to content

ContextMenu: motion handlers should use primitive's hit_test, not hand-rolled row math #210

Description

@JDonaghy

Problem

Both TUI and GTK consume `quadraui::ContextMenu` for context menus + top-bar dropdowns. The primitive ships a `ContextMenuLayout::hit_test(x, y)` that maps a mouse position to the right item — but the motion handlers that update the engine's hover state DON'T call it. Each backend computes `item_idx = (y / line_height).floor()` by hand and then writes the result into engine state, which the rasteriser later reads.

This means every time the rasteriser's row layout changes, the motion handlers have to be kept in sync by hand. When they drift, hover lands on the wrong row.

Concrete bugs surfaced

  1. Phase B.5 — GTK chrome catch-up onto quadraui primitives #205 slice 6 (a77cd72/95c0e4d): rasteriser used half-height separator rows; motion handler assumed uniform rows; hover off-by-half-row past every separator. Fixed by forcing uniform row heights in the rasteriser — papered over the structural issue without removing it.
  2. Phase B.5 — GTK chrome catch-up onto quadraui primitives #205 slice 6 follow-up (just now): motion handler clears engine `highlighted_item_idx` to None when hovering separator → adapter falls back to `unwrap_or(0)` → first item highlights instead. Fixed by changing adapter to use `usize::MAX` sentinel.

Both issues come from the motion handler computing `item_idx` independently of the layout. If it called `menu_layout.hit_test()` directly, neither bug would be possible.

Proper fix

Replace the hand-rolled row math in:

  • `src/gtk/mod.rs` motion handler near line 2530 (menu dropdown)
  • `src/gtk/mod.rs` editor DA's motion handler that fires for context menus
  • `src/tui_main/mouse.rs` equivalent paths

…with a call into a shared helper that:

  1. Builds the same `quadraui::ContextMenu` the rasteriser will use (via the adapter).
  2. Builds the same layout (with the same per-item measurer).
  3. Calls `menu_layout.hit_test(mouse_x, mouse_y)`.
  4. Translates the resulting `ContextMenuHit::Item(id)` back to engine_idx (or, better: stores the quadraui idx directly in engine state so no translation is needed).

The catch: motion handlers fire at 60+ Hz. Re-building the layout every move is allocate-heavy. Either:

  • Cache the layout in some component-level Rc/RefCell on menu open (clear on close)
  • Run hit-test in the rasteriser only, and have the motion handler just notify the renderer of the new mouse position (no engine round-trip)

The second option is structurally cleaner and matches how `draw_context_menu_popup` already handles right-click context menu hover today (mouse_pos passed into rasteriser, hit_test inside). But it requires plumbing mouse_pos through the menu dropdown's draw_func, which currently doesn't receive it.

Related

Priority

Medium. Today's slice 6 fixes are correct enough for users; this is structural debt that will keep recurring as more surfaces migrate.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinginfrastructureBuild, CI, distributionuiUI/rendering

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions