terminal icon indicating copy to clipboard operation
terminal copied to clipboard

Add support for custom CommandNotFound OSC

Open carlos-zamora opened this issue 1 year ago • 3 comments

Adds support for custom OSC "command not found" sequence. Upon receiving the "CmdNotFound" variant with the missing command payload, we search for a relevant package using winget and present the suggestions to the user in the suggestions UI (when they open the Suggestions UI).

carlos-zamora avatar Mar 08 '24 21:03 carlos-zamora

demo with fake data

carlos-zamora avatar Mar 08 '24 21:03 carlos-zamora

Why draft?

  • [ ] we get a class registration error (or some kind of error) when trying to create a PackageManager. Discussing with winget team.

carlos-zamora avatar Mar 08 '24 21:03 carlos-zamora

Demo (Outdated but kept around for full interaction)

See https://github.com/microsoft/terminal/pull/16848#issuecomment-2151075365 for updated design

Non-Collapsed (Normal)

quickFix demo

Collapsed

quickFix demo (collapsed)

carlos-zamora avatar Apr 26 '24 21:04 carlos-zamora

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.

carlos-zamora avatar Jun 04 '24 19:06 carlos-zamora

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.

zadjii-msft avatar Jun 04 '24 19:06 zadjii-msft

Feedback from bug bash

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

Mess with the spacing to close these out...

  • [x] Yea the button definitely needs to expand to be bigger when the padding is <16
  • [x] Quick Fixes don't appear if you don't have any padding
  • [x] 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:

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

carlos-zamora avatar Jun 04 '24 21:06 carlos-zamora

Alright! This PR is so close! Really just need to mess with the sizing of the quick fix button until it looks good.

I did hit an issue I need help with though. For whatever reason, I get a crash from XAML when I do the following:

  1. Open a terminal with a profile that has 0 padding
  2. get the quick fix menu to appear
  3. When you hover over the quick fix button, we crash!

@zadjii-msft when you have a minute, could you take a look at this?

carlos-zamora avatar Jun 05 '24 00:06 carlos-zamora

Huh. Seems like it's dependent on DPI somehow? didn't repro on my 100% display, did on my 125%....

Weird. I could continue past it, but after I did that (required a couple go's), then the button stopped responding to... anything. Didn't move when new output was emitted. Didn't respond to hovers. Weird.

(still investigating)

zadjii-msft avatar Jun 05 '24 18:06 zadjii-msft

We figured it out! Here's the culprit: QuickFixIcon().FontSize(std::min(static_cast<double>(args.Width() / dpiScale), GetPadding().Left));

GetPadding().Left() --> 0 so we'd try and make the icon's font size 0 which would eventually lead to a crash.

Fixing it as a part of polishing the UI since that's the last remaining feedback.

carlos-zamora avatar Jun 05 '24 20:06 carlos-zamora

Ok! Here's the updated UI:

0 padding

collapsed expanded

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

A lot of padding

image

carlos-zamora avatar Jun 05 '24 22:06 carlos-zamora

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.

lhecker avatar Jun 10 '24 23:06 lhecker

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

zadjii-msft avatar Jun 22 '24 11:06 zadjii-msft

Ohhh crap it conflicts with the renderer changes on main.

DHowett avatar Jun 28 '24 22:06 DHowett