Framework Design Guidelines

Events

@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.

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?