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 Quick Fix UI and support for custom CommandNotFound OSC #16848

Merged
merged 26 commits into from
Jun 29, 2024

Conversation

carlos-zamora
Copy link
Member

@carlos-zamora carlos-zamora commented Mar 8, 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

@carlos-zamora

This comment was marked as outdated.

@carlos-zamora

This comment was marked as outdated.

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/cmdNotFound branch from 779cda8 to 534c2d9 Compare March 26, 2024 17:52
src/cascadia/TerminalApp/Resources/en-US/Resources.resw Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/EventArgs.idl Show resolved Hide resolved
src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
Comment on lines 1351 to 1360
if (WI_IsFlagSet(source, SuggestionsSource::WinGetCommandNotFound) &&
context != nullptr)
{
const auto recentCommands = Command::ToSendInputCommands(context.WinGetSuggestions());
for (const auto& t : recentCommands)
{
commandsCollection.push_back(t);
}
}

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Comment on lines 2289 to 2291
// 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);
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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)
selection marker at the beginning of CMD's command not found output

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

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

@carlos-zamora carlos-zamora force-pushed the dev/cazamor/cmdNotFound branch from 222e334 to c2417bb Compare March 26, 2024 22:31
src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
auto c = winrt::make_self<Command>();
c->_ActionAndArgs = actionAndArgs;
c->_name = command;
c->_iconPath = L"\ue74c"; // OEM icon
Copy link
Member

Choose a reason for hiding this comment

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

image

src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
Comment on lines 2289 to 2291
// 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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
@@ -439,6 +439,11 @@
<ResolvedFrom>CppWinRTImplicitlyExpandTargetPlatform</ResolvedFrom>
<IsSystemReference>True</IsSystemReference>
</Reference>
<Reference Include="$(OpenConsoleDir)src\cascadia\TerminalApp\Microsoft.Management.Deployment.winmd">
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 carlos-zamora marked this pull request as ready for review April 26, 2024 21:55
@carlos-zamora
Copy link
Member Author

carlos-zamora commented Apr 26, 2024

Demo (Outdated but kept around for full interaction)

See #16848 (comment) for updated design

Non-Collapsed (Normal)

quickFix demo

Collapsed

quickFix demo (collapsed)

@zadjii-msft zadjii-msft added this to the Terminal v1.22 milestone May 31, 2024
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.

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 Show resolved Hide resolved

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

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

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

src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/ControlCore.cpp Outdated Show resolved Hide resolved
@@ -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 }));
Copy link
Member

Choose a reason for hiding this comment

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

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

src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp 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 May 31, 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 Jun 3, 2024
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

Most of the things I can think of are nits, that aren't really worth mentioning.

src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
src/cascadia/TerminalApp/TerminalPage.cpp Show resolved Hide resolved
src/cascadia/TerminalControl/TermControl.cpp Outdated Show resolved Hide resolved
@lhecker
Copy link
Member

lhecker commented Jun 10, 2024

It may be worth mentioning here as well that the 2 (?) functions in Windows Terminal that use \x7f to clear parts of the prompt are broken for non-Latin languages. This appears to include this PR as it's based on Command::HistoryToCommands. (The other place is _filterToSendInput. This doesn't seem to apply to ParsePowerShellMenuComplete since the backspace count stems from the shell itself via a VT sequence.)

The 2 functions correlate the number of characters with the number of backspaces. But that's not how grapheme clusters, combining characters, etc. work. I don't have the big picture understanding to how the \x7f backspaces are used exactly, but I'm convinced they aren't generated correctly at least. I think we probably can't ship this feature (or any other feature that depends on this indirectly) as stable until we addressed this. It's possible that simply using CodepointWidthDetector from my graphemes PR would already fix this issue, since it lets you count the number of graphemes in a string. Additionally, if all we want is to clear a prompt completely, we should IMO emit a single Esc instead of multiple backspaces, since that will always work independent of the prompt contents.

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.

Blocking until the spec is reviewed

@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 Jun 11, 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 Jun 12, 2024
@zadjii-msft
Copy link
Member

(@DHowett we could probably merge this even while carlos is out)

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.

Alright, the only thing I'm actually blocking over is the bad conflict resolution in TerminalCore.h! This looks really good. Well done.

void ControlCore::ClearQuickFix()
{
_cachedQuickFixes = nullptr;
RefreshQuickFixUI.raise(*this, nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

mildly alarmed that ClearQuickFix seems to refresh the QF UI but UpdateQuickFixes doesn't

return _cachedQuickFixes && _cachedQuickFixes.Size() > 0;
}

void ControlCore::UpdateQuickFixes(const Windows::Foundation::Collections::IVector<hstring>& quickFixes)
Copy link
Member

Choose a reason for hiding this comment

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

this is more of a Set than an Update. Update is like "do it on your own". Set is more like "i will give it to you"

@@ -354,6 +362,9 @@ namespace winrt::Microsoft::Terminal::Control::implementation

til::point _contextMenuBufferPosition{ 0, 0 };

Windows::Foundation::Collections::IVector<int32_t> _cachedSearchResultRows{ nullptr };
Copy link
Member

Choose a reason for hiding this comment

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

what is this?

Copy link
Member

Choose a reason for hiding this comment

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

it looks like a line that was removed in #17132. bad conflict resolution?

//
// Then treat this line like it's a prompt mark.
if (_autoMarkPrompts && vkey == VK_RETURN && !_inAltBuffer())
if (vkey == VK_RETURN && !_inAltBuffer())
Copy link
Member

Choose a reason for hiding this comment

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

quick fix is cleared if we're not in the alt buffer. do we have a way to hide it if you enter the alt buffer?

cmd may never do this, so it may be moot

src/cascadia/TerminalSettingsModel/Command.cpp Outdated Show resolved Hide resolved
@DHowett DHowett enabled auto-merge June 28, 2024 20:19
@DHowett
Copy link
Member

DHowett commented Jun 28, 2024

Ohhh crap it conflicts with the renderer changes on main.

@DHowett DHowett disabled auto-merge June 28, 2024 22:53
@DHowett DHowett enabled auto-merge June 28, 2024 22:54
@DHowett DHowett added this pull request to the merge queue Jun 28, 2024
Merged via the queue into main with commit 6589957 Jun 29, 2024
22 checks passed
@DHowett DHowett deleted the dev/cazamor/cmdNotFound branch June 29, 2024 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-TerminalControl Issues pertaining to the terminal control (input, selection, keybindings, mouse interaction, etc.) Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants