Get consistent on events: Either Action or event, not both

[tig]

First, I'll point out that none of this really matters. I'm investing blood & sweat into this project because it's fun. It's a low-risk project that potentially 10s of others will use ;-). I don't really care about these topics because I'm not a professional developer. I just play one on TV.

That said, I'd love to have people using Terminal.Gui and say "wow, this thing is really easy to use and work great! The people who worked on it sure were thoughtful.". Of course, 99% of the credit for that will always go to @migueldeicaza.

To this end: This project has become confused on whether the idiom for events is based on Action or event. In addition there's gross inconsistency between naming of events. E.g.

• Recently we added static public Action OnResized to Application.
• Meanwhile most View derived events use event as in public event EventHandler Enter.
• Some event handlers use "On" and some don't. E.g. public event EventHandler OnOpenMenu vs. public event EventHandler TextChanged.

I'm guilty of contributing to this myself. I hereby commit that moving forward, no more. I will do better.

The best guidance on naming event related things I've seen is this:
https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members?redirectedfrom=MSDN

Events always refer to some action, either one that is happening or one that has occurred. Therefore, as with methods, events are named with verbs, and verb tense is used to indicate the time when the event is raised.

✔️ DO name events with a verb or a verb phrase.

Examples include Clicked, Painting, DroppedDown, and so on.

✔️ DO give events names with a concept of before and after, using the present and past tenses.

For example, a close event that is raised before a window is closed would be called Closing, and one that is raised after the window is closed would be called Closed.

❌ DO NOT use "Before" or "After" prefixes or postfixes to indicate pre- and post-events. Use present and past tenses as just described.

✔️ DO name event handlers (delegates used as types of events) with the "EventHandler" suffix, as shown in the following example:

public delegate void ClickedEventHandler(object sender, ClickedEventArgs e);

✔️ DO use two parameters named sender and e in event handlers.

The sender parameter represents the object that raised the event. The sender parameter is typically of type object, even if it is possible to employ a more specific type.

✔️ DO name event argument classes with the "EventArgs" suffix.

I found this article to be especially clarifying: https://www.codeproject.com/Articles/20550/C-Event-Implementation-Fundamentals-Best-Practices

We are not consistent along these lines in Terminal.Gui at all. This leads to friction for adopters and bugs.

Because I'm so familiar with WinForms and classic .NET event I have a bias for the event/EventHandler model. Action seems well suited for internal APIs, but event/EventHandler seems to scale better for public APIs. I'd like us to minimize use of Action.

I would like to take on fixing this (it speaks to my OCD). There's no way to do this without breaking changes, I'm afraid. But the longer it waits, the harder it will be.

My proposed Event rules for Terminal.Gui moving forward:

1. We follow the naming advice provided in https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members?redirectedfrom=MSDN
2. We only use the Action<T> idiom for internal APIs, not for public APIs. For public APIs we use the event/EventHandler model.
3. For public APIs, the class that can raise the event will implement:
   • A virtual event raising function, named as OnEventToRaise. Typical implementations will simply do a EventToRaise?.Invoke(this, eventArgs).
   • An event as in public event EventHandler<EventToRaiseArgs> EventToRaise
   • Consumers of the event can do theobject.EventToRaise += (sender,, e) => {};
   • Sub-classes of the class implementing EventToRaise can override OnEventToRaise as needed.
4. Where possible, a subclass of EventArgs should be provided and the previous state should be included.

At the very minimum, can we agree that moving forward we will follow the above rules?

You'll note that my toplevel_ready PR attempts to follow these rules: #446

What enhancements, changes would you make to these rules?

[tig]

I did a code review of the project to get an inventory of just how good/bad of shape we are in. Here it is:

• ConsoleDriver

  • Init - Uses Action
    • Fine - Internal API
  • PepareToRun - Uses Action -
    • Fine - internal API
  • protected Action TerminalResized;
    • Fine - Internal API

• NetMainLoop -

  • Uses Action for public Action<ConsoleKeyInfo> WindowsKeyPressed;
    • FIX: This should not be public !?!?

• Application

  • public static Action<MouseEvent> RootMouseEvent;
    • Fine - Debugging API
  • public static event EventHandler<ResizedEventArgs> Loaded
    • great, except ResizedEventArgs only includes current; best practice is to include previous. Not worth fixing.
  • public static event EventHandler<ResizedEventArgs> Resized
    • great, except ResizedEventArgs only includes current; best practice is to include previous. Not worth fixing.

• MainLoop

  • public void Invoke (Action action)
    • Fine; threading related. Action works great with Tasks.

• View

  • void PerformActionForSubview (View subview, Action<View> action)
    • Fine - Internal API
  • public event EventHandler<FocusEventArgs> Enter;
  • public event EventHandler<FocusEventArgs> Leave;
  • public event EventHandler<MouseEventEventArgs> MouseEnter;
  • public event EventHandler<MouseEventEventArgs> MouseLeave;
  • public event EventHandler<MouseEventEventArgs> MouseClick;
  • public event EventHandler<KeyEventEventArgs> KeyPress; etc...
    • All fine.
  • public event EventHandler<LayoutEventArgs> LayoutComplete;
    • Fine.

