Add Quick Fix UI and support for custom CommandNotFound OSC #16848

carlos-zamora · 2024-03-08T21:14:14Z

`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

Add a key binding to open Quick Fix #17377: Add a key binding for quick fix
Improve OSC 9001; CmdNotFound suggestions #17378: Use winget to search for packages using missingCmd

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

src/cascadia/TerminalApp/TerminalPage.cpp

src/cascadia/TerminalControl/EventArgs.idl

src/cascadia/TerminalSettingsModel/Command.cpp

carlos-zamora · 2024-03-26T22:15:25Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                if (WI_IsFlagSet(source, SuggestionsSource::WinGetCommandNotFound) &&
+                    context != nullptr)
+                {
+                    const auto recentCommands = Command::ToSendInputCommands(context.WinGetSuggestions());
+                    for (const auto& t : recentCommands)
+                    {
+                        commandsCollection.push_back(t);
+                    }
+                }
+


@zadjii-msft is this kinda what you meant by adding the new suggestions to the context? If that's the case, we should rename the context to something else. Maybe ShellIntegrationContext?

Exactly what I was thinking. My quick recommendations:

I'd probably not name the member WinGetSuggestions, but instead QuickFixes. I'd think these should come through just the same as any other "quick fix" we add to the control (i.e. like a VT-driven one).

the same thing but with the flag in SuggestionsSource - that pobably also needs to get added to the JSON_FLAG_MAPPER in TerminalSettingsSerializationHelpers.h, as quickFixes

we probably want to pass a \uEA80 icon in here.

oofda I see the comment about the OEM icon below now. Well, similar idea I guess.

Oh huh, maybe we do want to give the winget suggestions a different icon. That's actually a really fun idea.

carlos-zamora · 2024-03-26T22:18:22Z

src/cascadia/TerminalControl/ControlCore.cpp

+        // TODO CARLOS: should we delete this after a new command is run? Or delete it after a suggestion is used? Or just after the next winget suggestion (current impl)?
+        //              No clue which we should do. Thoughts?
+        context->WinGetSuggestions(_cachedWinGetSuggestions);


@zadjii-msft thoughts on how to proceed here? I think...

delete after a new command is run: doable, but I'm not familiar with shell integration enough. If we wanna go this route, tips?

delete after a suggestion is used: this doesn't sound like something we can do easily. And also it doesn't sound like it'd feel totally right, imo.

delete after the next winget suggestion: I like that we don't hold on to stale suggestions, but I don't like that it's held on after another command is run.

Okay this is gonna sound like maybe a wacky idea, but what if we just stored these on the buffer, on the ROW itself? Kinda a bit like what I'm doing over in https://github.com/microsoft/terminal/pull/16937/files#diff-af0f626e5bb6718b60aa712a64c969e274f30968b5e24cc013318cf5c6badc12. If we just have one collection of suggestions per-row (a std::vector<wstring>), then we'd be able to just iterate up to the most recent collection of quick fixes, and only return those.

BUT even as I'm typing that up - I'd guess that a vector of strings, even an empty one, isn't low enough overhead to have the buffer go fast.

I guess a map of row->vector would work, but you'd have to deal with reflowing it and decrementing all rows on circling. Probably not terrible.

Knowing the row that the suggestion was printed on would also make drawing the icons more trivial.

another alternative / future thought - instead of just returning the most recent quick fix, you could further refine to be "the most recent quick fix if it was emitted in the most recent command with output". The most recent mark probably doesn't have output (it's just a prompt & possibly typed command), so keep looking backwards till you find one with output. Then only return quick fixes in that mark's extent.

Hmm... I'm leaning towards this idea:

I guess a map of row->vector would work, but you'd have to deal with reflowing it and decrementing all rows on circling. Probably not terrible.

We need to do this now so that we don't have the same suggestions appear forever. But the annoying thing is going to be that until quick fixes is implemented, we're just gonna hold on to the winget suggestions until they the buffer circles with no way to access them (since sxn ui can only be applied to the input line, right?). We can probably add some code in the interim that forces the map to only hold on to the most recent one, then undo that behavior as a part of quick fixes.

Another thing to point out here is that when the VT sequence is emitted, it'll be at the beginning of the "is not recognized..." line (where the flag is drawn in the image below)

So, this means that if there's multiple "is not recognized" messages, output from a command is going to have to consolidate all of them to suggest them as a quick fix on the next prompt. But without shell integration, we won't know which ones to consolidate.

As for this:

another alternative / future thought - instead of just returning the most recent quick fix, you could further refine to be "the most recent quick fix if it was emitted in the most recent command with output". The most recent mark probably doesn't have output (it's just a prompt & possibly typed command), so keep looking backwards till you find one with output. Then only return quick fixes in that mark's extent.

I like that idea a lot, but we can't rely on scroll marks existing since it requires changes to the shell, right?

So, if we're going to need shell integration anyways, perhaps the "the most recent quick fix if it was emitted in the most recent command with output" idea is best then. It just feels weird to couple it with shell integration 🤔

we're just gonna hold on to the winget suggestions

oh no a couple strings, oh nooooo /s. Honestly, if it's the right design in the next ~2 releases timeframe, then let's just do it right and not try and over-engineer the interim solution.

So, this means that if there's multiple "is not recognized" messages, output from a command is going to have to consolidate all of them to suggest them as a quick fix on the next prompt. But without shell integration, we won't know which ones to consolidate

If we store them as a property on the row, then we can do:

If there's a most recent prompt scroll mark, then get all the quick fixes from all the ROWs until that prompt

There's no shell integration? Then just get the quick fixes on the most recent ROW with quick fixes.

Basically, it works without shell integration, but it could work better with it

src/cascadia/TerminalSettingsModel/Command.cpp

zadjii-msft · 2024-03-27T11:02:51Z

src/cascadia/TerminalSettingsModel/Command.cpp

+            auto c = winrt::make_self<Command>();
+            c->_ActionAndArgs = actionAndArgs;
+            c->_name = command;
+            c->_iconPath = L"\ue74c"; // OEM icon


src/cascadia/TerminalSettingsModel/Command.cpp

src/cascadia/TerminalControl/ControlCore.cpp

zadjii-msft · 2024-03-27T11:17:56Z

src/cascadia/TerminalControl/ControlCore.cpp

+        // TODO CARLOS: should we delete this after a new command is run? Or delete it after a suggestion is used? Or just after the next winget suggestion (current impl)?
+        //              No clue which we should do. Thoughts?
+        context->WinGetSuggestions(_cachedWinGetSuggestions);


Okay this is gonna sound like maybe a wacky idea, but what if we just stored these on the buffer, on the ROW itself? Kinda a bit like what I'm doing over in https://github.com/microsoft/terminal/pull/16937/files#diff-af0f626e5bb6718b60aa712a64c969e274f30968b5e24cc013318cf5c6badc12. If we just have one collection of suggestions per-row (a std::vector<wstring>), then we'd be able to just iterate up to the most recent collection of quick fixes, and only return those.

BUT even as I'm typing that up - I'd guess that a vector of strings, even an empty one, isn't low enough overhead to have the buffer go fast.

I guess a map of row->vector would work, but you'd have to deal with reflowing it and decrementing all rows on circling. Probably not terrible.

Knowing the row that the suggestion was printed on would also make drawing the icons more trivial.

another alternative / future thought - instead of just returning the most recent quick fix, you could further refine to be "the most recent quick fix if it was emitted in the most recent command with output". The most recent mark probably doesn't have output (it's just a prompt & possibly typed command), so keep looking backwards till you find one with output. Then only return quick fixes in that mark's extent.

zadjii-msft · 2024-03-27T11:19:54Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                if (WI_IsFlagSet(source, SuggestionsSource::WinGetCommandNotFound) &&
+                    context != nullptr)
+                {
+                    const auto recentCommands = Command::ToSendInputCommands(context.WinGetSuggestions());
+                    for (const auto& t : recentCommands)
+                    {
+                        commandsCollection.push_back(t);
+                    }
+                }
+


Exactly what I was thinking. My quick recommendations:

I'd probably not name the member WinGetSuggestions, but instead QuickFixes. I'd think these should come through just the same as any other "quick fix" we add to the control (i.e. like a VT-driven one).

the same thing but with the flag in SuggestionsSource - that pobably also needs to get added to the JSON_FLAG_MAPPER in TerminalSettingsSerializationHelpers.h, as quickFixes

we probably want to pass a \uEA80 icon in here.

zadjii-msft · 2024-03-27T11:20:32Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                if (WI_IsFlagSet(source, SuggestionsSource::WinGetCommandNotFound) &&
+                    context != nullptr)
+                {
+                    const auto recentCommands = Command::ToSendInputCommands(context.WinGetSuggestions());
+                    for (const auto& t : recentCommands)
+                    {
+                        commandsCollection.push_back(t);
+                    }
+                }
+


oofda I see the comment about the OEM icon below now. Well, similar idea I guess.

zadjii-msft · 2024-03-27T11:21:33Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                if (WI_IsFlagSet(source, SuggestionsSource::WinGetCommandNotFound) &&
+                    context != nullptr)
+                {
+                    const auto recentCommands = Command::ToSendInputCommands(context.WinGetSuggestions());
+                    for (const auto& t : recentCommands)
+                    {
+                        commandsCollection.push_back(t);
+                    }
+                }
+


Oh huh, maybe we do want to give the winget suggestions a different icon. That's actually a really fun idea.

src/cascadia/TerminalApp/TerminalPage.cpp

zadjii-msft · 2024-03-27T11:28:06Z

src/cascadia/TerminalApp/TerminalAppLib.vcxproj

@@ -439,6 +439,11 @@
      <ResolvedFrom>CppWinRTImplicitlyExpandTargetPlatform</ResolvedFrom>
      <IsSystemReference>True</IsSystemReference>
    </Reference>
+    <Reference Include="$(OpenConsoleDir)src\cascadia\TerminalApp\Microsoft.Management.Deployment.winmd">


but also, just adding this here, is that enough to actually let our package know what dll these types are implemented in? Or do we need to do something to make sure the winget package (that presumably implements these) is actually in our package graph?

No clue! This and this comment are gonna be my next step (figuring out how to actually interact with WinGet).

a thought: stage this as two PRs.

this first one: plumbing, store the "quick fix" in the buffer, plumb into sxnui. But only store a blind winget install foo, don't actually do the package lookup.

probably just hide this behind velocity into canary only for the time being

a second PR that enlightens the suggestion to include real winget results for foo.

carlos-zamora · 2024-04-26T21:56:51Z

Demo (Outdated but kept around for full interaction)

See #16848 (comment) for updated design

Non-Collapsed (Normal)

Collapsed

zadjii-msft

Okay, I think I'm ready to sign off on this as a "this is canary, lets play with it and iterate" feature.

I think I have enough Q's here though to warrant a block.

My main one being: I thought we'd have the collapsed version always be like, max(2, padding.Left)dips wide (even covering part of the prompt!), but then always expand out to 16 (or font.Height or whatever) when hovered.

This is enough other plumbing that I think I'm fine saying "polish in post" but I just want to have plans on the record first ☺️

src/features.xml

zadjii-msft · 2024-05-31T19:07:30Z

src/cascadia/TerminalApp/TerminalPage.cpp

+
+        std::vector<hstring> suggestions;
+        suggestions.reserve(1);
+        suggestions.emplace_back(fmt::format(L"winget install {}", args.MissingCommand()));


📝 this doesn't actually do the winget lookup for args.MissingCommand, but we are including the winget winmd?

Oops. Yeah, we're blocked by winget. Internally tracking here.

Removed any references to the WinMD since we're not actually using it.

zadjii-msft · 2024-05-31T19:09:08Z

src/cascadia/TerminalApp/TerminalPage.cpp

+        // ShortcutActionDispatch. Used below to wire up each menu entry to the
+        // respective action. Then clear the quick fix menu.
+        auto weak = get_weak();
+        auto makeCallback = [weak](const hstring& suggestion) {


a lot of makeCallback and makeItem is shared with PopulateContextMenu - worth de-duplicating?

I actually completely based it off of PopulateContextMenu haha. Main changes I see are:

makeCallback clears quick fixes

AppBarButton vs MenuFlyoutItem

add a specific list of actions vs quick fixes from the control.CommandHistory() context

Honestly, (1) and (2) are going to be more annoying to deal with than its worth. I also suspect PopulateQuickFixMenu to diverge more as we add more sources in the future.

Leaving as-is for now, but if anybody feels strongly about it I can revisit it.

src/cascadia/TerminalControl/ControlCore.cpp

zadjii-msft · 2024-05-31T19:13:40Z

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1612,6 +1620,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
        _midiAudio.PlayNote(reinterpret_cast<HWND>(_owningHwnd), noteNumber, velocity, std::chrono::duration_cast<std::chrono::milliseconds>(duration));
    }

+    void ControlCore::_terminalSearchMissingCommand(std::wstring_view missingCommand)
+    {
+        SearchMissingCommand.raise(*this, make<implementation::SearchMissingCommandEventArgs>(hstring{ missingCommand }));


📝 this goes up on the BG thread, then comes back down into us in UpdateQuickFixes on the UI thread

src/cascadia/TerminalControl/TermControl.cpp

…ess some other feedback

src/cascadia/TerminalControl/TermControl.cpp

carlos-zamora · 2024-06-04T19:43:15Z

Cory also wanted a keybinding to open the quick fix menu (which, to be fair, we also need for accessibility). I'll add that in in this PR.

zadjii-msft · 2024-06-04T19:48:36Z

Cory also wanted a keybinding to open the quick fix menu (which, to be fair, we also need for accessibility). I'll add that in in this PR.

Wait can I NAK that? Merge, then iterate. This PR is plenty big enough, and has value on its own. We can always add a keybinding in post.

carlos-zamora · 2024-06-04T21:13:47Z

Feedback from bug bash

❌ Does the quick fix button account for top padding? Sure doesn't seem like it

Mess with the spacing to close these out...

Yea the button definitely needs to expand to be bigger when the padding is <16
Quick Fixes don't appear if you don't have any padding
I'm sorry but this is very hard to see if you don't know what QuickFixes are (but I'm not sure how to fix this without it being intrusive):

Accessibility Issues:

Dispatch a notification saying something like "suggestions available" when quick fix menu appears
Need keybinding to open quick fix menu
quick fix flyout menu (not button) needs a name

src/cascadia/TerminalControl/ControlCore.h

src/cascadia/TerminalApp/AppActionHandlers.cpp

carlos-zamora · 2024-06-05T22:37:40Z

Ok! Here's the updated UI:

0 padding

Small amount of padding

Note: at 15 padding, we're able to fit the button in the gutter, so we don't do the collapsed/expanded thing.

A lot of padding

This comment was marked as outdated.

Sign in to view

zadjii-msft mentioned this pull request Mar 22, 2024

Command Decorations / Marks in the gutter / margin / padding #16599

Open

carlos-zamora added 3 commits March 26, 2024 10:49

Add support for custom CommandNotFound OSC

12100c5

[Broken] Properly search through WinGet's catalog

c8d13d4

add debug code & dismiss button

534c2d9

carlos-zamora force-pushed the dev/cazamor/cmdNotFound branch from 779cda8 to 534c2d9 Compare March 26, 2024 17:52

carlos-zamora added 2 commits March 26, 2024 11:14

remove info bar code; fix branch

c1e1eab

use suggestions UI instead

68ed03d

carlos-zamora commented Mar 26, 2024

View reviewed changes

add a lil polish

c2417bb

carlos-zamora force-pushed the dev/cazamor/cmdNotFound branch from 222e334 to c2417bb Compare March 26, 2024 22:31

zadjii-msft reviewed Mar 27, 2024

View reviewed changes

carlos-zamora added 6 commits March 27, 2024 16:29

address feedback; winget integration still in progress

6a361ef

Merge branch 'main' into dev/cazamor/cmdNotFound

a2e57b4

moar progress! (useTransitions -> crash!)

cfaa09d

fix design

88b64cd

Merge branch 'main' into dev/cazamor/cmdNotFound

48a36be

feature flag; fix scroll bug; clear QF on enter

d8c8807

carlos-zamora marked this pull request as ready for review April 26, 2024 21:55

carlos-zamora requested a review from zadjii-msft April 26, 2024 21:57

clear quick fix on interaction

8c0d7c6

zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024

zadjii-msft requested changes May 31, 2024

View reviewed changes

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 May 31, 2024

Merge branch 'main' into dev/cazamor/cmdNotFound

ec1fbc2

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 Jun 3, 2024

zadjii-msft added a commit that referenced this pull request Jun 3, 2024

PRE-MERGE #16848 Add support for custom CommandNotFound OSC

4485e50

remove feature flag and WinMD refs; add QuickFixesAvailable API; addr…

767692c

…ess some other feedback

zadjii-msft reviewed Jun 4, 2024

View reviewed changes

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved

zadjii-msft mentioned this pull request Jun 4, 2024

1.22 We broke hot reload entirely #17364

Closed

zadjii-msft requested changes Jun 4, 2024

View reviewed changes

src/cascadia/TerminalControl/ControlCore.h Show resolved Hide resolved

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved

microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 4, 2024

carlos-zamora added 2 commits June 4, 2024 17:21

improve clearing functionality; fix build; x:Load; try to fix sizing

d4b6904

accessibility!

4e5d07d

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

carlos-zamora added 2 commits June 5, 2024 15:07

adjust button sizing for better visibility

27d464c

make the icon not dumb

0f801a9

codeformat

a545325

carlos-zamora changed the title ~~Add support for custom CommandNotFound OSC~~ Add Quick Fix UI and support for custom CommandNotFound OSC Jun 5, 2024

remove the winmd we're not using anymore

755f121

This was referenced Jun 5, 2024

Add a key binding to open Quick Fix #17377

Open

Improve OSC 9001; CmdNotFound suggestions #17378

Open

carlos-zamora assigned zadjii-msft Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Quick Fix UI and support for custom CommandNotFound OSC #16848

Add Quick Fix UI and support for custom CommandNotFound OSC #16848

carlos-zamora commented Mar 8, 2024 •

edited

This comment was marked as outdated.

This comment was marked as outdated.

carlos-zamora Mar 26, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

carlos-zamora Mar 26, 2024

zadjii-msft Mar 27, 2024

carlos-zamora Mar 27, 2024

zadjii-msft Apr 1, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

zadjii-msft Mar 27, 2024

carlos-zamora Mar 27, 2024

zadjii-msft Apr 1, 2024

carlos-zamora commented Apr 26, 2024 •

edited

zadjii-msft left a comment

zadjii-msft May 31, 2024

carlos-zamora Jun 3, 2024

zadjii-msft May 31, 2024

carlos-zamora Jun 3, 2024

zadjii-msft May 31, 2024

carlos-zamora commented Jun 4, 2024

zadjii-msft commented Jun 4, 2024 •

edited

carlos-zamora commented Jun 4, 2024 •

edited

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

carlos-zamora commented Jun 5, 2024

Add Quick Fix UI and support for custom CommandNotFound OSC #16848

Are you sure you want to change the base?

Add Quick Fix UI and support for custom CommandNotFound OSC #16848

Conversation

carlos-zamora commented Mar 8, 2024 • edited

OSC 9001; CmdNotFound; <missingCmd>

Quick Fix UI

Follow-ups

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Apr 26, 2024 • edited

Demo (Outdated but kept around for full interaction)

Non-Collapsed (Normal)

Collapsed

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora commented Jun 4, 2024

zadjii-msft commented Jun 4, 2024 • edited

carlos-zamora commented Jun 4, 2024 • edited

Feedback from bug bash

Accessibility Issues:

This comment was marked as resolved.

This comment was marked as outdated.

This comment was marked as resolved.

carlos-zamora commented Jun 5, 2024

0 padding

Small amount of padding

A lot of padding

carlos-zamora commented Mar 8, 2024 •

edited

`OSC 9001; CmdNotFound; <missingCmd>`

carlos-zamora commented Apr 26, 2024 •

edited

zadjii-msft commented Jun 4, 2024 •

edited

carlos-zamora commented Jun 4, 2024 •

edited