-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
Spec for the Suggestions UI #14864
Spec for the Suggestions UI #14864
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Or consider the Suggestions UI when invoked by [shell-driven autocompletion]. | ||
The shell might want to provide help text to the user with each of the | ||
suggestions. This would allow a user to browse through the suggestions that they | ||
might not know about, and learn how they work before committing to one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For .NET command-line applications, the System.CommandLine.Completions.CompletionItem API already supports help text, but the library does not yet provide a protocol with which a shell could retrieve that information (dotnet/command-line-api#1220).
This comment has been minimized.
This comment has been minimized.
How should we handle completions here? Tab? Enter? Right-Arrow? Should we have | ||
an element selected when we open the menu, or should tab/enter only work once | ||
the user has used the arrows at least once? Sublime allows for <kbd>tab</kbd> to | ||
complete the suggestion immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I envisioned it like this:
- PowerShell has a tab completion setting. One of the options would be to enable this.
- When the user presses tab, the suggestions UI pops up as a menu of options.
- up/down to navigate. Enter to apply. (copy VS experience, I believe)
Any other input closes the menu and passes the input through to the terminal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urgh, I generally use MenuComplete, and you do make a good point in the next paragraph. Tab is used to navigate through the menu. Typing more text further filters the list. But right-arrow navigates the menu.
I think the behavior is different when you use the default though, so it's probably ok to break UX pattern?
I guess I'd also add to the list above...
- typing text injects it to the shell, but filters the list in the UI (it refreshes)
- ESC closes the UI but does NOT go through to the shell (otherwise it might clear the input line and that sucks!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it's currently implemented:
- In menu mode, right-arrow, tab, and enter all "accept completion". Esc dismisses the UI (but doesn't send it through). Any other key dismisses the UI AND sends it through.
- In palette mode, right-arrow (at the end of the filter text) and enter both "accept completion". Esc dismisses the UI (but doesn't send it through). Other keys go into the filter text (like the command palette).
It feels good, but maybe I'm too close to it at this point.
I could be convinced that right-arrow could be dropped from the "accept" list (esp since pwsh uses that for its inline predictions)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, here's a weird thing. What if we swung hard the other way and said "the client application is always in control of it?" It's a surface we provide just like a text buffer, but they receive all the commands, manage the selection, etc.
This draws it closer to Carlos' VT Aria spec. Is that a good thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I guess the Suggestions UI would only be relevant here when the underlying client app is opting-in via VT. I like the possibility of giving them full control over that; that makes it more versatile. I'm down for that.
Also, have you spoken with possible consumers about this (i.e. PowerShell)? Do they have any preference?
* If the user hasn't enabled shell completion, we could add text to the | ||
`commandHistory` or `directoryHistory` menus to inform the user how they could | ||
go enable shell integration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo, this should be a part of "crawl" before the feature releases. I'm concerned about the discoverability of this feature. Yes, the feature is in the command palette by default, but most/all of them won't work unless they have shell integration enabled. So we need users to (1) realize that the feature can be invoked via the command palette and (2) register shell integration for the suggestions UI to be useful.
Doing this at least addresses the second point above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a note as to why this is Hard and why we're punting that for now.
* The app layer could easily translate between sendInput actions and these | ||
pseudo-actions. | ||
|
||
#### Description Tooltips |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm conflicted here. I want descriptions to be presented (at least optionally determined by the user). I don't think teaching tips are the way to go. They appear briefly and they're not really accessible.
I have a few proposals:
- each entry in the listview is actually 2 lines of text: (1) the suggestion itself and (2) a description, if available
- the description would be presented in a new line in a smaller font size
- a setting would disable descriptions being presented (this is more for accessibility, because it'd be annoying to have the descriptions read out every time imo)
- this would allow "more info" to be opt-in via the settings
- add a button and/or keybinding when in the suggestions UI to "ask for more information"
- if we kept the teaching tip UX, the teaching tip would only appear when this button/keybinding is invoked
- [accessibility] when the button/keybinding is invoked, we could dispatch a UIA notification with the description, forcing the screen reader to read out the description
- this would allow "more info" to be opt-in at runtime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW this gif is terrible and I came up with a better version.
- That has to be reconciled with Don't localize command names on the command palette #7039
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
> why it is in the "Future considerations" section. I think it is worth | ||
> mentioning. This might be better served in the [shell integration] doc._ | ||
|
||
We'll probably want a way for recent commands to be saved across sessions. That way, your `cmd.exe` command history could persist across sessions. We'd need: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(thinking out loud)
This could be useful for things like portable mode, ssh sessions, and cross-machine scenarios. I could be convinced.
On the other hand, this is also a bit dangerous in that you might get a command that straight up won't work in any of the scenarios mentioned above. And that I don't like.
This would seemingly SUPER conflict with PowerShell's own handler. Probably not | ||
something someone should enable for PowerShell 7 profiles if they're using that | ||
feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this would have to be enabled via PowerShell's profile as a KeyHandler for tab completion. That would output an escape sequence that would force WT to open the suggestions UI in inline mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also something deeper shell integration could give us. But then, what is it doing that the shell isn't?
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Despite all the comments, I'm approving with suggestions. Excellent write-up, thanks for taking the time to do it!
|
||
It will delight developers. | ||
|
||
Furthermore, our partners on the Visual Studio team have been requesting similar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VS: requesting; VSCode: working on!
menu in Visual Studio. That's a UI that only allows for up/down for navigation | ||
(and enter/tab for selecting the suggestion). | ||
|
||
For suggestions driven by the Terminal, we'll display a filtering text box in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's an interesting case! Would we ever allow extensions to augment the shell integration driven suggestions? If so, that's a hybrid mode -- should it have a filtering text box?
> **Warning** | ||
> TODO! For discussion, possibly with a real UX designer. | ||
|
||
How should we handle completions here? Tab? Enter? Right-Arrow? Should we have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX affordance required for showing the user how to submit it, too. Something like... Foo bar baz [enter]
How should we handle completions here? Tab? Enter? Right-Arrow? Should we have | ||
an element selected when we open the menu, or should tab/enter only work once | ||
the user has used the arrows at least once? Sublime allows for <kbd>tab</kbd> to | ||
complete the suggestion immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, here's a weird thing. What if we swung hard the other way and said "the client application is always in control of it?" It's a surface we provide just like a text buffer, but they receive all the commands, manage the selection, etc.
This draws it closer to Carlos' VT Aria spec. Is that a good thing?
> **Note** | ||
> In my prototype, for the "Menu" mode, I accepted ALL of right-arrow, tab, and | ||
> enter as "accept completion", and any other key dismissed the UI. This _felt_ | ||
> right for that mode. I'm not sure we could make the same call for "palette" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These few paragraph argue against eachother. Suggestion: Make a call or lay them out in a closing paragraph as open options
#### Automatic shell integration | ||
|
||
A large portion of these features all rely on shell integration being enabled by | ||
the user. However, this is not a trivial thing for the Terminal to do on behalf | ||
of the user. Shell integration relies on changes to the user's shell config. If | ||
the Terminal were to try and configure those itself, we may accidentally destroy | ||
configuration that the user has already set up. Hence why the Terminal can't | ||
just have a "Light up all the bells and whistles" toggle in the Settings UI. | ||
|
||
This is a non-trivial problem to solve, so it is being left as a future | ||
consideration. It deserves its own spec to sort out how we should expose this to | ||
users and safely implement it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section may belong in the broader Shell Integration spec rather than in here.
This would seemingly SUPER conflict with PowerShell's own handler. Probably not | ||
something someone should enable for PowerShell 7 profiles if they're using that | ||
feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also something deeper shell integration could give us. But then, what is it doing that the shell isn't?
Furthermore, async sources wouldn't work with sync | ||
ones, at all. E.g. if you did `source: ["tasks", "myAsyncSource"]`. It doesn't | ||
make sense to start with a list of `tasks`, then type, find no tasks, but then | ||
oh! the UI fills in some other suggestions too. That's weird. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Psychic debugging: you wrote this bullet point, and then later wrote a whole section on it! You may be able to streamline the doc by removing this in favor of the section below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed with Mike offline. Approved :)
> **Warning** | ||
> TODO! For discussion, possibly with a real UX designer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this TODO stuff should get resolved before merging.
> **Note** | ||
> In my prototype, for the "Menu" mode, I accepted ALL of right-arrow, tab, and | ||
> enter as "accept completion", and any other key dismissed the UI. This _felt_ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to keep this in the final spec?
![](tasks-suggestions.gif) | ||
|
||
(admittedly, the `TeachingTip` in that gif is a prototype and was later replaced | ||
with a better version.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we update this gif before merging?
How should we handle completions here? Tab? Enter? Right-Arrow? Should we have | ||
an element selected when we open the menu, or should tab/enter only work once | ||
the user has used the arrows at least once? Sublime allows for <kbd>tab</kbd> to | ||
complete the suggestion immediately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm... I guess the Suggestions UI would only be relevant here when the underlying client app is opting-in via VT. I like the possibility of giving them full control over that; that makes it more versatile. I'm down for that.
Also, have you spoken with possible consumers about this (i.e. PowerShell)? Do they have any preference?
user won't be pressing tab to tab-complete at the shell - the focus is out of | ||
the of terminal and in the Suggestions UI. With the menu version, the focus is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the focus is out of the of terminal
that doesn't read right
We will want to make sure that there's some semblance of consistency across our | ||
implementation for the Suggestions UI, our own Command Palette, VsCode's | ||
intellisense and their own implementation of shell-completions in the Terminal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add an analysis of those experiences to this spec?
"sources". As an example, consider the following actions: | ||
|
||
```json | ||
{ "command": { "action":"suggestions", "source": "commandHistory" } }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
second!
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 * Doesn't actually close anything * Heavily related to #3121, but I'm not gonna say that's closed till we settle on the protocol * See also: * #1595 * #14779 * microsoft/vscode#171648 ### 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 <kbd>Ctrl+space</kbd> to _essentially_ send <kbd>F12</kbd><kbd>b</kbd>. Wacky, but it works. ```json { "command": { "action": "sendInput","input": "\u001b[24~b" }, "keys": "ctrl+space" }, ``` ```ps1 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 * [x] `(prompt | format-hex).`<kbd>Ctrl+space</kbd> -> 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?
This adds support for a new action, `showSuggestions`, as described in #14864. This adds just one `source` currently, `recentCommands`. This requires shell integration to be enabled in the shell to work properly. When it is enabled, activating that action will invoke the suggestions UI as a palette, populated with `sendInput` actions for each of the user's recent commands. * These don't persist across reboots. * These are per-control. There's mild plans to remedy that in a follow-up, though that needs a bit more design consideration. Closes #14779
_targets #14943_ When this is true, this will re-use the existing commandline to pre-filter the results. This is especially helpful for completing a suggestion based on the text that's already been typed. Like with command history, this requires that shell integration is enabled before it will work.## Summary of the Pull Request ## References and Relevant Issues See also #13445 As spec'd in #14864 ## Validation Steps Performed Tested manually
_targets #15027_ Adds a new suggestion source, `tasks`, that allows a user to open the Suggestions UI with `sendInput` commands saved in their settings. `source` becomes a flag setting, so it can be combined like so: ```json { "keys": "ctrl+shift+h", "command": { "action": "suggestions", "source": "commandHistory", "useCommandline":true }, }, { "keys": "ctrl+shift+y", "command": { "action": "suggestions", "source": "tasks", "useCommandline":false }, }, { "keys": "ctrl+shift+b", "command": { "action": "suggestions", "source": ["all"], "useCommandline":true }, }, ``` If a nested command has `sendInput` commands underneath it, this will build a tree of commands that only include `sendInput`s as leaves (but leave the rest of the nesting structure intact). ## References and Relevant Issues Closes #1595 See also #13445 As spec'd in #14864 ## Validation Steps Performed Tested manually
Summary of the Pull Request
Detailed Description of the Pull Request / Additional comments
*** read the spec ***
Similar to #14792, a lot of this code is written. This stuff isn't checked in though, so I'm presenting formally before I start yeeting PRs out there.
PR Checklist
sendInput
to have promptable sections for further completion #12927