From abe68ef2f263f758191ddac1d4fd49d2ba8d50bc Mon Sep 17 00:00:00 2001 From: LagoLunatic Date: Mon, 2 Dec 2024 23:51:37 -0500 Subject: [PATCH] objdiff-gui: Implement keyboard shortcuts (#139) * Fix missing dependency feature for objdiff-gui * Update .gitignore * Add enter and back hotkeys * Add scroll hotkeys * Add hotkeys to select the next symbol above/below the current one in the listing * Do not clear highlighted symbol when backing out of diff view * Do not clear highlighted symbol when hovering mouse over an unpaired symbol * Auto-scroll the keyboard-selected symbols into view if offscreen * Fix some hotkeys stealing input from focused widgets e.g. The symbol list was stealing the W/S key presses when typing into the symbol filter text edit. If the user actually wants to use these shortcuts while a widget is focused, they can simply press the escape key to unfocus all widgets and then press the shortcut. * Add Ctrl+F/S shortcuts for focusing the object and symbol filter text edits * Add space as alternative to enter hotkey This is for consistency with egui's builtint enter/space hotkey for interacting with the focused widget. * Add hotkeys to change target and base functions * Split function diff view: Enable PageUp/PageDown/Home/End for scrolling * Add escape as an alternative to back hotkey * Fix auto-scrolling to highlighted symbol only working for the left side The flag is cleared after one scroll to avoid doing it continuously, but this breaks when we need to scroll to both the left and the right symbol at the same time. So now each side has its own flag to keep track of this state independently. * Simplify clearing of the autoscroll flag, remove &mut State * Found a better place to clear the autoscroll flag DiffViewState::post_update is where the flag gets set, so clearing it right before that at the start of the function seems to make the most sense, instead of doing it in App::update. --- objdiff-gui/src/hotkeys.rs | 108 ++++++++++++++++++++++++ objdiff-gui/src/main.rs | 1 + objdiff-gui/src/views/config.rs | 7 +- objdiff-gui/src/views/data_diff.rs | 17 ++-- objdiff-gui/src/views/extab_diff.rs | 21 +++-- objdiff-gui/src/views/function_diff.rs | 30 +++---- objdiff-gui/src/views/symbol_diff.rs | 111 ++++++++++++++++++++----- 7 files changed, 244 insertions(+), 51 deletions(-) create mode 100644 objdiff-gui/src/hotkeys.rs diff --git a/objdiff-gui/src/hotkeys.rs b/objdiff-gui/src/hotkeys.rs new file mode 100644 index 0000000..e68571b --- /dev/null +++ b/objdiff-gui/src/hotkeys.rs @@ -0,0 +1,108 @@ +use egui::{ + style::ScrollAnimation, vec2, Context, Key, KeyboardShortcut, Modifiers, PointerButton, +}; + +fn any_widget_focused(ctx: &Context) -> bool { ctx.memory(|mem| mem.focused().is_some()) } + +pub fn enter_pressed(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| { + i.key_pressed(Key::Enter) + || i.key_pressed(Key::Space) + || i.pointer.button_pressed(PointerButton::Extra2) + }) +} + +pub fn back_pressed(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| { + i.key_pressed(Key::Backspace) + || i.key_pressed(Key::Escape) + || i.pointer.button_pressed(PointerButton::Extra1) + }) +} + +pub fn up_pressed(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| i.key_pressed(Key::ArrowUp) || i.key_pressed(Key::W)) +} + +pub fn down_pressed(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| i.key_pressed(Key::ArrowDown) || i.key_pressed(Key::S)) +} + +pub fn page_up_pressed(ctx: &Context) -> bool { ctx.input_mut(|i| i.key_pressed(Key::PageUp)) } + +pub fn page_down_pressed(ctx: &Context) -> bool { ctx.input_mut(|i| i.key_pressed(Key::PageDown)) } + +pub fn home_pressed(ctx: &Context) -> bool { ctx.input_mut(|i| i.key_pressed(Key::Home)) } + +pub fn end_pressed(ctx: &Context) -> bool { ctx.input_mut(|i| i.key_pressed(Key::End)) } + +pub fn check_scroll_hotkeys(ui: &mut egui::Ui, include_small_increments: bool) { + let ui_height = ui.available_rect_before_wrap().height(); + if up_pressed(ui.ctx()) && include_small_increments { + ui.scroll_with_delta_animation(vec2(0.0, ui_height / 10.0), ScrollAnimation::none()); + } else if down_pressed(ui.ctx()) && include_small_increments { + ui.scroll_with_delta_animation(vec2(0.0, -ui_height / 10.0), ScrollAnimation::none()); + } else if page_up_pressed(ui.ctx()) { + ui.scroll_with_delta_animation(vec2(0.0, ui_height), ScrollAnimation::none()); + } else if page_down_pressed(ui.ctx()) { + ui.scroll_with_delta_animation(vec2(0.0, -ui_height), ScrollAnimation::none()); + } else if home_pressed(ui.ctx()) { + ui.scroll_with_delta_animation(vec2(0.0, f32::INFINITY), ScrollAnimation::none()); + } else if end_pressed(ui.ctx()) { + ui.scroll_with_delta_animation(vec2(0.0, -f32::INFINITY), ScrollAnimation::none()); + } +} + +pub fn consume_up_key(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| { + i.consume_key(Modifiers::NONE, Key::ArrowUp) || i.consume_key(Modifiers::NONE, Key::W) + }) +} + +pub fn consume_down_key(ctx: &Context) -> bool { + if any_widget_focused(ctx) { + return false; + } + ctx.input_mut(|i| { + i.consume_key(Modifiers::NONE, Key::ArrowDown) || i.consume_key(Modifiers::NONE, Key::S) + }) +} + +const OBJECT_FILTER_SHORTCUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::CTRL, Key::F); + +pub fn consume_object_filter_shortcut(ctx: &Context) -> bool { + ctx.input_mut(|i| i.consume_shortcut(&OBJECT_FILTER_SHORTCUT)) +} + +const SYMBOL_FILTER_SHORTCUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::CTRL, Key::S); + +pub fn consume_symbol_filter_shortcut(ctx: &Context) -> bool { + ctx.input_mut(|i| i.consume_shortcut(&SYMBOL_FILTER_SHORTCUT)) +} + +const CHANGE_TARGET_SHORTCUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::CTRL, Key::T); + +pub fn consume_change_target_shortcut(ctx: &Context) -> bool { + ctx.input_mut(|i| i.consume_shortcut(&CHANGE_TARGET_SHORTCUT)) +} + +const CHANGE_BASE_SHORTCUT: KeyboardShortcut = KeyboardShortcut::new(Modifiers::CTRL, Key::B); + +pub fn consume_change_base_shortcut(ctx: &Context) -> bool { + ctx.input_mut(|i| i.consume_shortcut(&CHANGE_BASE_SHORTCUT)) +} diff --git a/objdiff-gui/src/main.rs b/objdiff-gui/src/main.rs index 18e6b7f..0974a00 100644 --- a/objdiff-gui/src/main.rs +++ b/objdiff-gui/src/main.rs @@ -4,6 +4,7 @@ mod app; mod app_config; mod config; mod fonts; +mod hotkeys; mod jobs; mod update; mod views; diff --git a/objdiff-gui/src/views/config.rs b/objdiff-gui/src/views/config.rs index 6d7fd9e..da382de 100644 --- a/objdiff-gui/src/views/config.rs +++ b/objdiff-gui/src/views/config.rs @@ -21,6 +21,7 @@ use strum::{EnumMessage, VariantArray}; use crate::{ app::{AppConfig, AppState, AppStateRef, ObjectConfig}, config::ProjectObjectNode, + hotkeys, jobs::{ check_update::{start_check_update, CheckUpdateResult}, update::start_update, @@ -254,7 +255,11 @@ pub fn config_ui( } } else { let had_search = !config_state.object_search.is_empty(); - egui::TextEdit::singleline(&mut config_state.object_search).hint_text("Filter").ui(ui); + let response = + egui::TextEdit::singleline(&mut config_state.object_search).hint_text("Filter").ui(ui); + if hotkeys::consume_object_filter_shortcut(ui.ctx()) { + response.request_focus(); + } let mut root_open = None; let mut node_open = NodeOpen::Default; diff --git a/objdiff-gui/src/views/data_diff.rs b/objdiff-gui/src/views/data_diff.rs index 62379f0..37f2a6c 100644 --- a/objdiff-gui/src/views/data_diff.rs +++ b/objdiff-gui/src/views/data_diff.rs @@ -7,11 +7,14 @@ use objdiff_core::{ }; use time::format_description; -use crate::views::{ - appearance::Appearance, - column_layout::{render_header, render_table}, - symbol_diff::{DiffViewAction, DiffViewNavigation, DiffViewState}, - write_text, +use crate::{ + hotkeys, + views::{ + appearance::Appearance, + column_layout::{render_header, render_table}, + symbol_diff::{DiffViewAction, DiffViewNavigation, DiffViewState}, + write_text, + }, }; const BYTES_PER_ROW: usize = 16; @@ -176,6 +179,8 @@ fn data_table_ui( let left_diffs = left_section.map(|(_, section)| split_diffs(§ion.data_diff)); let right_diffs = right_section.map(|(_, section)| split_diffs(§ion.data_diff)); + hotkeys::check_scroll_hotkeys(ui, true); + render_table(ui, available_width, 2, config.code_font.size, total_rows, |row, column| { let i = row.index(); let address = i * BYTES_PER_ROW; @@ -224,7 +229,7 @@ pub fn data_diff_ui( render_header(ui, available_width, 2, |ui, column| { if column == 0 { // Left column - if ui.button("⏴ Back").clicked() { + if ui.button("⏴ Back").clicked() || hotkeys::back_pressed(ui.ctx()) { ret = Some(DiffViewAction::Navigate(DiffViewNavigation::symbol_diff())); } diff --git a/objdiff-gui/src/views/extab_diff.rs b/objdiff-gui/src/views/extab_diff.rs index b0bbada..5d99117 100644 --- a/objdiff-gui/src/views/extab_diff.rs +++ b/objdiff-gui/src/views/extab_diff.rs @@ -5,13 +5,16 @@ use objdiff_core::{ }; use time::format_description; -use crate::views::{ - appearance::Appearance, - column_layout::{render_header, render_strips}, - function_diff::FunctionDiffContext, - symbol_diff::{ - match_color_for_symbol, DiffViewAction, DiffViewNavigation, DiffViewState, SymbolRefByName, - View, +use crate::{ + hotkeys, + views::{ + appearance::Appearance, + column_layout::{render_header, render_strips}, + function_diff::FunctionDiffContext, + symbol_diff::{ + match_color_for_symbol, DiffViewAction, DiffViewNavigation, DiffViewState, + SymbolRefByName, View, + }, }, }; @@ -136,7 +139,7 @@ pub fn extab_diff_ui( if column == 0 { // Left column ui.horizontal(|ui| { - if ui.button("⏴ Back").clicked() { + if ui.button("⏴ Back").clicked() || hotkeys::back_pressed(ui.ctx()) { ret = Some(DiffViewAction::Navigate(DiffViewNavigation::symbol_diff())); } ui.separator(); @@ -232,6 +235,8 @@ pub fn extab_diff_ui( } }); + hotkeys::check_scroll_hotkeys(ui, true); + // Table render_strips(ui, available_width, 2, |ui, column| { if column == 0 { diff --git a/objdiff-gui/src/views/function_diff.rs b/objdiff-gui/src/views/function_diff.rs index a412649..081a0ab 100644 --- a/objdiff-gui/src/views/function_diff.rs +++ b/objdiff-gui/src/views/function_diff.rs @@ -14,12 +14,15 @@ use objdiff_core::{ }; use time::format_description; -use crate::views::{ - appearance::Appearance, - column_layout::{render_header, render_strips, render_table}, - symbol_diff::{ - match_color_for_symbol, symbol_list_ui, DiffViewAction, DiffViewNavigation, DiffViewState, - SymbolDiffContext, SymbolFilter, SymbolRefByName, SymbolViewState, View, +use crate::{ + hotkeys, + views::{ + appearance::Appearance, + column_layout::{render_header, render_strips, render_table}, + symbol_diff::{ + match_color_for_symbol, symbol_list_ui, DiffViewAction, DiffViewNavigation, + DiffViewState, SymbolDiffContext, SymbolFilter, SymbolRefByName, SymbolViewState, View, + }, }, }; @@ -436,6 +439,7 @@ fn asm_table_ui( }; if left_len.is_some() && right_len.is_some() { // Joint view + hotkeys::check_scroll_hotkeys(ui, true); render_table( ui, available_width, @@ -471,6 +475,7 @@ fn asm_table_ui( if column == 0 { if let Some(ctx) = left_ctx { if ctx.has_symbol() { + hotkeys::check_scroll_hotkeys(ui, false); render_table( ui, available_width / 2.0, @@ -516,9 +521,6 @@ fn asm_table_ui( SymbolRefByName::new(right_symbol, right_section), )); } - DiffViewAction::SetSymbolHighlight(_, _) => { - // Ignore - } _ => { ret = Some(action); } @@ -531,6 +533,7 @@ fn asm_table_ui( } else if column == 1 { if let Some(ctx) = right_ctx { if ctx.has_symbol() { + hotkeys::check_scroll_hotkeys(ui, false); render_table( ui, available_width / 2.0, @@ -576,9 +579,6 @@ fn asm_table_ui( right_symbol_ref, )); } - DiffViewAction::SetSymbolHighlight(_, _) => { - // Ignore - } _ => { ret = Some(action); } @@ -679,7 +679,7 @@ pub fn function_diff_ui( if column == 0 { // Left column ui.horizontal(|ui| { - if ui.button("⏴ Back").clicked() { + if ui.button("⏴ Back").clicked() || hotkeys::back_pressed(ui.ctx()) { ret = Some(DiffViewAction::Navigate(DiffViewNavigation::symbol_diff())); } ui.separator(); @@ -712,10 +712,11 @@ pub fn function_diff_ui( .color(appearance.highlight_color), ); if right_ctx.is_some_and(|m| m.has_symbol()) - && ui + && (ui .button("Change target") .on_hover_text_at_pointer("Choose a different symbol to use as the target") .clicked() + || hotkeys::consume_change_target_shortcut(ui.ctx())) { if let Some(symbol_ref) = state.symbol_state.right_symbol.as_ref() { ret = Some(DiffViewAction::SelectingLeft(symbol_ref.clone())); @@ -789,6 +790,7 @@ pub fn function_diff_ui( "Choose a different symbol to use as the base", ) .clicked() + || hotkeys::consume_change_base_shortcut(ui.ctx()) { if let Some(symbol_ref) = state.symbol_state.left_symbol.as_ref() { ret = Some(DiffViewAction::SelectingRight(symbol_ref.clone())); diff --git a/objdiff-gui/src/views/symbol_diff.rs b/objdiff-gui/src/views/symbol_diff.rs index 1ca66dd..0d008c3 100644 --- a/objdiff-gui/src/views/symbol_diff.rs +++ b/objdiff-gui/src/views/symbol_diff.rs @@ -1,8 +1,8 @@ -use std::{collections::BTreeMap, mem::take}; +use std::{collections::BTreeMap, mem::take, ops::Bound}; use egui::{ - text::LayoutJob, CollapsingHeader, Color32, Id, OpenUrl, ScrollArea, SelectableLabel, TextEdit, - Ui, Widget, + style::ScrollAnimation, text::LayoutJob, CollapsingHeader, Color32, Id, OpenUrl, ScrollArea, + SelectableLabel, TextEdit, Ui, Widget, }; use objdiff_core::{ arch::ObjArch, @@ -15,6 +15,7 @@ use regex::{Regex, RegexBuilder}; use crate::{ app::AppStateRef, + hotkeys, jobs::{ create_scratch::{start_create_scratch, CreateScratchConfig, CreateScratchResult}, objdiff::{BuildStatus, ObjDiffResult}, @@ -56,8 +57,8 @@ pub enum DiffViewAction { Build, /// Navigate to a new diff view Navigate(DiffViewNavigation), - /// Set the highlighted symbols in the symbols view - SetSymbolHighlight(Option, Option), + /// Set the highlighted symbols in the symbols view, optionally scrolling them into view. + SetSymbolHighlight(Option, Option, bool), /// Set the symbols view search filter SetSearch(String), /// Submit the current function to decomp.me @@ -135,6 +136,7 @@ pub struct DiffViewState { #[derive(Default)] pub struct SymbolViewState { pub highlighted_symbol: (Option, Option), + pub autoscroll_to_highlighted_symbols: bool, pub left_symbol: Option, pub right_symbol: Option, pub reverse_fn_order: bool, @@ -197,6 +199,9 @@ impl DiffViewState { ctx.output_mut(|o| o.open_url = Some(OpenUrl::new_tab(result.scratch_url))); } + // Clear the autoscroll flag so that it doesn't scroll continuously. + self.symbol_state.autoscroll_to_highlighted_symbols = false; + let Some(action) = action else { return; }; @@ -211,7 +216,6 @@ impl DiffViewState { // Ignore action if we're already navigating return; } - self.symbol_state.highlighted_symbol = (None, None); let Ok(mut state) = state.write() else { return; }; @@ -247,8 +251,9 @@ impl DiffViewState { self.post_build_nav = Some(nav); } } - DiffViewAction::SetSymbolHighlight(left, right) => { + DiffViewAction::SetSymbolHighlight(left, right, autoscroll) => { self.symbol_state.highlighted_symbol = (left, right); + self.symbol_state.autoscroll_to_highlighted_symbols = autoscroll; } DiffViewAction::SetSearch(search) => { self.search_regex = if search.is_empty() { @@ -534,7 +539,15 @@ fn symbol_ui( ret = Some(DiffViewAction::Navigate(result)); } }); - if response.clicked() { + if selected && state.autoscroll_to_highlighted_symbols { + // Automatically scroll the view to encompass the selected symbol in case the user selected + // an offscreen symbol by using a keyboard shortcut. + ui.scroll_to_rect_animation(response.rect, None, ScrollAnimation::none()); + // This autoscroll state flag will be reset in DiffViewState::post_update at the end of + // every frame so that we don't continuously scroll the view back when the user is trying to + // manually scroll away. + } + if response.clicked() || (selected && hotkeys::enter_pressed(ui.ctx())) { if let Some(section) = section { match section.kind { ObjSectionKind::Code => { @@ -561,20 +574,18 @@ fn symbol_ui( } } } else if response.hovered() { - ret = Some(if let Some(target_symbol) = symbol_diff.target_symbol { - if column == 0 { - DiffViewAction::SetSymbolHighlight( - Some(symbol_diff.symbol_ref), - Some(target_symbol), - ) - } else { - DiffViewAction::SetSymbolHighlight( - Some(target_symbol), - Some(symbol_diff.symbol_ref), - ) - } + ret = Some(if column == 0 { + DiffViewAction::SetSymbolHighlight( + Some(symbol_diff.symbol_ref), + symbol_diff.target_symbol, + false, + ) } else { - DiffViewAction::SetSymbolHighlight(None, None) + DiffViewAction::SetSymbolHighlight( + symbol_diff.target_symbol, + Some(symbol_diff.symbol_ref), + false, + ) }); } ret @@ -648,6 +659,58 @@ pub fn symbol_list_ui( } } + hotkeys::check_scroll_hotkeys(ui, false); + + let mut new_key_value_to_highlight = None; + if let Some(sym_ref) = + if column == 0 { state.highlighted_symbol.0 } else { state.highlighted_symbol.1 } + { + let up = if hotkeys::consume_up_key(ui.ctx()) { + Some(true) + } else if hotkeys::consume_down_key(ui.ctx()) { + Some(false) + } else { + None + }; + if let Some(mut up) = up { + if state.reverse_fn_order { + up = !up; + } + new_key_value_to_highlight = if up { + mapping.range(..sym_ref).next_back() + } else { + mapping.range((Bound::Excluded(sym_ref), Bound::Unbounded)).next() + }; + }; + } else { + // No symbol is highlighted in this column. Select the topmost symbol instead. + // Note that we intentionally do not consume the up/down key presses in this case, but + // we do when a symbol is highlighted. This is so that if only one column has a symbol + // highlighted, that one takes precedence over the one with nothing highlighted. + if hotkeys::up_pressed(ui.ctx()) || hotkeys::down_pressed(ui.ctx()) { + new_key_value_to_highlight = if state.reverse_fn_order { + mapping.last_key_value() + } else { + mapping.first_key_value() + }; + } + } + if let Some((new_sym_ref, new_symbol_diff)) = new_key_value_to_highlight { + ret = Some(if column == 0 { + DiffViewAction::SetSymbolHighlight( + Some(*new_sym_ref), + new_symbol_diff.target_symbol, + true, + ) + } else { + DiffViewAction::SetSymbolHighlight( + new_symbol_diff.target_symbol, + Some(*new_sym_ref), + true, + ) + }); + } + ui.scope(|ui| { ui.style_mut().override_text_style = Some(egui::TextStyle::Monospace); ui.style_mut().wrap_mode = Some(egui::TextWrapMode::Extend); @@ -838,7 +901,11 @@ pub fn symbol_diff_ui( }); let mut search = state.search.clone(); - if TextEdit::singleline(&mut search).hint_text("Filter symbols").ui(ui).changed() { + let response = TextEdit::singleline(&mut search).hint_text("Filter symbols").ui(ui); + if hotkeys::consume_symbol_filter_shortcut(ui.ctx()) { + response.request_focus(); + } + if response.changed() { ret = Some(DiffViewAction::SetSearch(search)); } } else if column == 1 {