From cf2177aac18d6e9adf1b9df20f9c83f95392eb7a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 22 Sep 2026 05:45:07 +0000 Subject: [PATCH] Dispatch editor shortcuts through ktsu.Keybinding.Core [patch] The editor wrote each chord down twice: once as an if/else if chain over ImGui's key state, and again as a literal string beside the menu item it fires ("Ctrl+Shift+S" and friends). Nothing tied the two together, so a chord could be rebound in one place and go on being advertised as the other. EditorShortcuts now holds the seven bindings once. The menu labels are rendered from the same binding the dispatcher matches, so the two cannot drift apart. The matching is ktsu.Keybinding.Core's, which Schema.Editor already referenced and did not use, so this takes on no new dependency. Chord.ToString spells a chord exactly as the menu used to hard-code it, so the menu reads as it did before. Nothing is persisted: KeybindingService is built over an in-memory CommandRegistry and ProfileManager, so no repository is involved and the editor neither reads nor writes a keybinding file. One behaviour changes deliberately. The old chain tested the modifiers each arm cared about and ignored the rest, so Ctrl+Shift+N started a new document just as Ctrl+N did, and Ctrl+Alt+O opened one. A chord now has to match exactly, so those no longer fire. The chords that were always meant to be distinguished - Ctrl+S against Ctrl+Shift+S, Ctrl+Z against Ctrl+Shift+Z - still are, and no longer depend on the order the arms happen to be written in. Covered by EditorShortcutTests, which needs no frame: it pins the label against the chord that fires the command, both ways round. The existing frame-driven ShortcutTests are unchanged and still pass, which is what says ImGui's real key state still reaches the table. Fixes #201 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JjErQAuuWQwrRBqqHTkgzZ --- Schema.Editor.Test/EditorShortcutTests.cs | 178 +++++++++++++++++++++ Schema.Editor/EditorShortcuts.cs | 186 ++++++++++++++++++++++ Schema.Editor/SchemaEditor.cs | 81 +++++----- 3 files changed, 408 insertions(+), 37 deletions(-) create mode 100644 Schema.Editor.Test/EditorShortcutTests.cs create mode 100644 Schema.Editor/EditorShortcuts.cs diff --git a/Schema.Editor.Test/EditorShortcutTests.cs b/Schema.Editor.Test/EditorShortcutTests.cs new file mode 100644 index 0000000..29dc98f --- /dev/null +++ b/Schema.Editor.Test/EditorShortcutTests.cs @@ -0,0 +1,178 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Schema.Editor.Test; + +using System; +using System.Collections.Generic; +using System.Linq; + +/// +/// The shortcut table: that a chord selects the command it is meant to, and that the menu's label +/// for a command is the chord that actually fires it. +/// +/// +/// +/// The chords used to be written twice - an if/else if chain for dispatch, and +/// literal strings such as "Ctrl+Shift+S" beside the menu items - with nothing holding the two +/// together. is the test that pins the join: +/// it reads the label the File and Edit menus now draw and checks it against the chord the +/// dispatcher matches, so rebinding a shortcut in one place and advertising the other cannot pass. +/// +/// +/// Frameless, like the rest of this suite. EditorShortcuts takes the pressed key as a name +/// rather than an ImGuiKey precisely so the table can be exercised without a rasterizer; +/// that the editor feeds it ImGui's real key state is covered by +/// Schema.Editor.UITests/ShortcutTests, which presses the chords into a live frame. +/// +/// +[TestClass] +public sealed class EditorShortcutTests +{ + private EditorShortcuts shortcuts = null!; + + [TestInitialize] + public void BuildShortcuts() => shortcuts = new EditorShortcuts(); + + /// + /// Presses a chord: the named key, with the modifiers the chord carries. + /// + private string? Press(string key, bool ctrl = true, bool shift = false, bool alt = false) => + shortcuts.FindCommand(ctrl, shift, alt, pressed => string.Equals(pressed, key, StringComparison.OrdinalIgnoreCase)); + + /// + /// Every command the menus label, against the chord the menu is expected to show for it. + /// + private static IEnumerable<(string CommandId, string Label)> LabelledCommands => + [ + (EditorShortcuts.New, "Ctrl+N"), + (EditorShortcuts.Open, "Ctrl+O"), + (EditorShortcuts.Save, "Ctrl+S"), + (EditorShortcuts.SaveAs, "Ctrl+Shift+S"), + (EditorShortcuts.Undo, "Ctrl+Z"), + (EditorShortcuts.Redo, "Ctrl+Y"), + ]; + + /// + /// The label the menu draws is the chord that fires the command, not a string that merely + /// resembles it. This is the duplication the shortcut table exists to remove. + /// + [TestMethod] + public void MenuLabelIsTheChordThatFiresTheCommand() + { + foreach ((string commandId, string label) in LabelledCommands) + { + Assert.AreEqual(label, shortcuts.Label(commandId), $"The menu label for {commandId} is not the chord bound to it."); + } + } + + /// + /// And the chord the label names is the one that actually selects that command, so the two + /// cannot drift apart in the other direction either. + /// + [TestMethod] + public void TheChordOnTheLabelSelectsThatCommand() + { + foreach ((string commandId, _) in LabelledCommands) + { + string label = shortcuts.Label(commandId); + string[] notes = label.Split('+'); + string key = notes[^1]; + + string? selected = Press( + key, + ctrl: notes.Contains("Ctrl", StringComparer.OrdinalIgnoreCase), + shift: notes.Contains("Shift", StringComparer.OrdinalIgnoreCase), + alt: notes.Contains("Alt", StringComparer.OrdinalIgnoreCase)); + + Assert.AreEqual(commandId, selected, $"Pressing {label}, the chord the menu shows for {commandId}, did not select it."); + } + } + + /// + /// Save As shares its key with Save, so the Shift has to be what tells them apart. Under the old + /// chain this held only because the Save As arm was written first. + /// + [TestMethod] + public void ShiftDistinguishesSaveAsFromSave() + { + Assert.AreEqual(EditorShortcuts.Save, Press("S")); + Assert.AreEqual(EditorShortcuts.SaveAs, Press("S", shift: true)); + } + + /// + /// Undo and Redo share Z the same way. + /// + [TestMethod] + public void ShiftDistinguishesRedoFromUndo() + { + Assert.AreEqual(EditorShortcuts.Undo, Press("Z")); + Assert.AreEqual(EditorShortcuts.RedoAlternate, Press("Z", shift: true)); + } + + /// + /// Ctrl+Y redoes as well, which is why Redo has a second binding rather than a second chord. + /// + [TestMethod] + public void CtrlYIsTheOtherRedo() + { + Assert.AreEqual(EditorShortcuts.Redo, Press("Y")); + Assert.AreEqual("Ctrl+Y", shortcuts.Label(EditorShortcuts.Redo)); + } + + /// + /// A chord has to match in every modifier, not only the ones a branch thought to test. + /// + /// + /// This is the one deliberate behaviour change. The old chain asked ctrl && + /// IsKeyPressed(N) and said nothing about Shift or Alt, so Ctrl+Shift+N and Ctrl+Alt+N both + /// started a new document. Neither is a chord the editor advertises, and neither fires now. + /// + [TestMethod] + public void AnExtraModifierIsNotTheSameChord() + { + Assert.AreEqual(EditorShortcuts.New, Press("N")); + Assert.IsNull(Press("N", shift: true), "Ctrl+Shift+N is not a chord the editor binds."); + Assert.IsNull(Press("N", alt: true), "Ctrl+Alt+N is not a chord the editor binds."); + Assert.IsNull(Press("O", shift: true), "Ctrl+Shift+O is not a chord the editor binds."); + } + + /// + /// A key with no Ctrl is ordinary typing, not a shortcut. + /// + [TestMethod] + public void AKeyWithoutItsModifierSelectsNothing() + { + Assert.IsNull(Press("N", ctrl: false)); + Assert.IsNull(Press("S", ctrl: false)); + } + + /// + /// A key no shortcut is built around never selects anything, whatever is held with it. + /// + [TestMethod] + public void AnUnboundKeySelectsNothing() + { + Assert.IsNull(Press("Q")); + Assert.IsNull(Press("Q", shift: true)); + } + + /// + /// No two commands answer to the same chord, or which one fires would come down to the order + /// they happen to be registered in - the ambiguity the else if chain had to be read + /// carefully to rule out. + /// + [TestMethod] + public void NoChordFiresTwoCommands() + { + string[] commandIds = + [ + EditorShortcuts.New, EditorShortcuts.Open, EditorShortcuts.Save, EditorShortcuts.SaveAs, + EditorShortcuts.Undo, EditorShortcuts.Redo, EditorShortcuts.RedoAlternate, + ]; + + List chords = [.. commandIds.Select(shortcuts.Label)]; + + CollectionAssert.AllItemsAreUnique(chords, $"Two commands share a chord: {string.Join(", ", chords)}"); + Assert.IsFalse(chords.Any(string.IsNullOrEmpty), "A command reached the menu with no chord bound to it."); + } +} diff --git a/Schema.Editor/EditorShortcuts.cs b/Schema.Editor/EditorShortcuts.cs new file mode 100644 index 0000000..981a097 --- /dev/null +++ b/Schema.Editor/EditorShortcuts.cs @@ -0,0 +1,186 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.Schema.Editor; + +using System; +using System.Collections.Generic; +using System.Linq; + +using ktsu.Keybinding.Core.Contracts; +using ktsu.Keybinding.Core.Models; +using ktsu.Keybinding.Core.Services; + +/// +/// The editor's keyboard shortcuts, held once so that dispatch and the menu labels cannot disagree. +/// +/// +/// +/// The chords used to be written down twice: once as an if/else if chain over ImGui's +/// key state, and again as literal strings beside the menu items ("Ctrl+Shift+S" and friends). +/// Nothing tied the two together, so a chord could be rebound in one place and go on being +/// advertised as the other. is now the only place a chord is written, and +/// renders the menu's text from the same binding that fires the command. +/// +/// +/// The matching itself is ktsu.Keybinding.Core's, which this project already referenced and +/// did not use. Its equality is insensitive to the order the modifiers are +/// written in and to case, and renders a chord in the same spelling +/// the menu used to hard-code - so adopting it leaves the menu reading exactly as it did. +/// +/// +/// Nothing here is persisted. is built over an in-memory +/// and , so no repository is involved and +/// the editor never reads or writes a keybinding file. +/// +/// +/// One behaviour did change, deliberately. The old chain tested the modifiers it cared about and +/// ignored the rest, so Ctrl+Shift+N started a new document just as Ctrl+N did, and Ctrl+Alt+O +/// opened one. A chord now has to match exactly, so those no longer fire. The chords that were +/// always meant to be distinguished - Ctrl+S against Ctrl+Shift+S, Ctrl+Z against Ctrl+Shift+Z - +/// still are, and no longer depend on the order the else if arms happen to be written in. +/// +/// +internal sealed class EditorShortcuts +{ + /// Start a new document. + internal const string New = "editor.file.new"; + + /// Open an existing document. + internal const string Open = "editor.file.open"; + + /// Write the open document to its current path. + internal const string Save = "editor.file.save"; + + /// Ask where to write a copy of the open document. + internal const string SaveAs = "editor.file.save-as"; + + /// Undo the last edit. + internal const string Undo = "editor.edit.undo"; + + /// Redo the last undone edit. + internal const string Redo = "editor.edit.redo"; + + /// + /// Redo, under the other chord that has always done it. Kept as its own command because a + /// binding holds one chord, and Ctrl+Y is the one the Edit menu advertises. + /// + internal const string RedoAlternate = "editor.edit.redo-alternate"; + + private const string ProfileId = "default"; + + private static readonly string[] ModifierNotes = ["CTRL", "SHIFT", "ALT", "META"]; + + /// + /// Every shortcut the editor has, and the only place each chord is written. + /// + private static readonly (string CommandId, string Name, string Chord)[] Bindings = + [ + (New, "New", "Ctrl+N"), + (Open, "Open", "Ctrl+O"), + (Save, "Save", "Ctrl+S"), + (SaveAs, "Save As", "Ctrl+Shift+S"), + (Undo, "Undo", "Ctrl+Z"), + (Redo, "Redo", "Ctrl+Y"), + (RedoAlternate, "Redo", "Ctrl+Shift+Z"), + ]; + + private readonly KeybindingService keybindings; + + /// + /// The keys a chord can be built around, each named as ktsu.Keybinding.Core spells it. + /// Derived from so that adding a shortcut needs no second edit here. + /// + private readonly string[] primaryKeys; + + /// + /// Registers the editor's commands and binds each to its chord. + /// + internal EditorShortcuts() + { + CommandRegistry commands = new(); + ProfileManager profiles = new(); + keybindings = new KeybindingService(commands, profiles); + + keybindings.CreateProfile(ProfileId, "Default", "The editor's built-in shortcuts"); + keybindings.SetActiveProfile(ProfileId); + + foreach ((string commandId, string name, string chord) in Bindings) + { + commands.RegisterCommand(new Command(commandId, name, name, "Editor")); + keybindings.BindChord(commandId, Chord.Parse(chord)); + } + + primaryKeys = [.. Bindings + .Select(binding => Chord.Parse(binding.Chord)) + .SelectMany(chord => chord.Notes) + .Select(note => note.ToString()) + .Where(note => !ModifierNotes.Contains(note, StringComparer.OrdinalIgnoreCase)) + .Distinct(StringComparer.OrdinalIgnoreCase)]; + } + + /// + /// The text the menu shows beside the item that names. + /// + /// The command whose chord to render. + /// The chord's spelling, or an empty string if the command has no chord bound. + internal string Label(string commandId) => keybindings.GetChord(commandId)?.ToString() ?? string.Empty; + + /// + /// The command whose chord is being pressed, if any. + /// + /// Whether Control is held. + /// Whether Shift is held. + /// Whether Alt is held. + /// + /// Answers whether the named key was pressed on this frame. Named rather than typed as an ImGui + /// key so that this can be exercised without a frame to press keys into. + /// + /// The command's id, or if no bound chord matches. + /// + /// The modifiers are taken as given and the pressed key is looked for among the keys the + /// bindings actually use, so a chord is only ever built for a key some shortcut wants. Matching + /// the result is left to , which is + /// what makes the match exact in the modifiers rather than merely in the ones a branch thought + /// to test. + /// + internal string? FindCommand(bool ctrl, bool shift, bool alt, Func isKeyPressed) + { + Ensure.NotNull(isKeyPressed); + + foreach (string key in primaryKeys) + { + if (!isKeyPressed(key)) + { + continue; + } + + List notes = []; + + if (ctrl) + { + notes.Add(new Note("CTRL")); + } + + if (shift) + { + notes.Add(new Note("SHIFT")); + } + + if (alt) + { + notes.Add(new Note("ALT")); + } + + notes.Add(new Note(key)); + + string? commandId = keybindings.FindCommandByChord(new Chord(notes)); + + if (commandId is not null) + { + return commandId; + } + } + + return null; + } +} diff --git a/Schema.Editor/SchemaEditor.cs b/Schema.Editor/SchemaEditor.cs index a634ebe..78610eb 100644 --- a/Schema.Editor/SchemaEditor.cs +++ b/Schema.Editor/SchemaEditor.cs @@ -37,6 +37,12 @@ public partial class SchemaEditor private ImGuiWidgets.DividerContainer DividerContainerCols { get; init; } internal IUndoRedoService UndoRedo { get; } + + /// + /// The keyboard shortcuts, read both to dispatch a chord and to label the menu items it fires. + /// + internal EditorShortcuts Shortcuts { get; } = new(); + internal Popups Popups { get; } private TreeSchema TreeSchema { get; init; } private CodeGeneratorPanel CodeGeneratorPanel { get; init; } @@ -192,42 +198,43 @@ private void ProcessKeyboardShortcuts() return; } - bool ctrl = io.KeyCtrl; - bool shift = io.KeyShift; - - if (ctrl && ImGui.IsKeyPressed(ImGuiKey.Z, false)) + switch (Shortcuts.FindCommand(io.KeyCtrl, io.KeyShift, io.KeyAlt, WasKeyPressed)) { - if (shift) - { - Redo(); - } - else - { + case EditorShortcuts.New: + New(); + break; + case EditorShortcuts.Open: + Open(); + break; + case EditorShortcuts.Save: + Save(); + break; + case EditorShortcuts.SaveAs: + SaveAs(); + break; + case EditorShortcuts.Undo: Undo(); - } - } - else if (ctrl && ImGui.IsKeyPressed(ImGuiKey.Y, false)) - { - Redo(); - } - else if (ctrl && shift && ImGui.IsKeyPressed(ImGuiKey.S, false)) - { - SaveAs(); - } - else if (ctrl && ImGui.IsKeyPressed(ImGuiKey.S, false)) - { - Save(); - } - else if (ctrl && ImGui.IsKeyPressed(ImGuiKey.N, false)) - { - New(); - } - else if (ctrl && ImGui.IsKeyPressed(ImGuiKey.O, false)) - { - Open(); + break; + case EditorShortcuts.Redo: + case EditorShortcuts.RedoAlternate: + Redo(); + break; + default: + break; } } + /// + /// Whether the key ktsu.Keybinding.Core names was pressed on this frame. + /// + /// + /// The library spells the keys a chord is built from ("N", "S") exactly as ImGui names them in + /// , so the two meet at a parse. A key the enum does not know counts as + /// not pressed, rather than throwing on every frame. + /// + private static bool WasKeyPressed(string key) => + Enum.TryParse(key, ignoreCase: true, out ImGuiKey imGuiKey) && ImGui.IsKeyPressed(imGuiKey, false); + internal void OnRender(float dt) { // Stashed for the parameterless tab content delegates (the Class Graph needs the frame delta). @@ -328,12 +335,12 @@ private void ShowFileMenu() return; } - if (MenuItem("New", "Ctrl+N")) + if (MenuItem("New", Shortcuts.Label(EditorShortcuts.New))) { New(); } - if (MenuItem("Open", "Ctrl+O")) + if (MenuItem("Open", Shortcuts.Label(EditorShortcuts.Open))) { Open(); } @@ -342,14 +349,14 @@ private void ShowFileMenu() ImGui.Separator(); - if (MenuItem("Save", "Ctrl+S", CurrentSchema is not null)) + if (MenuItem("Save", Shortcuts.Label(EditorShortcuts.Save), CurrentSchema is not null)) { Save(); } // Always available while a schema is open: without it there is no way to save a copy // somewhere else once the schema has a path. - if (MenuItem("Save As...", "Ctrl+Shift+S", CurrentSchema is not null)) + if (MenuItem("Save As...", Shortcuts.Label(EditorShortcuts.SaveAs), CurrentSchema is not null)) { SaveAs(); } @@ -383,12 +390,12 @@ private void ShowEditMenu() return; } - if (MenuItem("Undo", "Ctrl+Z", UndoRedo.CanUndo)) + if (MenuItem("Undo", Shortcuts.Label(EditorShortcuts.Undo), UndoRedo.CanUndo)) { Undo(); } - if (MenuItem("Redo", "Ctrl+Y", UndoRedo.CanRedo)) + if (MenuItem("Redo", Shortcuts.Label(EditorShortcuts.Redo), UndoRedo.CanRedo)) { Redo(); }