Skip to content

Framework Design Guidelines

Jump to bottom
Charlie Kindel edited this page Jun 5, 2020 · 7 revisions

Events

This project has become confused on whether the idiom for events is based on Action or event/EventHandler. 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.

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

@migueldeicaza is pretty clear on his preference for Action and dislike of event/EventHandler. Based on this, here is a PROPOSAL for adjusted "rules" for this project w.r.t. events.

Naming

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

Guidelines for defining Events

Proposed Event rules for Terminal.Gui moving forward (STILL UP FOR DEBATE):

  1. We follow the naming advice provided in https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/names-of-type-members?redirectedfrom=MSDN. Adaptaions specifically to Terminal.Gui:
    • To make it clear that the Action is being used for event semantics, event names should use the word Event in them and not Action. For use cases where the delgate does not have eventing semantics, using Action (or something else appropriate) is ok.
  2. We use the Action<T> idiom primarily. We do not use the event/EventHandler idiom.
  3. The class that defines the event and can raise the event will implement:
    • An event based on Action<T>, as in public Action<EventToRaiseArgs> EventToRaise
    • T can be any type, and designers are encouraged to be thoughtful about how subscribers will utilize the state.
      • If there is any chance that additional state may be needed to be passed with the event in the future, T should be a type defined along with the class (e.g. KeyEventArgs has members for KeyEvent, and Handled).
      • Generally, previous state is more valuable to a subscriber than new state (subscribers can usually just ask the object for the new state). For this reason, passing "previous values" is encouraged where appropriate.
      • Don't over do it. If there is not a clear/real use-case it is better to not over-engineer (but see the point above about making T a class that can more easily be extended in the future, minimizing subscriber changes required).
    • A virtual event raising function, named as OnEventToRaise. Typical implementations will simply do a EventToRaise?.Invoke(eventToRaiseArgs). This enables sub-classes to change the event raising behavior, an important extensibility mechanism.
    • Subscribers of the event can do theobject.EventToRaise+= (args) => {}; to subscribe.
    • Sub-classes of the class implementing EventToRaise can override OnEventToRaise as needed.

What needs to change for the project to adhere to the guidelines above:

(DRAFT - This is all incorrect!)

  • 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?
Clone this wiki locally