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 Spec for Quick Fix #17005

Merged
merged 6 commits into from
Jun 11, 2024
Merged

Add Spec for Quick Fix #17005

merged 6 commits into from
Jun 11, 2024

Conversation

carlos-zamora
Copy link
Member

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

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'm a NAK on the TerminalApp layer being the ones to draw this (as someone who's been down that road before)

doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
@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 Apr 3, 2024
@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 Apr 3, 2024
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
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.
Copy link
Member

Choose a reason for hiding this comment

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

accent color is an interesting choice - why that over like, the foreground color? Or yellow from the scheme's color table?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@carlos-zamora carlos-zamora Apr 26, 2024

Choose a reason for hiding this comment

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

@zadjii-msft here's the UI I have from my branch.

Normal version:
quickFix demo

Collapsed version:
quickFix demo (collapsed)

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).

Copy link
Member

Choose a reason for hiding this comment

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

oh my god it's so cool

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!)

image

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

@zadjii-msft zadjii-msft dismissed their stale review April 4, 2024 17:21

I won't block this while i'm out

This comment has been minimized.

doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
doc/specs/#16599 - Quick Fix.md Outdated Show resolved Hide resolved
## 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.
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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:
image

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 9001 at the start of the error message, or the end?

We emit OSC 9001 at the start of the error message.

- `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.
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 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());
});

Copy link
Member Author

Choose a reason for hiding this comment

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

Here's a bit of an overview:

  1. 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; };
}
  1. 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();
  1. 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 Show resolved Hide resolved
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

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

I'm picking nits at this point. My only main want is to make sure we can put these on more than just the last row (for shell integration features), but that's a solid vNext problem. Let's get to the code review

@carlos-zamora carlos-zamora added this pull request to the merge queue Jun 11, 2024
@DHowett DHowett removed this pull request from the merge queue due to a manual request Jun 11, 2024
@DHowett DHowett merged commit 125738b into main Jun 11, 2024
20 checks passed
@DHowett DHowett deleted the dev/cazamor/spec/quick-fix branch June 11, 2024 18:37
github-merge-queue bot pushed a commit that referenced this pull request Jun 29, 2024
### `OSC 9001; CmdNotFound; <missingCmd>`
Adds support for custom OSC "command not found" sequence `OSC 9001;
CmdNotFound; <missingCmd>`. Upon receiving the "CmdNotFound" variant
with the missing command payload, we send the missing command up to the
Quick Fix menu and add it in as `winget install <missingCmd>`.

### Quick Fix UI
The Quick Fix UI is a new UI surface that lives in the gutter (left
padding) of your terminal. The button appears if quick fixes are
available. When clicked, a list of suggestions appears in a flyout. If
there is not enough space in the gutter, the button will be presented in
a collapsed version that expands to a normal size upon hovering over it.

The Quick Fix UI was implemented similar to the context menu. The UI
itself lives in TermControl, but it can be populated by other layers
(i.e. TermApp layer).

Quick Fix suggestions are also automatically loaded into the Suggestions
UI.

If a quick fix is available and a screen reader is attached, we dispatch
an announcement that quick fixes are available to notify the user that
that's the case.

Spec: #17005
#16599

### Follow-ups
- #17377: Add a key binding for quick fix
- #17378: Use winget to search for packages using `missingCmd`

---------

Co-authored-by: Dustin L. Howett <duhowett@microsoft.com>
Co-authored-by: Dustin L. Howett <dustin@howett.net>
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.

5 participants