mirror of
https://github.com/gui-cs/Terminal.Gui.git
synced 2025-12-26 15:57:56 +01:00
Created Framework Design Guidelines (markdown)
Charlie Kindel
131
Framework-Design-Guidelines.md
Normal file
131
Framework-Design-Guidelines.md
Normal file
@@ -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<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<ustring> DirectoryChanged { get; set; }
|
||||
- FIX - Public API that should use the `event/EventHandler` model.
|
||||
- `public event EventHandler<ustring> DirectorySelected`?
|
||||
- public Action<ustring> FileChanged { get; set; }
|
||||
- FIX - Public API that should use the `event/EventHandler` model.
|
||||
- `public event EventHandler<ustring> 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?
|
||||
Reference in New Issue
Block a user