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

Add Suggestions UI & experimental shell completions support #14938

Merged
merged 60 commits into from
Aug 14, 2023

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Mar 1, 2023

There's two parts to this PR that should be considered separately.

  1. The Suggestions UI, a new graphical menu for displaying suggestions / completions to the user in the context of the terminal the user is working in.
  2. The VsCode shell completions protocol. This enables the shell to invoke this UI via a VT sequence.

These are being introduced at the same time, because they both require one another. However, I need to absolutely emphasize:

THE FORMAT OF THE COMPLETION PROTOCOL IS EXPERIMENTAL AND SUBJECT TO CHANGE

This is what we've prototyped with VsCode, but we're still working on how we want to conclusively define that protocol. However, we can also refine the Suggestions UI independently of how the protocol is actually implemented.

This will let us rev the Suggestions UI to support other things like tooltips, recent commands, tasks, INDEPENDENTLY of us rev'ing the completion protocol.

So yes, they're both here, but let's not nitpick that protocol for now.

Checklist

Detailed Description

Suggestions UI

The Suggestions UI is spec'ed over in #14864, so go read that. It's basically a transient Command Palette, that floats by the user's cursor. It's heavily forked from the Command Palette code, with all the business about switching modes removed. The major bit of new code is SuggestionsControl::Anchor. It also supports two "modes":

  • A "palette", which is like the command palette - a list with a text box
  • A "menu", which is more like the intellisense flyout. No text box. This is the mode that the shell completions use

Shell Completions Protocol

I literally cannot say this enough times - this protocol is experimental and subject to change. Build on it at your own peril. It's disabled in Release builds (but available in preview behind globals.experimental.enableShellCompletionMenu), so that when it ships, no one can take a dependency on it accidentally.

Right now we're just taking a blob of JSON, passing that up to the App layer, who asks Command to parse it and build a list of sendInput actions to populate the menu with. It's not a particularly elegant solution, but it's good enough to prototype with.

How do I test this?

I've been testing this in two parts. You'll need a snippet in your powershell profile, and a keybinding in the Terminal settings to trigger it. The work together by binding Ctrl+space to essentially send F12b. Wacky, but it works.

{ "command": { "action": "sendInput","input": "\u001b[24~b" }, "keys": "ctrl+space" },
function Send-Completions2 {
  $commandLine = ""
  $cursorIndex = 0
  # TODO: Since fuzzy matching exists, should completions be provided only for character after the
  #       last space and then filter on the client side? That would let you trigger ctrl+space
  #       anywhere on a word and have full completions available
  [Microsoft.PowerShell.PSConsoleReadLine]::GetBufferState([ref]$commandLine, [ref]$cursorIndex)
  $completionPrefix = $commandLine

  # Get completions
  $result = "`e]633;Completions"
  if ($completionPrefix.Length -gt 0) {
    # Get and send completions
    $completions = TabExpansion2 -inputScript $completionPrefix -cursorColumn $cursorIndex
    if ($null -ne $completions.CompletionMatches) {
      $result += ";$($completions.ReplacementIndex);$($completions.ReplacementLength);$($cursorIndex);"
      $result += $completions.CompletionMatches | ConvertTo-Json -Compress
    }
  }
  $result += "`a"

  Write-Host -NoNewLine $result
}

function Set-MappedKeyHandlers {
  # VS Code send completions request (may override Ctrl+Spacebar)
  Set-PSReadLineKeyHandler -Chord 'F12,b' -ScriptBlock {
    Send-Completions2
  }
}

# Register key handlers if PSReadLine is available
if (Get-Module -Name PSReadLine) {
  Set-MappedKeyHandlers
}

