From ffd5efc202f9adfec0921cc769ac4b3b5c7be25f Mon Sep 17 00:00:00 2001 From: Charlie Kindel Date: Fri, 5 Jun 2020 12:53:43 -0600 Subject: [PATCH] Created Framework Design Guidelines (markdown) --- Framework-Design-Guidelines.md | 131 +++++++++++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 Framework-Design-Guidelines.md diff --git a/Framework-Design-Guidelines.md b/Framework-Design-Guidelines.md new file mode 100644 index 0000000..eb29a87 --- /dev/null +++ b/Framework-Design-Guidelines.md @@ -0,0 +1,131 @@ +# 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 WindowsKeyPressed;` + - FIX: This should not be `public` !?!? + +- Application + - `public static Action RootMouseEvent;` + - Fine - Debugging API + - `public static event EventHandler Loaded` + - great, except `ResizedEventArgs` only includes current; best practice is to include *previous*. Not worth fixing. + - `public static event EventHandler 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 action)` + - Fine - Internal API + - `public event EventHandler Enter;` + - `public event EventHandler Leave;` + - `public event EventHandler MouseEnter;` + - `public event EventHandler MouseLeave;` + - `public event EventHandler MouseClick;` + - `public event EventHandler KeyPress;` etc... + - All fine. + - `public event EventHandler 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` or via a `EventArgs` subclass) + +- ComboBox + - `public event EventHandler 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 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 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 SelectedItemChanged`? + - public Action DirectoryChanged { get; set; } + - FIX - Public API that should use the `event/EventHandler` model. + - `public event EventHandler DirectorySelected`? + - public Action FileChanged { get; set; } + - FIX - Public API that should use the `event/EventHandler` model. + - `public event EventHandler FileSelected? + +- ListView + - `public event EventHandler SelectedChanged;` + - Fine, although ideally `ListViewItemEventArgs` would include the previous state. + - Rename to `SelectedItemChanged`? + - `public event EventHandler 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 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?