• TopLevel

  • public event EventHandler Ready;
    • Fine.

• Button

  • public Action Clicked;
    • FIX - Public API that should use the event/EventHandler model
      • BREAKING CHANGE unless we can come up with a name for an event that makes more sense than Clicked (Selected ???).

• Checkbox

  • public event EventHandler Toggled
    • Fine, but ideally would include previous state (either EventHandler<bool> or via a EventArgs subclass)

• ComboBox

  • public event EventHandler<ustring> Changed;
    • The implementation is questionable.
      1. It's sending the current text; best practice is for an event to provide previous value
      2. Calls to Invoke are spread around. Ideally there would be public virtual void OnTextChanged() method that calls Changed?.Invoke.
      3. The name is "Changed" but the event actually represents "the selection has been confirmed". Rename it to SelectionChanged?

• Menu

  • public MenuItem (ustring title, string help, Action action, Func<bool> canExecute = null) -
  • public Action Action { get; set; }
  • etc...
    • FIX - Public API that should use the event/EventHandler model
      • BREAKING CHANGE
      • May be possible to introduce a parallel API retaining backwards compat (an event named named Clicked or Selected).
  • public event EventHandler OnOpenMenu;
    • Should event include state? E.g. Which menu?
  • public event EventHandler OnCloseMenu;
    - Should event include state? E.g. Which menu?

• RadioGroup

  • public Action<int> SelectionChanged;
    • FIX - Public API that should use the event/EventHandler model.
      • Could avoid BREAKING CHANGE by introducing a parallel event named SelectedItemChanged.

• ScrollView

  • public event Action ChangedPosition;
    • FIX - Public API that should use the event/EventHandler model.
      • Could avoid BREAKING CHANGE by introducing a parallel event named SelectedItemChanged.

• StatusBar

  • public Action Action { get; } etc..
    • FIX - Public API that should use the event/EventHandler model.
      • Could avoid BREAKING CHANGE by introducing a parallel event named Clicked or Selected
    • Unrelated: Why does Run use Application.MainLoop.AddIdle() and not just do Invoke?

• FileDialog

  • public Action<(string, bool)> SelectedChanged { get; set; }
    • FIX - Public API that should use the event/EventHandler model.
      • public event EventHandler<int> SelectedItemChanged?
  • public Action DirectoryChanged { get; set; }
    • FIX - Public API that should use the event/EventHandler model.
      • public event EventHandler<ustring> DirectorySelected?
  • public Action FileChanged { get; set; }
    • FIX - Public API that should use the event/EventHandler model.
      • `public event EventHandler FileSelected?

• ListView

  • public event EventHandler<ListViewItemEventArgs> SelectedChanged;
    • Fine, although ideally ListViewItemEventArgs would include the previous state.
    • Rename to SelectedItemChanged?
  • public event EventHandler<ListViewItemEventArgs> OpenSelectedItem;
    • Fine, although ideally ListViewItemEventArgs would include the previous state.

• TextView

  • public event EventHandler TextChanged;
    • Inconsistently named relative to same property on TextField
    • BAD DESIGN
      • Every time Text text changes, a copy of the ENTIRE buffer will be made and passed with this event.
      • For classes that deal with small amounts of data, this design would be fine. But TextView may hold megabytes or more.
      • Ideally a EventArgs subclass would be deinfed to make it explicitly clear that the ustring is the old value AND make it easier to add more state in the future.
      • Instead, we should make an exception to sending state with the event and not do it.
      • Should this class should be redesigned to use reference values for the text buffer, vs naively just causing copy operations?

• TextField

  • public event EventHandler<ustring> Changed
    • Inconsistently named relative to same property on TextView
    • BAD DESIGN
      • Every time Text text changes, a copy of the ENTIRE buffer will be made and passed with this event.
      • For classes that deal with small amounts of data, this design would be fine. But TextView may hold megabytes or more.
      • Instead, we should make an exception to sending state with the event and not do it.
      • Should this class should be redesigned to use reference values for the text buffer, vs naively just causing copy operations?

[BDisp]

* Unrelated: Why does `Run` use `Application.MainLoop.AddIdle()` and not just do `Invoke`?

If an Action is not added to it, it will not perform properly threading Tasks, unless the invoke get called from inside an Action that was previously added to the Application.MainLoop.AddIdle().

[BDisp]

Commented here #621 (comment)

[tig]

See: https://github.com/migueldeicaza/gui.cs/wiki/Framework-Design-Guidelines

[BDisp]

@tig is there any change from yesterday? It’s because I’ve read it several times and it’s always the same thing, or else I’ll step aside :-)

[tig]

@tig is there any change from yesterday? It’s because I’ve read it several times and it’s always the same thing, or else I’ll step aside :-)

Literally just updated it ;-)

[BDisp]

It would be easier to read if you highlight the changes than to be reading everything again because it always looks like the same text that I end up giving up on reading it all over ;-)

[tig]

Roger. Next time I will do that. We're almost done though :-)

[BDisp]

I'm dealing with Button's unicode. Only in justified alignment is it a little difficult to get the hotkey position but I will get there.

[tig]

This was mostly taken care of as part of the push to 1.0. Closing.