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.

Sign up for GitHub

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

Jump to bottom

[Vi-Mode] Supports di' and di" commands (quoted text objects) #3791

Closed
wants to merge 2 commits into from
Closed

[Vi-Mode] Supports di' and di" commands (quoted text objects) #3791

wants to merge 2 commits into from

Conversation

springcomp
Copy link
Contributor

@springcomp springcomp commented Aug 23, 2023 •
edited by microsoft-github-policy-service bot

PR Summary

Fixes #3790.

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
    • Not Applicable
    • OR
    • Documentation needed at PowerShell-Docs
      • Doc Issue filed:

As #2059 has been readily accepted, we have not had the time to discuss the documentation.
This PR, like #2059, wire keybindings to private methods. Please, let me know how to best document this behaviour.

Microsoft Reviewers: Open in CodeFlow

@springcomp springcomp mentioned this pull request Aug 27, 2023
Open
daxian-dbw
daxian-dbw requested changes Oct 23, 2023
View reviewed changes
Copy link
Member

@daxian-dbw daxian-dbw left a comment

Choose a reason for hiding this comment

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

Before moving forward with more 3-key chord supports, we need to first figure out how to make Get/Set-PSReadLineKeyHandler support chords with more than 2 keys. Today, _chordDispatchTable is a 2-level dictionary, and hence it only really supports 2-key chords. Get/Set-PSReadLineKeyHandler are implemented under that assumption.

By looking at #2059 again, I think I merged it a little too prematurely :)
Today, we have two 3-key chords built-in:

There are 2 concerns here:

  1. Their implementation should be consistent in the way how the 3rd level dictionary is stored.
  2. Changes to VI key binding data structure are needed to make Get/Set-PSReadLineKeyHandler to support chords with 3 keys (are we stopping at 3-key chords for VI mode here? maybe we should and not support chords with more than 3 keys).

I think we should first resolve the above concerns before adding more text object commands.

PSReadLine/StringBuilderTextObjectExtensions.cs
/// starting from the specified "current" position.
/// </summary>
/// <param name="current">The position in the current logical line.</param>
internal static int GetBeginningOfLogicalLinePos(this StringBuilder buffer, int current)
Copy link
Member

Choose a reason for hiding this comment

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

It seems GetBeginningOfLogicalLinePos and GetEndOfLogicalLinePos shoudl be put in StringBuilderLinewiseExtensions instead.

PSReadLine/StringBuilderTextObjectExtensions.cs
@@ -1,4 +1,5 @@
using System;
using System.Management.Automation;
Copy link
Member

Choose a reason for hiding this comment

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

Added by mistake? It doesn't seem to be needed by the code changes in this file.

@daxian-dbw
Copy link
Member

daxian-dbw commented Oct 23, 2023 •
edited

BTW, the <d,i> shows up in the results of Get-PSReadLineKeyHandler:

User defined functions
======================

Key   Function          Description
---   --------          -----------
<d,i> ChordViTextObject ChordViTextObject

Two problems here:

  1. ChordViTextObject is not put in GetDisplayGrouping(string function), so it's displayed as User defined functions.
  2. The chord <d,i> is incomplete -- users will try using it but observe no result (or unexpected result).

It's the same for <d,g>:

Miscellaneous functions
=======================

Key        Function          Description
---        --------          -----------
<d,g>      ViDGChord         ViDGChord

@springcomp springcomp mentioned this pull request Oct 31, 2023
Draft
6 tasks
@springcomp
Copy link
Contributor Author

@daxian-dbw I have published #3853 to address your feedback, re-working #2059 in the process.
I’m happy for this current PR to be closed as I will be able to move the code forward in a better shape if #3853 is accepted.

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

@daxian-dbw daxian-dbw daxian-dbw requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[vi-mode] Support di' and di" commands
2 participants
@springcomp @daxian-dbw