-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Draft API proposal for #888 Support for UWP callers and a non command line interface API #889
Conversation
Merge microsoft/winget-cli changes.
Rest endpoint helper and json object field check - Bug fix (microsoft#821)
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
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.
The other items should just be added to expect.txt/allow.txt
First pass of updates from jonwis feedback.
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
This comment has been minimized.
This comment has been minimized.
Change CRLF to LF.
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.
Seems OK to start API review, invite coming soon.
doc/specs/#888 - Com Api.md
Outdated
installOptions.Package(package); | ||
installOptions.InstallMode(InstallMode::Silent); | ||
|
||
return appInstaller.InstallPackageAsync(installOptions); |
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.
Oopsie, you're right.
doc/specs/#888 - Com Api.md
Outdated
{ | ||
AppInstaller appInstaller = CreateAppInstaller(); | ||
AppCatalog catalog{ co_await appInstaller.GetAppCatalogAsync(packageSource) }; | ||
co_await catalog.OpenAsync(); |
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.
See https://www.osgwiki.com/wiki/WinRT_API_Design_Guidelines#Specifying_Asynchronous_Operation_Objects_in_IDL (non-MSFT folks, I hope to have these publicly visible soon) for how to do it. The issue is that we want people to not wrap all their operations in try/catch. When an operation fails and returns an hresult it generates an exception in most langauges, so you'd have to write this:
AppCatalog ac{nullptr};
winrt::hresult ecode;
try {
ac = co_await foo.FindSource(sourceName);
} catch (winrt::hresult_exception const& ex) {
ecode = ex.code();
}
if (!ac) {
// do something with 'ecode'
} else {
// catalog ready
}
Compare that to:
FindAppCatalogResult found = co_await thing.FindSource(sourceName);
if (found.Catalog) {
// use found.Catalog
} else {
switch (found.Status()) {
case FindCatalogResultStatus::AccessDenied:
case FindCatalogResultStatus::NotFound:
case FindCatalogResultStatus::ServerError:
case FindCatalogResultStatus::MalformedResponse:
// do whatever with those
return;
case FindCatalogResultStatus::Unknown:
// Send telemetry using found.ExtendedError();
return;
}
}
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.
API review Left off on about line 486.
AppInstaller CreateAppInstaller() { | ||
return ActivateByCoCreate<AppInstaller>(CLSID_AppInstaller); | ||
} | ||
InstallOptions CreateInstallOptions() { |
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.
CONSIDER: Ask COM team if it's possible to have a DLL from the package loaded into the packaged-COM client to put the "brains" of the option object into the client side.
CONSIDER: Define a "low level" valueset based interface, and there's a Reunion wrapper that has the better thing that can be loaded into the caller.
filter.Field(PackageMatchField::Id); | ||
filter.Type(MatchType::Exact); | ||
filter.Value(packageId); | ||
findPackagesOptions.Filters().Append(filter); |
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.
DISCUSSION: Yes, we should look into how we can reduce this set of calls
Misspellings found, please review:
To accept these changes, run the following commands from this repository on this branch
|
/// The name of the app catalog. | ||
String Name { get; }; | ||
/// The type of the app catalog. | ||
String Type { get; }; |
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.
DISCUSSION: This is set when someone adds a catalog at the command line. It should be an open-ended set.
RECOMMEND: Use the KnownFoo
pattern, where you have KnownAppCatalogKinds::Rest
which is a static property whose value can be assigned here. (Apply this more places.)
[contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 1)] | ||
enum PackageVersionMetadata | ||
{ | ||
/// The InstallerType of an installed 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.
Recommend: Use the KnownAppInstallerKinds
pattern here as well
doc/specs/#888 - Com Api.md
Outdated
[contract(Microsoft.Management.Deployment.WindowsPackageManagerContract, 1)] | ||
runtimeclass AppInstaller | ||
{ | ||
AppInstaller(); |
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.
DISCUSSION: See earlier comments about CoCreate vs RoActivate here
@sreadingMSFT , can we get the spelling fixes and merge this? |
I'll be sharing this with the API design review team and will update this when that review has a scheduled date.
See the Files Changed tab for the Com Api.md.
Microsoft Reviewers: Open in CodeFlow