Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #2926 - Refactor KeyEvent and KeyEventEventArgs to simplify #2927

Merged
merged 205 commits into from
Dec 16, 2023

Conversation

tig
Copy link
Collaborator

@tig tig commented Oct 21, 2023

Fixes #2926 - Refactor KeyEvent and KeyEventEventArgs to simplify
Fixes #2978 - Simplify Key - BackTab and upper-case alphas
Fixes #3038 - 'View.HotKey' automatic setting supports only char.IsLetterOrDigit
Fixes #3042 - Enable Views to declare key bindings for their SuperView
Fixes #2826 - v2 lowercase letters must be removed from the Key enum.

  • Rearrange Event.cs to Keyboard.cs and Mouse.cs.
  • Rename KeyEventEventArgs to KeyEventArgs
  • Merge KeyEvent members into KeyEventArgs, delete KeyEvent
  • Rename ProcessKey etc...
  • Fix broken things
  • Add View.ProcessKeyPressed (non-virtual) and move virtual OnKeyPressed from Responder.
  • Remove Shortcut from View as shortcuts are only relevant for Menu
  • Hack Menu and StatusBar to work properly
  • Fully remove OnHot/ColdKey
  • Update API docs
  • Remove Key.BackTab; it's only relevant w/in CursesDriver and confuses the App API
  • Figure out if the whole OnHot/ColdKey thing can be nuked
  • Get rid of KeyEventArgs._keyModifiers and replace IsAlt etc .. with test for mask. - Overly complicates system given benefit (enabled tracking of Caps, scroll, and num lock on WindowsDriver only.
    • Remove IsCapsLock, IsNumLock, and IsScrollLock
    • Remove KeyModifiers class and and change KeyEventArgs to test for the masks in the IsAlt etc... properties.
  • Remove Key.a-z
  • Fix NetDriver
  • Fix CursesDriver
  • Finish Fixes Enable Views to declare key bindings for their SuperView #3042 - Enable Views to declare key bindings for their SuperView
  • Ensure all library code uses consistent naming/usage
  • Ensure all Scenario code uses best practices related to new design.
  • Re-re-edit keyboard.md and API docs

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

I propose to remove the KeyCode.Unknown which aren't used by the drivers now, since they return KeyCode.Null if KeyChar is '\0'. Thus avoids using the prior to check if a key is valid or not and only compare if it's KeyCode.Null.

@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

It's needed many information about Alt, Ctrl and Shift?

image

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

Should methods that invoke events should be protected virtual instead of public virtual? See here https://stackoverflow.com/questions/54914477/why-the-method-that-fires-the-c-sharp-event-must-be-protected-virtual.

I actually went to bed thinking that last night.

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

Based on this, I moved running the test to windows-latest instead of ubuntu-latest. Tests now pass.
microsoft/vstest#2952
Sheesh.

My opinion is that when that vstest issue is fixed then change it again to ubuntu-latest again because normally Windows users forgot to test in WSL first to detect conflicts before pushing them. Don't you think?

I agree. FWIW, the tests DO sometimes fail for me now under WSL:
image

So, this might just be a bad test on our part.

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

I propose to remove the KeyCode.Unknown which aren't used by the drivers now, since they return KeyCode.Null if KeyChar is '\0'. Thus avoids using the prior to check if a key is valid or not and only compare if it's KeyCode.Null.

I agree.

@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

So, this might just be a bad test on our part.

It may have to do with .Net 8.0. Sometimes I see tests that aren't runs, flagged as not run without any information on what going on. This is really a bug after the latest VS2022 update.

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

It's needed many information about Alt, Ctrl and Shift?

image

I think the API docs for each of those makes it clear what they all do. They are all different, and not all are checks.

The idea is Key is a high-level class with helper functions for common things that are either tricky to remember to do correctly, or cause ugly code.

image

The Is properties (e.g. IsShift) test for shift state.

image

The With properties (e.g. WithShift) return a copy of the Key with the shift modifier applied.

image

The No properties (e.g. NoShift) return a copy of the Key with the shift modifier removed.

image

I considered making the No properties be Remove properties (e.g. RemoveShift), but I liked the shortness of No better. Happy to debate this though.

The BareKey is a helper that is not actually used, so I'll remove it.

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

I propose to remove the KeyCode.Unknown which aren't used by the drivers now, since they return KeyCode.Null if KeyChar is '\0'. Thus avoids using the prior to check if a key is valid or not and only compare if it's KeyCode.Null.

I agree.

Note that Key already doesn't have Unknown.

@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

It seems that is a Xunit bug.

image

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

Here's a snapshot of the lastest API docs:

image

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

I'm about to merge this. I have to stop working on it or my personal life is going to hell.

I know this is going to cause hell for the other outstanding PRs.... Sorry.

@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

One question. When the user do With.Shift, With.Ctrl and With.Alt what's KeyCode is used on the Key class?

@tig tig merged commit dcb3b35 into gui-cs:v2_develop Dec 16, 2023
1 check passed
@BDisp
Copy link
Collaborator

BDisp commented Dec 16, 2023

I propose to remove the KeyCode.Unknown which aren't used by the drivers now, since they return KeyCode.Null if KeyChar is '\0'. Thus avoids using the prior to check if a key is valid or not and only compare if it's KeyCode.Null.

I agree.

Note that Key already doesn't have Unknown.

I think it should also been removed from the KeyCode enum.

@tig
Copy link
Collaborator Author

tig commented Dec 16, 2023

This is merged.

FWIW, see the new API docs: https://gui-cs.github.io/Terminal.GuiV2Docs/docs/keyboard.html

@tig tig mentioned this pull request Dec 16, 2023
BDisp added a commit to BDisp/Terminal.Gui that referenced this pull request Dec 29, 2023
…fy (gui-cs#2927)

* Adds basic MainLoop unit tests

* Remove WinChange action from Curses

* Remove WinChange action from Curses

* Remove ProcessInput action from Windows MainLoop

* Simplified MainLoop/ConsoleDriver by making MainLoop internal and moving impt fns to Application

* Modernized Terminal resize events

* Modernized Terminal resize events

* Removed un used property

* for _isWindowsTerminal devenv->wininit; not sure what changed

* Modernized mouse/keyboard events (Action->EventHandler)

* Updated OnMouseEvent API docs

* Using WT_SESSION to detect WT

* removes hacky GetParentProcess

* Updates to fix gui-cs#2634 (clear last line)

* removes hacky GetParentProcess2

* Addressed mac resize issue

* Addressed mac resize issue

* Removes ConsoleDriver.PrepareToRun, has Init return MainLoop

* Removes unneeded Attribute methods

* Removed GetProcesssName

* Removed GetProcesssName

* Refactored KeyEvent and KeyEventEventArgs into a single class

* Revert "Refactored KeyEvent and KeyEventEventArgs into a single class"

This reverts commit 88a0065.

* Fixed key repeat issue; reverted stupidity on 1049/1047 confusion

* Updated CSI API Docs

* merge

* Rearranged Event.cs to Keyboard.cs and Mouse.cs

* Renamed KeyEventEventArgs KeyEventArgs

* temp renamed KeyEvent OldKeyEvent

* Merged KeyEvent into KeyEventArgs

* Renamed Application.ProcessKey members

* Renamed Application.ProcessKey members

* Renamed Application.ProcessKey members

* Added Responder.KeyPressed

* Removed unused references

* Fixed arg naming

* InvokeKeybindings->InvokeKeyBindings

* InvokeKeybindings->InvokeKeyBindings

* Fixed unit tests fail

* More progress on refactoring key input; still broken and probably wrong

* Moved OnKeyPressed out of Responder and made ProcessKeyPrssed non-virtual

* Updated API docs

* Moved key handling from Responder to View

* Updated API docs

* Updated HotKey API docs

* Updated shortcut API docs

* Fixed responder unit tests

* Removed Shortcut from View as it is not used

* Removed unneeded OnHotKey override from Button

* Fixed BackTab logic

* Button now uses Key Bindings exclusively

* Button now uses Key Bindings exclusively

* Updated keyboard.md docs

* Fixed unit tests to account for Toplevel handling default button

* Added View.InvokeCommand API

* Modernized RadioGroup

* Removed ColdKey

* Modernized (partially) StatusBar

* Worked around FileDialog issue with Ctrl-F

* Fixed driver unit test; view must be focused to reciev key pressed

* Application code cleanup

* Start on updaing menu

* Menu now mostly works

* Menu Select refinement

* Fixed known menu bugs!

* Enabled HotKey to cause focus- experimental

* Fixes gui-cs#3022 & adds unit test to prove it

* Actually Fixes gui-cs#3022 & adds unit test to prove it

* Working through hotkey issues

* Misc fixes

* removed hot/cold key stuff from Keys scenario

* Fixed scenarios

* Simplified shortcut string handling

* Modernized Checkbox

* Modernized TileView

* Updated API docs

* Updated API docs

* attempting to publish v2 docs

* Revert "attempting to publish v2 docs"

This reverts commit 59dcec1.

* Playing with api docs

* Removed Key.BackTab

* Removed Caps/Scroll/Numlock

* Partial removal of keymodifiers - unit tests pass

* Partial removal of keymodifiers - broke netdriver somewhere

* WindowsDriver & added KeyEventArgsTests

* Fixing menu shortcut/hotkeys - broke Menu.cs into separate files

* Fixed MenuBar!

* Finished modernizing Menu/MenuBar

* Removed Key.a-z. Broke lots of stuff

* checkout@v4

* progress on key mapping and formatting

* VK tests are still failing

* Fixed some unit tests

* Added Hotkey and Keybinding unit tests

* fixed unit test

* All unit tests pass again...

* Fixed broken unit tests

* KeyEventArgs.KeyValue -> AsRune

* Fixed bugs. Still some broken

* Added KeyEventArgs.IsAlpha. Added KeyEventArgs.cast ops. Fixed bugs. Unit tests pass

* Fixed WindowsDriver

* Oops.

* Refactoring based on bdisp's help. Not complete!

* removed calling into subviews from OnKeyBindings

* removed calling into subviews from OnKeyBindings

* Improved View KeyEvent unit tests

* More hotkey unit tests

* BIg change - Got rid of KeyPress w/in Application/Drivers

* Unit tests now pass again

* Refreshed API docs

* Better HotKey logic. More progress. Getting close.

* Fixed handling of shifted chars like ö

* Minor code cleanup

* Minor code cleanup2

* Why is build Action failing?

* Why is build Action failing??

* upgraded to .net8 to try to fix weird CI/CD build errors

* upgraded to .net8 to try to fix weird CI/CD build errors2

* Disabling TextViewTests to diagnose build errors

* reenable TextViewTests to diagnose build errors

* Arrrrrrg

* Merged v2_develop

* Fixed uppercase accented keys in WindowsDriver

* Fixed key binding api docs

* Experimental impl of CommandScope.SubViews for MenuBar

* Removed dead code from application.cs

* Removed dead code from application.cs

* Removed dead code from ConsoleDriver.cs

* Cleaned up some key binding stuff

* Disabled Alt to activate menu for now

* Updated label commands

* Fixed menu bugs. Upgraded menu unit tests

* Fixed unit tests

* Working on NetDriver

* fixed netdriver

* Fixed issues called out by @BDisp CR

* fixed CursesDriver

* added todo to netdriver

* Cherry picked treeview test fix 1b415e5

* Fix NetDriver.

* CommandScope->KeyBindingScope

* Address some tznind feedback

* Refactored KeyBindings big time!

* Added key consts to KeyEventArgs and renamed Key to ConsoleDriverKey

* Fixed some API docs

* Moved ConsoleDriverKey to ConsoleDriver.cs

* Renamed Key->ConsoleDriverKey

* Renamed Key->ConsoleDriverKey

* Renamed Key->ConsoleDriverKey

* renamed file I forgot to rename before

* Updated name and API docs of KeyEventArgs.isAlpha

* Fixed issues with OnKeyUp not doing the right thing.

* Fixed MainLoop.Running never being used

* Fixed MainLoop.Running never being used - unit tests

* Claned up BUGBUG comments

* Disabled a unit test to see why ci/cd tests are failing

* Removed defunct commented code

* Removed more defunct commented code

* Re-eanbled unit test; jsut removing one test case...

* Disabled more...

* Renambed Global->Applicaton and updated scope API docs

* Disabled more unit tests...

* Removed dead code

* Disabled more unit tests...2

* Disabled more unit tests...3

* Renambed Global->Applicaton and updated scope API docs 2

* Added more KeyBinding scope tests

* Added more KeyBinding scope tests2

* ConsoleDriverKey too long. Key too ambiguous. Settled on KeyCode. (Partialy because eventually I want to intro a class named Key).

* KeyEventArgs improvements. cast to Rune must be explicit as it's lossy

* Fixed warnings

* Renamed KeyEventArgs to Key... progress on fixing broken stuff that resulted

* Fix ConsoleKeyMapping bugs.

* Fix NetDriver issue from converting a lower case to a upper case.

* Started migration to Key from KeyCode - e.g. made HotKeys all consistent.

* Fixed build warnings

* Added key defns to Key

* KeyBindings now uses Key vs. KeyCode

* Verified by tweaking UICatalog

* Fixed treeview test ... again

* Renamed ProcessKeyDown/Up to NewKeyDown/Up and OnKeyPressed to OnProcessKeyDown to make things more clear

* Added test AllViews_KeyDown_All_EventsFire unit tests and fixed a few Views that were wrong

* fixed stupid KeyUp event bug

* If key not handled, return false for datefield

* dotnet test --no-restore --verbosity diag

* dotnet test --blame

* run tests on windows

* Fix TestVKPacket unit test and move it to ConsoleKeyMappingTests.cs file.

* Remove unnecessary commented code.

* Tweaked unit tests and removed Key.BareKey

* Fixed little details and updated api docs

* updated api docs

* AddKeyBindingsForHotKey: KeyCode->Key

* Cleaned up more old KeyCode usages. Added TODOs

---------

Co-authored-by: BDisp <bd.bdisp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 For discussions, issues, etc... relavant for v2
Projects
No open projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants