Add Spec for Quick Fix #17005
Add Spec for Quick Fix #17005
Conversation
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
doc/specs/#16599 - Quick Fix.md
Outdated
| The WinGet API to convert the given `missingCmd` into a list of suggestions may be relatively slow as we have to query the WinGet database. Performing the search when the quick fix menu is open would not be ideal as the user may have to wait for WinGet to finish searching. Instead, the conversion should happen around the time that the prompt is introduced (whether from output or a reflow scenario). | ||
|
|
||
| ## UI/UX Design | ||
| A Quick Fix icon will be displayed immediately left of the prompt. If the icon fits in the left-padding region (a `padding` value where the left side is 16 or greater), it will be drawn there. Otherwise, a thin accent-colored vertical bar will be displayed overlapping the text region; when the icon is hovered over, the icon will appear more clear by becoming more opaque and enlarging to overlap with the text region. |
There was a problem hiding this comment.
accent color is an interesting choice - why that over like, the foreground color? Or yellow from the scheme's color table?
There was a problem hiding this comment.
Personally, I feel like accent color is a good way to signify that the UI is personalized to the user from by the system (vs color scheme colors being a part of the standard theming). Accent colors just pop more, imo.
There was a problem hiding this comment.
@zadjii-msft here's the UI I have from my branch.
Normal version:
Collapsed version:
I did change it so that it's not just based on the "padding". It's a little more complicated than that, but basically we check if the button would "fit" in the gutter (with the icon being a similar size to the buffer font size).
There was a problem hiding this comment.
@zadjii-msft here's the UI I have from my branch.
Looks slick! One question though - is the button supposed to show up on the prompt with the error in it? The GIF shows the button showing up on the next empty prompt line, which IMO is a little unintuitive
There was a problem hiding this comment.
Yup! Intentional to align with VS Code's design. I think it's so that if you had a lot of output, you don't have to scroll all the way back up to get to the quick fix menu.
There was a problem hiding this comment.
That makes sense! Should the suggestion get input into that empty line then? Like in the image below. (Not sure if that's just a demo bug, just making sure I'm not missing it if it's in the spec!)
There was a problem hiding this comment.
eh, it's probably because of the secret ^C we insert before the command to make sure it doesn't conflict with input the user has already typed
There was a problem hiding this comment.
eh, it's probably because of the secret ^C we insert before the command to make sure it doesn't conflict with input the user has already typed
Yup, exactly that. We're always sending over the ^C for now, which is why we end up getting that blank line. But hopefully sometime in the future (perhaps with shell integration) we won't need to do that always.
This comment has been minimized.
This comment has been minimized.
doc/specs/#16599 - Quick Fix.md
Outdated
| ## Solution Design | ||
| Quick Fix will be a new UI surface that allows the user to interact with the terminal and leverage the context of a specific command being executed. The UI is discussed in the [UI/UX Design section](#ui/ux-design). | ||
|
|
||
| The UI itself will live in the TerminalControl layer, but it can be populated by both the TerminalControl and the TerminalApp layer (similar to the context menu). The suggestions will be appended as top-level items, but a nested design can be revisited as more sources are added to populate the Quick Fix menu. Only one Quick Fix menu will be available at a time as it only appears on the latest prompt if an error occurred; this is in alignment with VS Code's design. |
There was a problem hiding this comment.
as it only appears on the latest prompt
how are we placing that / knowing where a prompt is? I forget - do we emit the osc 9001 at the start of the error message, or the end?
There was a problem hiding this comment.
how are we placing that / knowing where a prompt is?
So, actually, implementation-wise, I'm not really checking for when the next prompt is ready:
It's been working for me so far (** knock on wood **). Any suggestions on a better way to do it?
do we emit the
osc 9001at the start of the error message, or the end?
We emit OSC 9001 at the start of the error message.
doc/specs/#16599 - Quick Fix.md
Outdated
| - `OSC 9001; CmdNotFound; <missingCmd>` will be used by the attached CLI app to notify the terminal that a command `missingCmd` wasn't found. The TerminalControl layer will notify the TerminalApp layer that this VT sequence as received and the TerminalApp layer will use WinGet's API to search for any packages that would remediate this error and construct a list of suggested packages to be installed. | ||
| `OSC 9001` can be expanded on to add more suggestions via VT sequences from the underlying CLI app; for more on this, read the [Future considerations section](#future-considerations). | ||
|
|
||
| TerminalApp will be notified upon receiving an `OSC 9001; CmdNotFound`. At this point, we'll know that a Quick Fix menu must be shown at the next prompt, and we can use the WinGet API to convert the given `missingCmd` into a list of suggestions (in this case, of the form `winget install...`). These suggestions will be handed back down to the TerminalControl layer to be added to the Quick Fix menu as entries. Since we're in the TerminalControl layer, the suggestions may not need to be converted into SendInput actions, as we already have access to the `RawWriteString()` api. The menu will simply use `RawWriteString()` to inject the string into the prompt. |
There was a problem hiding this comment.
I'm a little curious what the shape of this API actually looks like? Is it like,
runtimeclass TermControl {
// ...
event Windows.Foundation.TypedEventHandler<TermControl, CommandNotFoundRequestedArgs> CommandNotFound;
Microsoft.UI.Xaml.Controls.CommandBarFlyout QuickFixMenu { get; };
}(maybe not a CommandBarFlyout - something that's appropriate. Flyouts are hard)
...
// TerminalPage.cpp
term.CommandNotFound({ this, &TerminalPage::DoAWingetLookupForCommand });
term.QuickFixMenu().Opening([this](auto&& sender, auto&& /*args*/) {
this->AddQuickFixesToMenu(term.QuickFixMenu());
});There was a problem hiding this comment.
Here's a bit of an overview:
OSC 9001; CmdNotFound; <missingCmd>bubbles up from Terminal --> TermControl --> TermApp
// TermControl.idl
event Windows.Foundation.TypedEventHandler<Object, SearchMissingCommandEventArgs> SearchMissingCommand;
// EventArgs.idl
runtimeclass SearchMissingCommandEventArgs
{
String MissingCommand { get; };
}
- TermApp receives the SearchMissingCommand event and does the following:
std::vector<hstring> suggestions;
// populate suggestions with "winget install {}"
auto term = _GetActiveControl();
term.UpdateWinGetSuggestions(single_threaded_vector<hstring>(std::move(suggestions)));
term.ShowQuickFixMenu();- Quick Fix button is now displayed. When clicked, we...
term.QuickFixMenu().Opening([weak = get_weak(), weakTerm](auto&& sender, auto&& /*args*/) {
if (const auto& page{ weak.get() })
{
// Stuff the flyout menu with quick fixes we retrieve from the control
// NOTE 1: we get them from the control via "control.CommandHistory().QuickFixes()" which
// returns the list of suggestions from step 2
// NOTE 2: each menu item also clears the quick fix menu using "ClearQuickFix()" when invoked
page->_PopulateQuickFixMenu(weakTerm.get(), sender.try_as<Controls::MenuFlyout>());
}
});So TermControl.idl ends up exposing the following:
event Windows.Foundation.TypedEventHandler<Object, SearchMissingCommandEventArgs> SearchMissingCommand;
Windows.UI.Xaml.Controls.MenuFlyout QuickFixMenu { get; };
void UpdateWinGetSuggestions(Windows.Foundation.Collections.IVector<String> suggestions);
void ShowQuickFixMenu();
void ClearQuickFix();
doc/specs/#16599 - Quick Fix.md
Outdated
| The WinGet API to convert the given `missingCmd` into a list of suggestions may be relatively slow as we have to query the WinGet database. Performing the search when the quick fix menu is open would not be ideal as the user may have to wait for WinGet to finish searching. Instead, the conversion should happen around the time that the prompt is introduced (whether from output or a reflow scenario). | ||
|
|
||
| ## UI/UX Design | ||
| A Quick Fix icon will be displayed immediately left of the prompt. If the icon fits in the left-padding region (a `padding` value where the left side is 16 or greater), it will be drawn there. Otherwise, a thin accent-colored vertical bar will be displayed overlapping the text region; when the icon is hovered over, the icon will appear more clear by becoming more opaque and enlarging to overlap with the text region. |
There was a problem hiding this comment.
@zadjii-msft here's the UI I have from my branch.
Looks slick! One question though - is the button supposed to show up on the prompt with the error in it? The GIF shows the button showing up on the next empty prompt line, which IMO is a little unintuitive
This comment has been minimized.
This comment has been minimized.
Quick Fix will be a new UI surface that allows the user to interact with the terminal and leverage the context of a specific command being executed. This new UI surface will be a home for features like WinGet Command Not Found.
#16599