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

Refactoring Key Chords Handling #3853

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

springcomp
Copy link
Contributor

@springcomp springcomp commented Oct 31, 2023

PR Summary

This PR refactors, the handling of key chords and consolidates the behaviour from Emacs and VI modes into a single consistent function.

This PR addresses freedback made on #3791 following #2059 which has been merged a bit too optimistically.

PR Content

In order to achieve a consistent behaviour, the Chord Dispatch Table structures are refactored using a KeyHandlerOrChordDispatchTable class that can hold either a regular KeyHandler or a nested ChordDispatchTable. This means that chord dispatch tables are organized in a tree structure and looking up a key handler is a simple matter of walking down the tree structure.

This allows to greatly simplify the handling of VI-mode three key-chord commands, such as d, g, g (<d,g,g> delete relative lines) or d, i, w (<d,i,w> delete inner word)

This allows to remove all the references to a secondary dispatch table when handling key chords in favor of a more consistent algorithm that walks through the chord dispatch table tree structure and identifies whether the next node is a leaf (i.e a KeyHandler) or another nested node (i.e an inner-level chord dispatch table).

Note in #3791 was discussed the fact that we may want to limit key chords to a maximum of three keys. This PR does not enforce any limit. Maybe such a limit could be hardcoded in the PSConsoleReadLine.SetKeyHandler() CmdLet implementation.

This PR consolidates handling of key chords in the ProcessOneKey function – which could be renamed ProcessKeyChord. Most of the current implementations were using recursive code – albeit, indirectly by calling either the Chord or the ViChord method. In contrast, this PR almost eliminates recursive code in favor of walking the tree structure. However, handling of VI-mode digit-arguments is a bit complex and I was not able to solve this without resorting to recursive code. This make the ProcessOneKey function a bit hard to understand. I have tried adding as much comments as I could think of.

The unit-tests sometimes randomly fail on my machine, but that was already the case with the current code in the master branch before this PR. Sometimes, unit-tests fail as part of running the whole test suite. When running one failing test again it then succeeds. Finally, most tests around history fail on my machine but – again – that is already the case with the current code in the master branch 🤔.

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • Make sure you've tested these changes in terminals that PowerShell is commonly used in (i.e. conhost.exe, Windows Terminal, Visual Studio Code Integrated Terminal, etc.)
  • User-facing changes
Microsoft Reviewers: Open in CodeFlow

{
Assert.Equal("Ctrl+x", handler.Key);
}
Assert.Empty(handlers);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I can see, this is the only behaviour change. Previously to this PR, a partial chord would return ChordFirstKey as its handler.

This PR eliminates this indirection and does not return any handler for partial chords. Only handlers at the leaf are returned.


private static void ViDeleteInnerWord(ConsoleKeyInfo? key = null, object arg = null)
/// <summary>
/// Delete the content inside and including the current word
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ViDeleteInnerWord method is now wired directly form the chord dispatch table and has been made public so that it can be remapped by the user.

All the complex wiring to ultimately call this function on <d,i,w> has been eliminated away, in favor of simply wiring this method as a handler in the appropriate location in the chord dispatch table.

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.

1 participant