Skip to content

Framework Design Guidelines

Jump to bottom
Tig edited this page Dec 9, 2023 · 6 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.

Good article:

https://www.codeproject.com/articles/20550/c-event-implementation-fundamentals-best-practices

In sum, 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.

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

UPDATE THIS AS THESE GET FIXED

  • 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
    • FIXED: public static Action<ResizedEventArgs> Loaded
    • FIXED: public static Action<ResizedEventArgs> Resized

  • MainLoop

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

  • View

    • ALL FIXED: public Action<FocusEventArgs> Enter; etcc..

  • TopLevel

    • FIXED

  • Button

    • FIXED

  • Checkbox

    • FIXED

  • ComboBox

    • FIXED?

  • Menu

    • FIXED

  • RadioGroup

    • FIXED: public Action<SelectedItemChangedArgs> SelectedItemChanged;

  • ScrollView

    • FINE.

  • StatusBar

    • FINE. public Action Action { get; }

  • FileDialog

    • public Action<ustring> DirectoryChanged { get; set; }
      • SHOULD FIX - DirectorySelected`?
    • public Action<ustring> FileChanged { get; set; }
      • SHOULD FIX: FileSelected
    • public Action<(string, bool)> SelectedChanged { get; set; }
      • SHOULD FIX: SelectedFileChanged ???

  • ListView

    • SHOULD FIX: OnSelectedChanged -> OnSelectedItemChanged

  • TextView

    • public Action TextChanged;
      • 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 Action<ustring> TextChanged;
      • Inconsistent 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