From 84b271947a205f2ada39fc596e7784f11c90b79a Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 14 Oct 2024 12:43:14 -0600 Subject: [PATCH] Fixed InvokingKeyBindings event semantics --- .../View/Orientation/OrientationHelper.cs | 6 +- Terminal.Gui/View/View.Keyboard.cs | 89 ++++++++----------- UnitTests/View/Keyboard/KeyboardEventTests.cs | 6 +- docfx/docs/events.md | 84 ++++++++--------- 4 files changed, 85 insertions(+), 100 deletions(-) diff --git a/Terminal.Gui/View/Orientation/OrientationHelper.cs b/Terminal.Gui/View/Orientation/OrientationHelper.cs index 33c33c70f..380dfbb53 100644 --- a/Terminal.Gui/View/Orientation/OrientationHelper.cs +++ b/Terminal.Gui/View/Orientation/OrientationHelper.cs @@ -69,7 +69,7 @@ public class OrientationHelper return; } - // Best practice is to invoke the virtual method first. + // Best practice is to call the virtual method first. // This allows derived classes to handle the event and potentially cancel it. if (_owner?.OnOrientationChanging (value, _orientation) ?? false) { @@ -98,10 +98,8 @@ public class OrientationHelper } } - // Best practice is to invoke the virtual method first. + // Best practice is to call the virtual method first, then raise the event. _owner?.OnOrientationChanged (_orientation); - - // Even though Changed is not cancelable, it is still a good practice to raise the event after. OrientationChanged?.Invoke (_owner, new (in _orientation)); } } diff --git a/Terminal.Gui/View/View.Keyboard.cs b/Terminal.Gui/View/View.Keyboard.cs index cf7f9cdf1..e3b2ab2ba 100644 --- a/Terminal.Gui/View/View.Keyboard.cs +++ b/Terminal.Gui/View/View.Keyboard.cs @@ -219,7 +219,7 @@ public partial class View // Keyboard APIs private void SetHotKeyFromTitle () { - if (TitleTextFormatter == null || HotKeySpecifier == new Rune ('\xFFFF')) + if (HotKeySpecifier == new Rune ('\xFFFF')) { return; // throw new InvalidOperationException ("Can't set HotKey unless a TextFormatter has been created"); } @@ -264,7 +264,7 @@ public partial class View // Keyboard APIs /// process the key press. /// /// - /// Callling this method for a key bound to the view via an Application-scoped keybinding will have no effect. + /// Calling this method for a key bound to the view via an Application-scoped keybinding will have no effect. /// Instead, /// use . /// @@ -294,8 +294,6 @@ public partial class View // Keyboard APIs // During (this is what can be cancelled) // TODO: NewKeyDownEvent returns bool. It should be bool? so state of RaiseInvokingKeyBindingsAndInvokeCommands can be reflected up stack - - if (RaiseInvokingKeyBindingsAndInvokeCommands (key) is true || key.Handled) { return true; @@ -310,28 +308,28 @@ public partial class View // Keyboard APIs return key.Handled; - bool RaiseKeyDown (Key key) + bool RaiseKeyDown (Key k) { // Before (fire the cancellable event) - if (OnKeyDown (key) || key.Handled) + if (OnKeyDown (k) || k.Handled) { return true; } // fire event - KeyDown?.Invoke (this, key); + KeyDown?.Invoke (this, k); - return key.Handled; + return k.Handled; } - bool RaiseProcessKeyDown (Key key) + bool RaiseProcessKeyDown (Key k) { - if (OnProcessKeyDown (key) || key.Handled) + if (OnProcessKeyDown (k) || k.Handled) { return true; } - ProcessKeyDown?.Invoke (this, key); + ProcessKeyDown?.Invoke (this, k); return false; } @@ -343,7 +341,7 @@ public partial class View // Keyboard APIs /// to true to /// stop the key from being processed by other views. /// - /// Contains the details about the key that produced the event. + /// Contains the details about the key that produced the event. /// /// if the key press was not handled. if the keypress was handled /// and no other view should see it. @@ -355,7 +353,7 @@ public partial class View // Keyboard APIs /// /// Fires the event. /// - protected virtual bool OnKeyDown (Key keyEvent) { return false; } + protected virtual bool OnKeyDown (Key key) { return false; } /// /// Raised when the user presses a key, allowing subscribers to pre-process the key down event. Raised @@ -385,12 +383,12 @@ public partial class View // Keyboard APIs /// KeyUp events. /// /// - /// Contains the details about the key that produced the event. + /// Contains the details about the key that produced the event. /// /// if the key press was not handled. if the keypress was handled /// and no other view should see it. /// - protected virtual bool OnProcessKeyDown (Key keyEvent) { return keyEvent.Handled; } + protected virtual bool OnProcessKeyDown (Key key) { return key.Handled; } /// /// Raised when the user presses a key, allowing subscribers to do things during key down events. Set @@ -456,25 +454,25 @@ public partial class View // Keyboard APIs return false; - bool RaiseKeyUp (Key key) + bool RaiseKeyUp (Key k) { // Before (fire the cancellable event) - if (OnKeyUp (key) || key.Handled) + if (OnKeyUp (k) || k.Handled) { return true; } // fire event - KeyUp?.Invoke (this, key); + KeyUp?.Invoke (this, k); - return key.Handled; + return k.Handled; } } /// Method invoked when a key is released. This method is called from . - /// Contains the details about the key that produced the event. + /// Contains the details about the key that produced the event. /// - /// if the key stroke was not handled. if no other view should see + /// if the keys up event was not handled. if no other view should see /// it. /// /// @@ -486,7 +484,7 @@ public partial class View // Keyboard APIs /// /// See for an overview of Terminal.Gui keyboard APIs. /// - public virtual bool OnKeyUp (Key keyEvent) { return false; } + public virtual bool OnKeyUp (Key key) { return false; } /// /// Invoked when a key is released. Set to true to stop the key up event from being processed @@ -514,7 +512,6 @@ public partial class View // Keyboard APIs /// /// /// - /// /// /// if no command was invoked or there was no matching key binding; input processing should continue. /// if a command was invoked and was not handled (or cancelled); input processing should @@ -523,25 +520,18 @@ public partial class View // Keyboard APIs /// internal bool? RaiseInvokingKeyBindingsAndInvokeCommands (Key key) { - //// Before - //// fire event only if there's a hotkey binding for the key - //if (!KeyBindings.TryGet (key, KeyBindingScope.Focused | KeyBindingScope.HotKey, out KeyBinding kb)) - //{ - // return null; - //} - KeyBindingScope scope = KeyBindingScope.Focused | KeyBindingScope.HotKey; // During - // BUGBUG: The proper pattern is for the v-method (OnInvokingKeyBindings) to be called first, then the event - InvokingKeyBindings?.Invoke (this, key); - - if (key.Handled) + if (OnInvokingKeyBindings (key, scope)) { return true; } - if (OnInvokingKeyBindings (key, scope)) + // BUGBUG: The proper pattern is for the v-method (OnInvokingKeyBindings) to be called first, then the event + InvokingKeyBindings?.Invoke (this, key); + + if (key.Handled) { return true; } @@ -589,9 +579,9 @@ public partial class View // Keyboard APIs } - private bool ProcessAdornmentKeyBindings (Adornment adornment, Key keyEvent, KeyBindingScope scope, ref bool? handled) + private bool ProcessAdornmentKeyBindings (Adornment adornment, Key key, KeyBindingScope scope, ref bool? handled) { - bool? adornmentHandled = adornment.OnInvokingKeyBindings (keyEvent, scope); + bool? adornmentHandled = adornment.OnInvokingKeyBindings (key, scope); if (adornmentHandled is true) { @@ -605,7 +595,7 @@ public partial class View // Keyboard APIs foreach (View subview in adornment.Subviews) { - bool? subViewHandled = subview.OnInvokingKeyBindings (keyEvent, scope); + bool? subViewHandled = subview.OnInvokingKeyBindings (key, scope); if (subViewHandled is { }) { @@ -621,7 +611,7 @@ public partial class View // Keyboard APIs return false; } - private bool ProcessSubViewKeyBindings (Key keyEvent, KeyBindingScope scope, ref bool? handled, bool invoke = true) + private bool ProcessSubViewKeyBindings (Key key, KeyBindingScope scope, ref bool? handled, bool invoke = true) { // Now, process any key bindings in the subviews that are tagged to KeyBindingScope.HotKey. foreach (View subview in Subviews) @@ -631,7 +621,7 @@ public partial class View // Keyboard APIs continue; } - if (subview.KeyBindings.TryGet (keyEvent, scope, out KeyBinding binding)) + if (subview.KeyBindings.TryGet (key, scope, out KeyBinding binding)) { if (binding.Scope == KeyBindingScope.Focused && !subview.HasFocus) { @@ -643,7 +633,7 @@ public partial class View // Keyboard APIs return true; } - bool? subViewHandled = subview.RaiseInvokingKeyBindingsAndInvokeCommands (keyEvent); + bool? subViewHandled = subview.RaiseInvokingKeyBindingsAndInvokeCommands (key); if (subViewHandled is { }) { @@ -656,7 +646,7 @@ public partial class View // Keyboard APIs } } - bool recurse = subview.ProcessSubViewKeyBindings (keyEvent, scope, ref handled, invoke); + bool recurse = subview.ProcessSubViewKeyBindings (key, scope, ref handled, invoke); if (recurse || (handled is { } && (bool)handled)) { @@ -676,7 +666,7 @@ public partial class View // Keyboard APIs /// The key to test. /// Returns the view the key is bound to. /// - public bool IsHotKeyKeyBound (Key key, out View? boundView) + public bool IsHotKeyBound (Key key, out View? boundView) { // recurse through the subviews to find the views that has the key bound boundView = null; @@ -690,7 +680,7 @@ public partial class View // Keyboard APIs return true; } - if (subview.IsHotKeyKeyBound (key, out boundView)) + if (subview.IsHotKeyBound (key, out boundView)) { return true; } @@ -707,14 +697,14 @@ public partial class View // Keyboard APIs /// /// See for an overview of Terminal.Gui keyboard APIs. /// - /// Contains the details about the key that produced the event. + /// Contains the details about the key that produced the event. /// The scope. /// /// if the event was raised and was not handled (or cancelled); input processing should /// continue. /// if the event was raised and handled (or cancelled); input processing should stop. /// - protected virtual bool OnInvokingKeyBindings (Key keyEvent, KeyBindingScope scope) + protected virtual bool OnInvokingKeyBindings (Key key, KeyBindingScope scope) { return false; } @@ -749,19 +739,16 @@ public partial class View // Keyboard APIs #if DEBUG - // TODO: Determine if App scope bindings should be fired first or last (currently last). if (Application.KeyBindings.TryGet (key, KeyBindingScope.Focused | KeyBindingScope.HotKey, out KeyBinding b)) { - //var boundView = views [0]; - //var commandBinding = boundView.KeyBindings.Get (key); Debug.WriteLine ( - $"WARNING: InvokeKeyBindings ({key}) - An Application scope binding exists for this key. The registered view will not invoke Command."); //{commandBinding.Commands [0]}: {boundView}."); + $"WARNING: InvokeKeyBindings ({key}) - An Application scope binding exists for this key. The registered view will not invoke Command."); } // TODO: This is a "prototype" debug check. It may be too annoying vs. useful. // Scour the bindings up our View hierarchy // to ensure that the key is not already bound to a different set of commands. - if (SuperView?.IsHotKeyKeyBound (key, out View? previouslyBoundView) ?? false) + if (SuperView?.IsHotKeyBound (key, out View? previouslyBoundView) ?? false) { Debug.WriteLine ($"WARNING: InvokeKeyBindings ({key}) - A subview or peer has bound this Key and will not see it: {previouslyBoundView}."); } diff --git a/UnitTests/View/Keyboard/KeyboardEventTests.cs b/UnitTests/View/Keyboard/KeyboardEventTests.cs index ae597a596..471016231 100644 --- a/UnitTests/View/Keyboard/KeyboardEventTests.cs +++ b/UnitTests/View/Keyboard/KeyboardEventTests.cs @@ -225,7 +225,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews { Assert.Equal (KeyCode.A, e.KeyCode); Assert.False (keyPressed); - Assert.False (view.OnInvokingKeyBindingsCalled); + Assert.True (view.OnInvokingKeyBindingsCalled); e.Handled = true; invokingKeyBindings = true; }; @@ -245,7 +245,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews Assert.False (keyPressed); Assert.True (view.OnKeyDownCalled); - Assert.False (view.OnInvokingKeyBindingsCalled); + Assert.True (view.OnInvokingKeyBindingsCalled); Assert.False (view.OnProcessKeyDownCalled); } @@ -362,7 +362,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews { Assert.Equal (KeyCode.A, e.KeyCode); Assert.False (processKeyDown); - Assert.False (view.OnInvokingKeyBindingsCalled); + Assert.True (view.OnInvokingKeyBindingsCalled); e.Handled = false; invokingKeyBindings = true; }; diff --git a/docfx/docs/events.md b/docfx/docs/events.md index 19e853f6a..56580a7a4 100644 --- a/docfx/docs/events.md +++ b/docfx/docs/events.md @@ -71,56 +71,56 @@ A cancellable event is really two events and some activity that takes place betw The `OrientationHelper` class supporting `IOrientation` and a `View` having an `Orientation` property illustrates the preferred TG pattern for cancelable events. ```cs - /// - /// Gets or sets the orientation of the View. - /// - public Orientation Orientation - { - get => _orientation; - set - { - if (_orientation == value) - { - return; - } + /// + /// Gets or sets the orientation of the View. + /// + public Orientation Orientation + { + get => _orientation; + set + { + if (_orientation == value) + { + return; + } - // Best practice is to invoke the virtual method first. - // This allows derived classes to handle the event and potentially cancel it. - if (_owner?.OnOrientationChanging (value, _orientation) ?? false) - { - return; - } + // Best practice is to call the virtual method first. + // This allows derived classes to handle the event and potentially cancel it. + if (_owner?.OnOrientationChanging (value, _orientation) ?? false) + { + return; + } - // If the event is not canceled by the virtual method, raise the event to notify any external subscribers. - CancelEventArgs args = new (in _orientation, ref value); - OrientationChanging?.Invoke (_owner, args); + // If the event is not canceled by the virtual method, raise the event to notify any external subscribers. + CancelEventArgs args = new (in _orientation, ref value); + OrientationChanging?.Invoke (_owner, args); - if (args.Cancel) - { - return; - } + if (args.Cancel) + { + return; + } - // If the event is not canceled, update the value. - Orientation old = _orientation; + // If the event is not canceled, update the value. + Orientation old = _orientation; - if (_orientation != value) - { - _orientation = value; + if (_orientation != value) + { + _orientation = value; - if (_owner is { }) - { - _owner.Orientation = value; - } - } + if (_owner is { }) + { + _owner.Orientation = value; + } + } - // Best practice is to invoke the virtual method first. - _owner?.OnOrientationChanged (_orientation); - - // Even though Changed is not cancelable, it is still a good practice to raise the event after. - OrientationChanged?.Invoke (_owner, new (in _orientation)); - } - } + // Best practice is to call the virtual method first, then raise the event. + _owner?.OnOrientationChanged (_orientation); + OrientationChanged?.Invoke (_owner, new (in _orientation)); + } + } ``` + ## `bool` or `bool?` +