Skip to content

Conversation

tznind
Copy link
Collaborator

@tznind tznind commented Aug 25, 2022

Fixes #1962 - Allow specifying more than 1 Command to order when using AddKeyBinding

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

@tznind
Copy link
Collaborator Author

tznind commented Aug 25, 2022

Needs more tests (hence Draft) but I think that this is what 'command chaining' would look like.

I've changed AddKeyBinding from taking Command to taking params Command[] rather than adding overloads. This means all existing code written will be compatible but might not be binary compatible (e.g. dll binding redirects to new version with code compiled against old version)

It also means a user could specify no Command objects at all (params are optional) which would be wierd... probably needs a test but isn't going to break the world.

@BDisp
Copy link
Collaborator

BDisp commented Aug 25, 2022

Needs more tests (hence Draft) but I think that this is what 'command chaining' would look like.

I like your solution for this.

I've changed AddKeyBinding from taking Command to taking params Command[] rather than adding overloads. This means all existing code written will be compatible but might not be binary compatible (e.g. dll binding redirects to new version with code compiled against old version)

No big deal, compiling again solves it.

It also means a user could specify no Command objects at all (params are optional) which would be wierd... probably needs a test but isn't going to break the world.

I think is better to throw an exception if the command is null or was not passed no command at all.

@tznind tznind marked this pull request as ready for review August 27, 2022 07:26
Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, tidy, work.

@tig tig merged commit 8a90453 into gui-cs:develop Aug 30, 2022
public void ClearKeybinding (params Command [] command)
{
foreach (var kvp in KeyBindings.Where (kvp => kvp.Value == command).ToArray ()) {
foreach (var kvp in KeyBindings.Where (kvp => kvp.Value.SequenceEqual (command))) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just looking again at this code and see that we lost the .ToArray(). I thought this would cause the old 'Collection modified during enumeration' error but the test passes which is wierd.

I looked into why and it seems maybe the behaviour changed as of .netcore3.1. I will open a quick PR to put it back incase this would break when running on an older runtime.

Ideally, it is not advisable to do it. The GetEnumerator() that is called while iterating to the next item will throw an InvalidOperationException. However, from .netcore 3.1 onwards, this will work when you do a dictionary.Remove() or dictionary.Clear() from inside the loop.

https://stackoverflow.com/a/67650242/4824531

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support "Auto Move To Next Item" in ListView

3 participants