From c476b67c706197b4cad4f129ffb69bea6b3a00cb Mon Sep 17 00:00:00 2001 From: JDonaghy Date: Sat, 16 May 2026 20:41:24 +0000 Subject: [PATCH] refactor: drop native PopoverMenu for action menu + dead right-clicks (#395) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- SUMMARIES/gtk_mod.md | 2 +- src/gtk/click.rs | 4 +- src/gtk/mod.rs | 511 +------------------------------------------ 3 files changed, 8 insertions(+), 509 deletions(-) diff --git a/SUMMARIES/gtk_mod.md b/SUMMARIES/gtk_mod.md index 77b3038d..5b0e0a31 100644 --- a/SUMMARIES/gtk_mod.md +++ b/SUMMARIES/gtk_mod.md @@ -1,4 +1,4 @@ -# src/gtk/mod.rs — 10,888 lines +# src/gtk/mod.rs — 10,256 lines GTK4/Relm4 application shell. Defines the `App` struct, `Msg` enum, and `SimpleComponent` impl (init/view/update). Contains the main event loop, window setup, input handling, and all GTK widget wiring. diff --git a/src/gtk/click.rs b/src/gtk/click.rs index c5fc778f..8164481f 100644 --- a/src/gtk/click.rs +++ b/src/gtk/click.rs @@ -322,7 +322,9 @@ pub(super) fn handle_mouse_click( (None, ea) } ClickTarget::ActionMenuButton(group_id) => { - engine.open_editor_action_menu(group_id, 0, 0); + let col = (x / char_width.max(1.0)) as u16; + let row = (y / line_height.max(1.0)) as u16; + engine.open_editor_action_menu(group_id, col, row); (None, None) } _ => (None, None), diff --git a/src/gtk/mod.rs b/src/gtk/mod.rs index cdf1302f..a232af39 100644 --- a/src/gtk/mod.rs +++ b/src/gtk/mod.rs @@ -7047,22 +7047,17 @@ impl App { // Buffer click — fire hooks and reveal file } None => { - // Check if the click opened an editor action menu. + // Engine-drawn action menu is already opened with the + // correct anchor by click.rs::handle_mouse_click. The + // engine-drawn renderer at draw.rs:906 + click dispatch + // at line ~6022 take over from here (#395). if engine.context_menu.as_ref().is_some_and(|cm| { matches!( cm.target, core::engine::ContextMenuTarget::EditorActionMenu { .. } ) }) { - let group_id = - match &engine.context_menu.as_ref().unwrap().target { - core::engine::ContextMenuTarget::EditorActionMenu { - group_id, - } => *group_id, - _ => unreachable!(), - }; drop(engine); - self.show_action_menu_popover(group_id, x, y, sender); self.draw_needed.set(true); return; } @@ -7527,504 +7522,6 @@ impl App { self.draw_needed.set(true); } - fn show_action_menu_popover( - &mut self, - group_id: core::window::GroupId, - x: f64, - y: f64, - _sender: &ComponentSender, - ) { - let da = match self.drawing_area.borrow().as_ref() { - Some(da) => da.clone(), - None => return, - }; - - // Extract the items from the engine context menu (already populated). - let items: Vec = { - let engine = self.engine.borrow(); - engine - .context_menu - .as_ref() - .map(|cm| cm.items.clone()) - .unwrap_or_default() - }; - // Close the engine-side context menu; GTK handles it natively. - self.engine.borrow_mut().close_context_menu(); - - let menu = build_gio_menu_from_engine_items(&items, "actmenu"); - - let enabled_map: std::collections::HashMap = items - .iter() - .map(|it| (it.action.clone(), it.enabled)) - .collect(); - - let actions = gtk4::gio::SimpleActionGroup::new(); - - // Register an action for each menu item that delegates to engine. - for item in &items { - let action_name = item.action.clone(); - let engine_ref = self.engine.clone(); - let draw_ref = self.draw_needed.clone(); - let gid = group_id; - let a = gtk4::gio::SimpleAction::new(&action_name, None); - let act = action_name.clone(); - a.connect_activate(move |_, _| { - let mut e = engine_ref.borrow_mut(); - e.active_group = gid; - // Re-open the context menu so confirm() can find items. - e.open_editor_action_menu(gid, 0, 0); - // Find and select the matching item. - if let Some(ref mut cm) = e.context_menu { - if let Some(idx) = cm.items.iter().position(|i| i.action == act) { - cm.selected = idx; - } - } - e.context_menu_confirm(); - draw_ref.set(true); - }); - if enabled_map.get(&action_name) == Some(&false) { - a.set_enabled(false); - } - actions.add_action(&a); - } - - da.insert_action_group("actmenu", Some(&actions)); - - let n_rows = menu_row_count(&menu); - swap_ctx_popover(&self.active_ctx_popover, { - let popover = gtk4::PopoverMenu::from_model(Some(&menu)); - popover.set_parent(&da); - popover.set_pointing_to(Some(>k4::gdk::Rectangle::new(x as i32, y as i32, 1, 1))); - popover.set_has_arrow(false); - popover.set_position(gtk4::PositionType::Bottom); - popover.set_size_request(-1, n_rows * 22 + 14); - popover - }); - if let Some(ref p) = *self.active_ctx_popover.borrow() { - p.popup(); - } - } - - #[allow(dead_code)] - fn handle_tab_right_click( - &mut self, - group_id: core::window::GroupId, - tab_idx: usize, - x: f64, - y: f64, - _sender: &ComponentSender, - ) { - let da = match self.drawing_area.borrow().as_ref() { - Some(da) => da.clone(), - None => return, - }; - - // Build gio::Menu from engine-generated items (single source of truth). - let items: Vec = { - let mut engine = self.engine.borrow_mut(); - engine.open_tab_context_menu(group_id, tab_idx, 0, 0); - let items = engine - .context_menu - .as_ref() - .map(|cm| cm.items.clone()) - .unwrap_or_default(); - engine.close_context_menu(); - items - }; - - let menu = build_gio_menu_from_engine_items(&items, "tabctx"); - - // Collect enabled state from engine items keyed by action string. - let enabled_map: std::collections::HashMap = items - .iter() - .map(|it| (it.action.clone(), it.enabled)) - .collect(); - - // Build action group - let actions = gtk4::gio::SimpleActionGroup::new(); - - macro_rules! tab_action { - ($name:expr, $engine:expr, $draw:expr, $body:expr) => {{ - let engine_ref = $engine.clone(); - let draw_ref = $draw.clone(); - let a = gtk4::gio::SimpleAction::new($name, None); - a.connect_activate(move |_, _| { - $body(&engine_ref, &draw_ref); - }); - if enabled_map.get($name) == Some(&false) { - a.set_enabled(false); - } - actions.add_action(&a); - }}; - } - - { - let engine_ref = self.engine.clone(); - let draw_ref = self.draw_needed.clone(); - let sender = self.sender.clone(); - let a = gtk4::gio::SimpleAction::new("close", None); - a.connect_activate(move |_, _| { - let mut e = engine_ref.borrow_mut(); - e.active_group = group_id; - if let Some(g) = e.editor_groups.get_mut(&group_id) { - g.active_tab = tab_idx; - } - if e.dirty() { - drop(e); - let _ = sender.send(Msg::ShowCloseTabConfirm); - } else { - e.close_tab(); - draw_ref.set(true); - } - }); - actions.add_action(&a); - } - - tab_action!( - "close_others", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - let mut e = engine_ref.borrow_mut(); - e.active_group = group_id; - if let Some(g) = e.editor_groups.get_mut(&group_id) { - g.active_tab = tab_idx; - } - e.close_other_tabs(); - draw_ref.set(true); - } - ); - tab_action!( - "close_right", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - let mut e = engine_ref.borrow_mut(); - e.active_group = group_id; - if let Some(g) = e.editor_groups.get_mut(&group_id) { - g.active_tab = tab_idx; - } - e.close_tabs_to_right(); - draw_ref.set(true); - } - ); - tab_action!( - "close_saved", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - let mut e = engine_ref.borrow_mut(); - e.active_group = group_id; - if let Some(g) = e.editor_groups.get_mut(&group_id) { - g.active_tab = tab_idx; - } - e.close_saved_tabs(); - draw_ref.set(true); - } - ); - tab_action!( - "copy_path", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - let e = engine_ref.borrow(); - if let Some(path) = e.tab_file_path(group_id, tab_idx) { - let text = path.to_string_lossy().to_string(); - if let Some(ref cb) = e.clipboard_write { - let _ = cb(&text); - } - drop(e); - engine_ref.borrow_mut().message = format!("Copied: {text}"); - } - draw_ref.set(true); - } - ); - tab_action!( - "copy_relative_path", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - let e = engine_ref.borrow(); - if let Some(path) = e.tab_file_path(group_id, tab_idx) { - let rel = e.copy_relative_path(&path); - if let Some(ref cb) = e.clipboard_write { - let _ = cb(&rel); - } - drop(e); - engine_ref.borrow_mut().message = format!("Copied: {rel}"); - } - draw_ref.set(true); - } - ); - tab_action!( - "reveal", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, _draw_ref: &Rc>| { - let e = engine_ref.borrow(); - if let Some(path) = e.tab_file_path(group_id, tab_idx) { - drop(e); - engine_ref.borrow().reveal_in_file_manager(&path); - } - } - ); - tab_action!( - "split_right", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - engine_ref - .borrow_mut() - .split_window(core::window::SplitDirection::Vertical, None); - draw_ref.set(true); - } - ); - tab_action!( - "split_down", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - engine_ref - .borrow_mut() - .split_window(core::window::SplitDirection::Horizontal, None); - draw_ref.set(true); - } - ); - tab_action!( - "group_split_right", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - engine_ref - .borrow_mut() - .open_editor_group(core::window::SplitDirection::Vertical); - draw_ref.set(true); - } - ); - tab_action!( - "group_split_down", - self.engine, - self.draw_needed, - |engine_ref: &Rc>, draw_ref: &Rc>| { - engine_ref - .borrow_mut() - .open_editor_group(core::window::SplitDirection::Horizontal); - draw_ref.set(true); - } - ); - - da.insert_action_group("tabctx", Some(&actions)); - - let n_rows = menu_row_count(&menu); - swap_ctx_popover(&self.active_ctx_popover, { - let popover = gtk4::PopoverMenu::from_model(Some(&menu)); - popover.set_parent(&da); - popover.set_pointing_to(Some(>k4::gdk::Rectangle::new(x as i32, y as i32, 1, 1))); - popover.set_has_arrow(false); - popover.set_position(gtk4::PositionType::Right); - popover.set_size_request(-1, n_rows * 22 + 14); - popover - }); - if let Some(ref p) = *self.active_ctx_popover.borrow() { - p.popup(); - } - } - - #[allow(dead_code)] - fn handle_editor_right_click(&mut self, x: f64, y: f64) { - let da = match self.drawing_area.borrow().as_ref() { - Some(da) => da.clone(), - None => return, - }; - - // Build gio::Menu from engine-generated items (single source of truth). - let items: Vec = { - let mut engine = self.engine.borrow_mut(); - engine.open_editor_context_menu(0, 0); - let items = engine - .context_menu - .as_ref() - .map(|cm| cm.items.clone()) - .unwrap_or_default(); - engine.close_context_menu(); - items - }; - - let menu = build_gio_menu_from_engine_items(&items, "edctx"); - - let enabled_map: std::collections::HashMap = items - .iter() - .map(|it| (it.action.clone(), it.enabled)) - .collect(); - - let actions = gtk4::gio::SimpleActionGroup::new(); - - // Helper macro to reduce boilerplate for engine-driven actions. - macro_rules! add_editor_ctx_action { - ($name:expr, $engine_rc:expr, $draw_rc:expr, $body:expr) => {{ - let engine_ref = $engine_rc.clone(); - let draw_ref = $draw_rc.clone(); - let a = gtk4::gio::SimpleAction::new($name, None); - a.connect_activate(move |_, _| { - ($body)(&engine_ref, &draw_ref); - }); - if enabled_map.get($name) == Some(&false) { - a.set_enabled(false); - } - actions.add_action(&a); - }}; - } - - add_editor_ctx_action!( - "goto_definition", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - eng.borrow_mut().lsp_request_definition(); - dr.set(true); - } - ); - - add_editor_ctx_action!( - "goto_references", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - eng.borrow_mut().lsp_request_references(); - dr.set(true); - } - ); - - add_editor_ctx_action!( - "rename_symbol", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - let mut e = eng.borrow_mut(); - e.mode = core::Mode::Command; - e.command_buffer = "Rename ".to_string(); - dr.set(true); - } - ); - - add_editor_ctx_action!( - "open_changes", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - eng.borrow_mut().open_diff_peek(); - dr.set(true); - } - ); - - add_editor_ctx_action!( - "cut", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - let mut e = eng.borrow_mut(); - if matches!( - e.mode, - core::Mode::Visual | core::Mode::VisualLine | core::Mode::VisualBlock - ) { - e.yank_visual_selection(); - if let Some((ref text, _)) = e.registers.get(&'"') { - let text = text.clone(); - if let Some(ref cb) = e.clipboard_write { - let _ = cb(&text); - } - } - let mut changed = false; - e.delete_visual_selection(&mut changed); - } - dr.set(true); - } - ); - - add_editor_ctx_action!( - "copy", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - let mut e = eng.borrow_mut(); - if matches!( - e.mode, - core::Mode::Visual | core::Mode::VisualLine | core::Mode::VisualBlock - ) { - e.yank_visual_selection(); - if let Some((ref text, _)) = e.registers.get(&'"') { - let text = text.clone(); - if let Some(ref cb) = e.clipboard_write { - let _ = cb(&text); - } - } - e.mode = core::Mode::Normal; - } - dr.set(true); - } - ); - - add_editor_ctx_action!( - "paste", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - let mut e = eng.borrow_mut(); - if let Some(ref cb_read) = e.clipboard_read { - if let Ok(text) = cb_read() { - if !text.is_empty() { - e.registers.insert('"', (text, false)); - let mut changed = false; - e.paste_after(&mut changed); - } - } - } - dr.set(true); - } - ); - - add_editor_ctx_action!( - "open_side_vsplit", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - let mut e = eng.borrow_mut(); - if let Some(path) = e.file_path().map(|p| p.to_path_buf()) { - e.split_window(core::window::SplitDirection::Vertical, None); - let _ = e.open_file_with_mode(&path, core::OpenMode::Permanent); - } - dr.set(true); - } - ); - - add_editor_ctx_action!( - "command_palette", - self.engine, - self.draw_needed, - |eng: &std::cell::RefCell, dr: &std::cell::Cell| { - eng.borrow_mut() - .open_picker(core::engine::PickerSource::Commands); - dr.set(true); - } - ); - - da.insert_action_group("edctx", Some(&actions)); - - let n_rows = menu_row_count(&menu); - swap_ctx_popover(&self.active_ctx_popover, { - let popover = gtk4::PopoverMenu::from_model(Some(&menu)); - popover.set_parent(&da); - popover.set_pointing_to(Some(>k4::gdk::Rectangle::new(x as i32, y as i32, 1, 1))); - popover.set_has_arrow(false); - popover.set_position(gtk4::PositionType::Right); - popover.set_size_request(-1, n_rows * 22 + 14); - popover - }); - if let Some(ref p) = *self.active_ctx_popover.borrow() { - p.popup(); - } - } - fn handle_terminal_msg(&mut self, msg: Msg) { match msg { Msg::ToggleTerminal => {