From 6df071fbe1e420efee7ffd2fb626d3b0f624cea4 Mon Sep 17 00:00:00 2001 From: Tig Date: Mon, 14 Oct 2024 12:32:41 -0600 Subject: [PATCH] Fixed. Added event.md --- Terminal.Gui/View/View.Keyboard.cs | 70 +++++----- Terminal.Gui/Views/Menu/Menu.cs | 16 +-- Terminal.Gui/Views/TextField.cs | 2 +- Terminal.Gui/Views/TextView.cs | 2 +- .../View/{ => Keyboard}/KeyboardEventTests.cs | 76 +++++------ UnitTests/Views/MenuBarTests.cs | 2 +- docfx/docs/events.md | 126 ++++++++++++++++++ 7 files changed, 207 insertions(+), 87 deletions(-) rename UnitTests/View/{ => Keyboard}/KeyboardEventTests.cs (86%) create mode 100644 docfx/docs/events.md diff --git a/Terminal.Gui/View/View.Keyboard.cs b/Terminal.Gui/View/View.Keyboard.cs index c1a28a5b9..cf7f9cdf1 100644 --- a/Terminal.Gui/View/View.Keyboard.cs +++ b/Terminal.Gui/View/View.Keyboard.cs @@ -1,5 +1,6 @@ #nullable enable using System.Diagnostics; +using Microsoft.CodeAnalysis.FlowAnalysis; namespace Terminal.Gui; @@ -292,17 +293,20 @@ public partial class View // Keyboard APIs // During (this is what can be cancelled) - if (RaiseInvokingKeyBindingsAndInvokeCommands (key) is not false || key.Handled) - { - return true; - } + // TODO: NewKeyDownEvent returns bool. It should be bool? so state of RaiseInvokingKeyBindingsAndInvokeCommands can be reflected up stack - if (RaiseProcessKeyDown (key) || key.Handled) + + if (RaiseInvokingKeyBindingsAndInvokeCommands (key) is true || key.Handled) { return true; } // After + // TODO: Is ProcessKeyDown really the right name? + if (RaiseProcessKeyDown (key) || key.Handled) + { + return true; + } return key.Handled; @@ -322,14 +326,13 @@ public partial class View // Keyboard APIs bool RaiseProcessKeyDown (Key key) { - // BUGBUG: The proper pattern is for the v-method (OnProcessKeyDown) to be called first, then the event - ProcessKeyDown?.Invoke (this, key); - - if (!key.Handled && OnProcessKeyDown (key)) + if (OnProcessKeyDown (key) || key.Handled) { return true; } + ProcessKeyDown?.Invoke (this, key); + return false; } } @@ -512,17 +515,22 @@ 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 + /// continue. + /// if was handled or a command was invoked and handled (or cancelled); input processing should stop. + /// 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; - } + //// 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 = kb.Scope; + KeyBindingScope scope = KeyBindingScope.Focused | KeyBindingScope.HotKey; // During // BUGBUG: The proper pattern is for the v-method (OnInvokingKeyBindings) to be called first, then the event @@ -533,14 +541,13 @@ public partial class View // Keyboard APIs return true; } - // TODO: NewKeyDownEvent returns bool. It should be bool? so state of InvokeCommand can be reflected up stac - bool? handled = OnInvokingKeyBindings (key, scope); - - if (handled is { } && (bool)handled) + if (OnInvokingKeyBindings (key, scope)) { return true; } + bool? handled; + // After // * If no key binding was found, `InvokeKeyBindings` returns `null`. @@ -636,7 +643,7 @@ public partial class View // Keyboard APIs return true; } - bool? subViewHandled = subview.OnInvokingKeyBindings (keyEvent, scope); + bool? subViewHandled = subview.RaiseInvokingKeyBindingsAndInvokeCommands (keyEvent); if (subViewHandled is { }) { @@ -694,26 +701,25 @@ public partial class View // Keyboard APIs /// - /// Low-level API called when a user presses a key; invokes any key bindings set on the view. This is called - /// during after has returned. + /// Called when a key is pressed that may be mapped to a key binding. Set to true to + /// stop the key from being processed by other views. /// /// - /// Fires the event. /// See for an overview of Terminal.Gui keyboard APIs. /// /// Contains the details about the key that produced the event. /// The scope. /// - /// if no event was raised; input proessing should continue. - /// if the event was raised and was not handled (or cancelled); input proessing should + /// 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 proessing should stop. + /// 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 keyEvent, KeyBindingScope scope) { return false; } + // TODO: This does not carry KeyBindingScope, but OnInvokingKeyBindings does /// /// Raised when a key is pressed that may be mapped to a key binding. Set to true to /// stop the key from being processed by other views. @@ -727,10 +733,10 @@ public partial class View // Keyboard APIs /// The key event passed. /// The scope. /// - /// if no command was invoked; input proessing should continue. - /// if at least one command was invoked and was not handled (or cancelled); input proessing + /// if no command was invoked; input processing should continue. + /// if at least one command was invoked and was not handled (or cancelled); input processing /// should continue. - /// if at least one command was invoked and handled (or cancelled); input proessing should stop. + /// if at least one command was invoked and handled (or cancelled); input processing should stop. /// protected bool? InvokeCommands (Key key, KeyBindingScope scope) { diff --git a/Terminal.Gui/Views/Menu/Menu.cs b/Terminal.Gui/Views/Menu/Menu.cs index 7d7715a53..9ee580d49 100644 --- a/Terminal.Gui/Views/Menu/Menu.cs +++ b/Terminal.Gui/Views/Menu/Menu.cs @@ -305,19 +305,11 @@ internal sealed class Menu : View return true; } - /// - protected override bool? OnInvokingKeyBindings (Key keyEvent, KeyBindingScope scope) + /// + protected override bool OnProcessKeyDown (Key keyEvent) { - bool? handled = base.OnInvokingKeyBindings (keyEvent, scope); - - if (handled is { } && (bool)handled) - { - return true; - } - - // TODO: Determine if there's a cleaner way to handle this. - // This supports the case where the menu bar is a context menu - return _host.RaiseInvokingKeyBindingsAndInvokeCommands (keyEvent); + // We didn't handle the key, pass it on to host + return _host.RaiseInvokingKeyBindingsAndInvokeCommands (keyEvent) == true; } private void Current_TerminalResized (object? sender, SizeChangedEventArgs e) diff --git a/Terminal.Gui/Views/TextField.cs b/Terminal.Gui/Views/TextField.cs index b27cd82e0..d1cfed3b1 100644 --- a/Terminal.Gui/Views/TextField.cs +++ b/Terminal.Gui/Views/TextField.cs @@ -1014,7 +1014,7 @@ public class TextField : View } /// - protected override bool? OnInvokingKeyBindings (Key a, KeyBindingScope scope) + protected override bool OnInvokingKeyBindings (Key a, KeyBindingScope scope) { // Give autocomplete first opportunity to respond to key presses if (SelectedLength == 0 && Autocomplete.Suggestions.Count > 0 && Autocomplete.ProcessKey (a)) diff --git a/Terminal.Gui/Views/TextView.cs b/Terminal.Gui/Views/TextView.cs index 770e4293b..4cc86bf2f 100644 --- a/Terminal.Gui/Views/TextView.cs +++ b/Terminal.Gui/Views/TextView.cs @@ -3664,7 +3664,7 @@ public class TextView : View } /// - protected override bool? OnInvokingKeyBindings (Key a, KeyBindingScope scope) + protected override bool OnInvokingKeyBindings (Key a, KeyBindingScope scope) { if (!a.IsValid) { diff --git a/UnitTests/View/KeyboardEventTests.cs b/UnitTests/View/Keyboard/KeyboardEventTests.cs similarity index 86% rename from UnitTests/View/KeyboardEventTests.cs rename to UnitTests/View/Keyboard/KeyboardEventTests.cs index fc601590c..ae597a596 100644 --- a/UnitTests/View/KeyboardEventTests.cs +++ b/UnitTests/View/Keyboard/KeyboardEventTests.cs @@ -12,7 +12,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews /// [Theory] [MemberData (nameof (AllViewTypes))] - public void AllViews_KeyDown_All_EventsFire (Type viewType) + public void AllViews_NewKeyDownEvent_All_EventsFire (Type viewType) { var view = CreateInstanceIfNotGeneric (viewType); @@ -49,7 +49,8 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews keyDownProcessed = true; }; - Assert.True (view.NewKeyDownEvent (Key.A)); // this will be true because the ProcessKeyDown event handled it + // Key.Empty is invalid, but it's used here to test that the event is fired + Assert.True (view.NewKeyDownEvent (Key.Empty)); // this will be true because the ProcessKeyDown event handled it Assert.True (keyDown); Assert.True (invokingKeyBindings); Assert.True (keyDownProcessed); @@ -62,7 +63,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews /// [Theory] [MemberData (nameof (AllViewTypes))] - public void AllViews_KeyUp_All_EventsFire (Type viewType) + public void AllViews_NewKeyUpEvent_All_EventsFire (Type viewType) { var view = CreateInstanceIfNotGeneric (viewType); @@ -92,13 +93,13 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews [InlineData (true, false, false)] [InlineData (true, true, false)] [InlineData (true, true, true)] - public void Events_Are_Called_With_Only_Key_Modifiers (bool shift, bool alt, bool control) + public void NewKeyDownUpEvents_Events_Are_Raised_With_Only_Key_Modifiers (bool shift, bool alt, bool control) { var keyDown = false; var keyPressed = false; var keyUp = false; - var view = new OnKeyTestView (); + var view = new OnNewKeyTestView (); view.CancelVirtualMethods = false; view.KeyDown += (s, e) => @@ -139,7 +140,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews ); Assert.True (keyPressed); Assert.True (view.OnKeyDownCalled); - Assert.True (view.OnKeyPressedContinued); + Assert.True (view.OnProcessKeyDownCalled); view.NewKeyUpEvent ( new ( @@ -154,7 +155,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews } [Fact] - public void InvokingKeyBindings_Handled_Cancels () + public void NewKeyDownEvent_InvokingKeyBindings_Handled_Cancels () { var view = new View (); var keyPressInvoked = false; @@ -201,13 +202,13 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews } [Fact] - public void InvokingKeyBindings_Handled_True_Stops_Processing () + public void NewKeyDownEvent_InvokingKeyBindings_Handled_True_Stops_Processing () { var keyDown = false; var invokingKeyBindings = false; var keyPressed = false; - var view = new OnKeyTestView (); + var view = new OnNewKeyTestView (); Assert.True (view.CanFocus); view.CancelVirtualMethods = false; @@ -233,7 +234,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews { Assert.Equal (KeyCode.A, e.KeyCode); Assert.False (keyPressed); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); e.Handled = true; keyPressed = true; }; @@ -245,17 +246,17 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews Assert.True (view.OnKeyDownCalled); Assert.False (view.OnInvokingKeyBindingsCalled); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); } [Fact] - public void KeyDown_Handled_True_Stops_Processing () + public void NewKeyDownEvent_Handled_True_Stops_Processing () { var keyDown = false; var invokingKeyBindings = false; var keyPressed = false; - var view = new OnKeyTestView (); + var view = new OnNewKeyTestView (); Assert.True (view.CanFocus); view.CancelVirtualMethods = false; @@ -281,7 +282,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews { Assert.Equal (KeyCode.A, e.KeyCode); Assert.False (keyPressed); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); e.Handled = true; keyPressed = true; }; @@ -293,11 +294,11 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews Assert.True (view.OnKeyDownCalled); Assert.False (view.OnInvokingKeyBindingsCalled); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); } [Fact] - public void KeyPress_Handled_Cancels () + public void NewKeyDownEvent_KeyDown_Handled_Stops_Processing () { var view = new View (); var invokingKeyBindingsInvoked = false; @@ -338,13 +339,13 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews } [Fact] - public void KeyPressed_Handled_True_Stops_Processing () + public void NewKeyDownEvent_ProcessKeyDown_Handled_Stops_Processing () { var keyDown = false; var invokingKeyBindings = false; - var keyPressed = false; + var processKeyDown = false; - var view = new OnKeyTestView (); + var view = new OnNewKeyTestView (); Assert.True (view.CanFocus); view.CancelVirtualMethods = false; @@ -360,7 +361,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews view.InvokingKeyBindings += (s, e) => { Assert.Equal (KeyCode.A, e.KeyCode); - Assert.False (keyPressed); + Assert.False (processKeyDown); Assert.False (view.OnInvokingKeyBindingsCalled); e.Handled = false; invokingKeyBindings = true; @@ -369,28 +370,28 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews view.ProcessKeyDown += (s, e) => { Assert.Equal (KeyCode.A, e.KeyCode); - Assert.False (keyPressed); - Assert.False (view.OnKeyPressedContinued); + Assert.False (processKeyDown); + Assert.True (view.OnProcessKeyDownCalled); e.Handled = true; - keyPressed = true; + processKeyDown = true; }; view.NewKeyDownEvent (Key.A); Assert.True (keyDown); Assert.True (invokingKeyBindings); - Assert.True (keyPressed); + Assert.True (processKeyDown); Assert.True (view.OnKeyDownCalled); Assert.True (view.OnInvokingKeyBindingsCalled); - Assert.False (view.OnKeyPressedContinued); + Assert.True (view.OnProcessKeyDownCalled); } [Fact] - public void KeyUp_Handled_True_Stops_Processing () + public void NewKeyUpEvent_KeyUp_Handled_True_Stops_Processing () { var keyUp = false; - var view = new OnKeyTestView (); + var view = new OnNewKeyTestView (); Assert.True (view.CanFocus); view.CancelVirtualMethods = false; @@ -398,7 +399,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews { Assert.Equal (KeyCode.A, e.KeyCode); Assert.False (keyUp); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); e.Handled = true; keyUp = true; }; @@ -409,14 +410,14 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews Assert.True (view.OnKeyUpCalled); Assert.False (view.OnKeyDownCalled); Assert.False (view.OnInvokingKeyBindingsCalled); - Assert.False (view.OnKeyPressedContinued); + Assert.False (view.OnProcessKeyDownCalled); } [Theory] [InlineData (null, null)] [InlineData (true, true)] [InlineData (false, false)] - public void OnInvokingKeyBindings_Returns_Nullable_Properly (bool? toReturn, bool? expected) + public void RaiseInvokingKeyBindingsAndInvokeCommands_Returns_Nullable_Properly (bool? toReturn, bool? expected) { var view = new KeyBindingsTestView (); view.CommandReturns = toReturn; @@ -439,17 +440,17 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews } /// A view that overrides the OnKey* methods so we can test that they are called. - public class OnKeyTestView : View + public class OnNewKeyTestView : View { - public OnKeyTestView () { CanFocus = true; } + public OnNewKeyTestView () { CanFocus = true; } public bool CancelVirtualMethods { set; private get; } public bool OnInvokingKeyBindingsCalled { get; set; } public bool OnKeyDownCalled { get; set; } - public bool OnKeyPressedContinued { get; set; } + public bool OnProcessKeyDownCalled { get; set; } public bool OnKeyUpCalled { get; set; } public override string Text { get; set; } - protected override bool? OnInvokingKeyBindings (Key keyEvent, KeyBindingScope scope) + protected override bool OnInvokingKeyBindings (Key keyEvent, KeyBindingScope scope) { OnInvokingKeyBindingsCalled = true; @@ -473,12 +474,7 @@ public class KeyboardEventTests (ITestOutputHelper output) : TestsAllViews protected override bool OnProcessKeyDown (Key keyEvent) { - if (base.OnProcessKeyDown (keyEvent)) - { - return true; - } - - OnKeyPressedContinued = true; + OnProcessKeyDownCalled = true; return CancelVirtualMethods; } diff --git a/UnitTests/Views/MenuBarTests.cs b/UnitTests/Views/MenuBarTests.cs index 736e79890..849d9b8f4 100644 --- a/UnitTests/Views/MenuBarTests.cs +++ b/UnitTests/Views/MenuBarTests.cs @@ -1384,7 +1384,7 @@ wo foreach (Key key in keys) { - top.NewKeyDownEvent (new (key)); + top.NewKeyDownEvent (key); Application.MainLoop.RunIteration (); } diff --git a/docfx/docs/events.md b/docfx/docs/events.md new file mode 100644 index 000000000..19e853f6a --- /dev/null +++ b/docfx/docs/events.md @@ -0,0 +1,126 @@ +# Terminal.Gui Event Deep Dive + +Terminal.Gui exposes and uses events in many places. This deep dive covers the patterns used, where they are used, and notes any exceptions. + +## Tenets for Terminal.Gui Events (Unless you know better ones...) + +Tenets higher in the list have precedence over tenets lower in the list. + +* **UI Interaction and Live Data Are Different Beasts** - TG distinguishes between events used for human interaction and events for live data. We don't believe in a one-size-fits-all eventing model. For UI interactions we use `EventHandler`. For data binding we think `INotifyPropertyChanged` is groovy. For some callbacks we use `Action`. + +## Lexicon and Taxonomy + +* *Action* +* *Event* +* *Command* +* *Invoke* +* *Raise* +* *Listen* +* *Handle/Handling/Handled* - Applies to scenarios where an event can either be handled by an event listener (or override) vs not handled. Events that originate from a user action like mouse moves and key presses are examples. +* *Cancel/Cancelling/Cancelled* - Applies to scenarios where something can be cancelled. Changing the `Orientation` of a `Slider` is cancelable. + +## Useful External Documentation + +* [.NET Naming Guidelines - Names of Events](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members?redirectedfrom=MSDN#names-of-events) +* [.NET Design for Extensibility - Events and Callbacks](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/events-and-callbacks) +* [C# Event Implementation Fundamentals, Best Practices and Conventions](https://www.codeproject.com/Articles/20550/C-Event-Implementation-Fundamentals-Best-Practices) + +## Naming + +TG follows the *naming* advice provided in [.NET Naming Guidelines - Names of Events](https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members?redirectedfrom=MSDN#names-of-events). + +## `EventHandler` style event best-practices + +* Implement a helper method for raising the event: `RaisexxxEvent`. + * If the event is cancelable, the return type should be either `bool` or `bool?`. + * Can be `private`, `internal`, or `public` depending on the situation. `internal` should only be used to enable unit tests. +* Raising an event involves FIRST calling the `protected virtual` method, THEN invoking the `EventHandler. + +## `Action` style callback best-practices + +- tbd + +## `INotifyPropertyChanged` style notification best practices + +- tbd + +## Common Patterns + +The primary pattern for events is the `event/EventHandler` idiom. We use the `Action` idiom sparingly. We support `INotifyPropertyChanged` in cases where data binding is relevant. + + + +## Cancellable Event Pattern + +A cancellable event is really two events and some activity that takes place between those events. The "pre-event" happens before the activity. The activity then takes place (or not). If the activity takes place, then the "post-event" is typically raised. So, to be precise, no event is being cancelled even though we say we have a cancellable event. Rather, the activity that takes place between the two events is what is cancelled — and likely prevented from starting at all. + +### **Before** - If any pre-conditions are met raise the "pre-event", typically named in the form of "xxxChanging". e.g. + + - A `protected virtual` method is called. This method is named `OnxxxChanging` and the base implementation simply does `return false`. + - If the `OnxxxChanging` method returns `true` it means a derived class canceled the event. Processing should stop. + - Otherwise, the `xxxChanging` event is invoked via `xxxChanging?.Invoke(args)`. If `args.Cancel/Handled == true` it means a subscriber has cancelled the event. Processing should stop. + + +### **During** - Do work. + +### **After** - Raise the "post-event", typically named in the form of "xxxChanged" + + - A `protected virtual` method is called. This method is named `OnxxxChanged` has a return type of `void`. + - The `xxxChanged` event is invoked via `xxxChanging?.Invoke(args)`. + +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; + } + + // 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; + } + + // 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 the event is not canceled, update the value. + Orientation old = _orientation; + + if (_orientation != value) + { + _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)); + } + } +``` + + +