TODO

  • (prompt | format-hex).Ctrl+space -> This always throws an exception. Seems like the payload is always clipped to
    {"CompletionText":"Ascii","ListItemText":"Ascii","ResultType":5,"ToolTip":"string Ascii { get
    and that ain't JSON. Investigate on the pwsh side?

zadjii-msft and others added 27 commits April 28, 2022 16:14
…Ev-DaYs' into dev/migrie/fhl-2023/pwsh-autocomplete-demo
(cherry picked from commit a172f53)
@zadjii-msft
Copy link
Member Author

gh-3121-for-pr

@github-actions

This comment has been minimized.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

43/44! I haven't reviewed the changes to CommandPalette since they confuse me.

src/cascadia/TerminalApp/CommandPalette.cpp Outdated Show resolved Hide resolved
<UserControl.Resources>
<ResourceDictionary>
<!-- This creates an instance of our CommandKeyChordVisibilityConverter we can reference below -->
<local:EmptyStringVisibilityConverter x:Key="CommandKeyChordVisibilityConverter" />
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Also, FWIW, I don't believe we need a separate one of these for every place we might use it

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't? Like, the ParentCommandVisibilityConverter we do still need. I assumed since it was in the UserControl.Resources, that it couldn't be accessed from another control outside of it.

Oh but we could just stick it in like, the App.Resources and have a singleton instance of it, huh.

src/cascadia/TerminalApp/SuggestionsControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/SuggestionsControl.xaml Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/terminal/adapter/adaptDispatch.cpp Show resolved Hide resolved
const auto coreWindow = CoreWindow::GetForCurrentThread();
const auto ctrlDown = WI_IsFlagSet(coreWindow.GetKeyState(winrt::Windows::System::VirtualKey::Control), CoreVirtualKeyStates::Down);

if (key == VirtualKey::Home && ctrlDown)
Copy link
Member

Choose a reason for hiding this comment

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

That doesn't necessarily mean that we shouldn't do both, just that we shouldn't do both in this PR

{
_updateFilteredActions();
}
else // THIS BRANCH IS NEW
Copy link
Member

Choose a reason for hiding this comment

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

new where exactly? this whole file is new!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, that one is new relative to the command palette's code. That comment is useless though... but why did I add that?

@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 8, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 8, 2023
src/cascadia/TerminalControl/ControlCore.cpp Show resolved Hide resolved
src/cascadia/TerminalCore/Terminal.hpp Show resolved Hide resolved
replacementLength);
// Get the combined lengths of parts 0-3, plus the semicolons. We
// need this so that we can just pass the remainder of the string.
const auto prefixLength = til::at(parts, 0).size() + 1 +
Copy link
Member

Choose a reason for hiding this comment

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

I'm mostly horrified that SplitString doesn't have a count limit (but you don't need to add one)

til::at(parts, 1).size() + 1 +
til::at(parts, 2).size() + 1 +
til::at(parts, 3).size() + 1;
if (prefixLength > string.size())
Copy link
Member

Choose a reason for hiding this comment

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

Because of the additional +1 above, won't this make the condition on line 3812 (parts.size) always false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah, we need 5 parts:

  • completions
  • replacementIndex
  • replacementLength
  • cursorIndex
  • the payload

maybe I'm just not mentally parsing why the extra +1 to account for the semi after the cursorIndex would cause that

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I get it now. I think i was reading it wrong before.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, no, yeah. Here's the deal.

1;2;3;4

string.size == 7

If you consume parts 0-3, that gives you a prefix length of 8 (the length of each part, plus 1 for each part to account for the semicolon)

You fail out here because string is too short. you are expecting 5 parts.

The same is true if the string is 1;2;3;4;, string.size==8 and you fail out here.

My contention is that you're checking parts.size() on line 3812, when you have all but guaranteed that there are five parts. i.e. the code looks like it was written to support "no payload", but the code before you get there enforces "yes payload" in a different way.

src/terminal/parser/OutputStateMachineEngine.cpp Outdated Show resolved Hide resolved
@DHowett
Copy link
Member

DHowett commented Aug 10, 2023

Still concerned about building on their protocol camping on their OSC, but since it is very experimental it's probably OK.

@DHowett
Copy link
Member

DHowett commented Aug 10, 2023

You have bare unchecked TODOs in the body of this PR. I was about to merge, but I didn't want them in there unchecked--the merge message should be relatively complete. Is this PR ready? Are the TODOs relevant?

@zadjii-msft
Copy link
Member Author

You have bare unchecked TODOs in the body of this PR

Ah, I BET that was the "; in the JSON" issue you called out earlier, because that works now!

@zadjii-msft zadjii-msft merged commit a0c88bb into main Aug 14, 2023
17 of 19 checks passed
@zadjii-msft zadjii-msft deleted the dev/migrie/fhl-2023/pwsh-autocomplete-demo branch August 14, 2023 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants