terminal
terminal copied to clipboard
Add support for custom CommandNotFound OSC
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).
Why draft?
- [ ] we get a class registration error (or some kind of error) when trying to create a
PackageManager. Discussing with winget team.
Demo (Outdated but kept around for full interaction)
See https://github.com/microsoft/terminal/pull/16848#issuecomment-2151075365 for updated design
Non-Collapsed (Normal)
Collapsed
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.
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.
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
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:
- Open a terminal with a profile that has 0 padding
- get the quick fix menu to appear
- When you hover over the quick fix button, we crash!
@zadjii-msft when you have a minute, could you take a look at this?
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)
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.
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
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.
(@DHowett we could probably merge this even while carlos is out)
Ohhh crap it conflicts with the renderer changes on main.