Git_Mirror/Terminal.Gui
1
0
Fork 0
mirror of https://github.com/gui-cs/Terminal.Gui.git synced 2026-01-02 01:03:29 +01:00
Code Issues Packages Projects Releases Wiki Activity

Updated Framework Design Guidelines (markdown)

Tig
2024-10-14 12:33:19 -06:00
parent 7e9892b6a2
commit 458437f9ce
1 changed files with 1 additions and 124 deletions
Download Patch File Download Diff File Expand all files Collapse all files

125
Framework-Design-Guidelines.md

@@ -1,124 +1 @@
# 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.
- See https://github.com/migueldeicaza/gui.cs/issues/447
- I found this article to be especially clarifying: https://www.codeproject.com/Articles/20550/C-Event-Implementation-Fundamentals-Best-Practices
@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?
See `events.md` in Conceptual Docs