-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Use WinGet API to improve Quick Fix results #17614
Conversation
build/rules/Microsoft.WindowsPackageManager.ComInterop.Additional.targets
Fixed
Show fixed
Hide fixed
build/rules/Microsoft.WindowsPackageManager.ComInterop.Additional.targets
Fixed
Show fixed
Hide fixed
Now that we're adding
It's annoying because we're just plain clipping the text and we can't configure it. Looks like the only way is to retemplate the MenuFlyout (or maybe retemplate the MenuFlyoutItem?). I'll switch to the shorthand of |
This hangs the UI pretty significantly while it's querying - gotta move that work off the main thread! |
It's already off the main thread, isn't it? terminal/src/cascadia/TerminalApp/TerminalPage.cpp Lines 3046 to 3053 in a0f2d88
If I'm correct about that, I think it's the console lock that's being held by the terminal during VT parsing. The UI thread tries to acquire the console lock regularly for info. In that case we just need to go to a different background thread instead, just not the VT parser one. 😅 (We could also add debouncing, but... eh. That may be too much for this limited purpose.) |
Ah! We were on the |
The newest change now includes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay a minor thought: In 1.22 I added Description
's to Command
s. If the package catalog gives us a description, we could totally stick that in the teaching tip 👀
Similarly, IMO, we don't need --source winget
in the Name
of the Command. It's clearer to just say winget install Foo
and have the rest of the args in the preview text (but that's just my opinion)
@@ -0,0 +1,22 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝: weird to me that this isn't just in their nuget package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are working on improving this facet.
@@ -20,6 +20,7 @@ | |||
<TerminalCppWinrt>true</TerminalCppWinrt> | |||
<TerminalThemeHelpers>true</TerminalThemeHelpers> | |||
<TerminalMUX>true</TerminalMUX> | |||
<TerminalWinGetInterop>true</TerminalWinGetInterop> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need this for WindowsTerminal.exe in addition to TerminalApp? Is this cause the dependency doesn't get rolled up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Just tested it without it. The QF menu won't appear.
@@ -0,0 +1,61 @@ | |||
// Copyright (c) Microsoft Corporation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝: it's so gross that we have to write this ourselves
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, which is why the nuget package is soon going to include a DLL that handles this part (exposes the activation factory so that one can just use WinRT language projections as normal).
Opened #17755. I like the idea and it should be pretty straightforward to do, but it's a bit bigger of a change than I'd like for this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops didn't ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'd approve, but I'll wait a bit for you to read my comments. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's go!
Summary of the Pull Request
Improves Quick Fix's suggestions to use WinGet API and actually query winget for packages based on the missing command.
To interact with the WinGet API, we need the
Microsoft.WindowsPackageManager.ComInterop
NuGet package.Microsoft.WindowsPackageManager.ComInterop.Additional.targets
is used to copy over the winmd into CascadiaPackage. The build variableTerminalWinGetInterop
is used to import the package properly.WindowsPackageManagerFactory
is used as a centralized way to generate the winget objects. Long-term, we may need to do manual activation for elevated sessions, which this class can easily be extended to support. In the meantime, we'll just use the normalwinrt::create_instance
on all sessions.In
TerminalPage
, we conduct the search asynchronously when a missing command was found. Search results are limited to 20 packages. We try to retrieve packages with the following filters set, then fallback into the next step:PackageMatchField::Command
,PackageFieldMatchOption::StartsWithCaseInsensitive
PackageMatchField::Name
,PackageFieldMatchOption::ContainsCaseInsensitive
PackageMatchField::Moniker
,PackageFieldMatchOption::ContainsCaseInsensitive
This aligns with the Microsoft.WinGet.CommandNotFound PowerShell module (link to relevant code).
Closes #17378
Closes #17631
Support for elevated sessions tracked in #17677
References
Validation Steps Performed
winget install {}
suggestion)