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.

Sign up for GitHub

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

Fix list/upgrade table for packages with multiple ARP entries. #2137

Merged
merged 8 commits into from
May 18, 2022
Merged

Fix list/upgrade table for packages with multiple ARP entries. #2137

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ca93379
Fix list/upgrade table for packages with multiple ARP entries.
jedieaston May 3, 2022
07521bf
Replaced tabs with spaces, sorry.
jedieaston May 3, 2022
499c008
Now using a pair of the ID and source for deduping.
jedieaston May 4, 2022
2959a36
Fixed issues from @yao-msft's code review.
jedieaston May 4, 2022
589b6d8
Source identifier, not source name!
jedieaston May 4, 2022
51b20ec
Removed argument. Now we always output the ARP table name.
jedieaston May 13, 2022
8543c1d
Removed unused variable (oops!)
jedieaston May 15, 2022
7089b6b
Merge remote-tracking branch 'upstream/master' into this-is-getting-o…
jedieaston May 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/ListCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ namespace AppInstaller::CLI
Argument::ForType(Execution::Args::Type::Exact),
Argument::ForType(Execution::Args::Type::CustomHeader),
Argument::ForType(Execution::Args::Type::AcceptSourceAgreements),
Argument{ "all", Argument::NoAlias, Execution::Args::Type::ListAll, Resource::String::ListAllArgumentDescription, ArgumentType::Flag},
Copy link
Member

Choose a reason for hiding this comment

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

I agree with @Trenly that this could just use Args::Type::All (especially now that its not in ForType [which existed pre-localization, but now I don't think we should use]).

Ultimately it's just another internal enum value, so it is more about how its makes us feel than anything else.

};
}

Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/ExecutionArgs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ namespace AppInstaller::CLI::Execution

// Other
All, // Used in Update command to update all installed packages to latest
ListAll, // Used in List command to list all installed apps associated with a package
ListVersions, // Used in Show command to list all available versions of an app
NoVT, // Disable VirtualTerminal outputs
RetroStyle, // Makes progress display as retro
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(InvalidNameError);
WINGET_DEFINE_RESOURCE_STRINGID(LicenseAgreement);
WINGET_DEFINE_RESOURCE_STRINGID(Links);
WINGET_DEFINE_RESOURCE_STRINGID(ListAllArgumentDescription);
WINGET_DEFINE_RESOURCE_STRINGID(ListCommandLongDescription);
WINGET_DEFINE_RESOURCE_STRINGID(ListCommandShortDescription);
WINGET_DEFINE_RESOURCE_STRINGID(LocaleArgumentDescription);
Expand Down
50 changes: 38 additions & 12 deletions src/AppInstallerCLICore/Workflows/WorkflowBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -710,6 +710,7 @@ namespace AppInstaller::CLI::Workflow
int unknownPackagesCount = 0;
auto &source = context.Get<Execution::Data::Source>();
bool shouldShowSource = source.IsComposite() && source.GetAvailableSources().size() > 1;
std::set<std::pair<Utility::LocIndString, Utility::LocIndString>> packageIdsPrinted;

for (const auto& match : searchResult.Matches)
{
Expand All @@ -730,27 +731,52 @@ namespace AppInstaller::CLI::Workflow
// The only time we don't want to output a line is when filtering and no update is available.
if (updateAvailable || !m_onlyShowUpgrades)
{
Utility::LocIndString availableVersion, sourceName;
Utility::LocIndString availableVersion, sourceName, sourceIdentifier;
Utility::LocIndString packageId = match.Package->GetProperty(PackageProperty::Id);
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest that rather than storing the packageId and sourceIdentifier separately, just store the pair directly. That would remove 3 times where the strings will be copied to create a temporary pair.

Copy link
Member

Choose a reason for hiding this comment

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

Getting value into packageId but not using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in latest commit. Oops!


if (latestVersion)
{
// Always show the source for correlated packages
sourceName = latestVersion->GetProperty(PackageVersionProperty::SourceName);
sourceIdentifier = latestVersion->GetProperty(PackageVersionProperty::SourceIdentifier);

if (updateAvailable)
{
availableVersion = latestVersion->GetProperty(PackageVersionProperty::Version);
availableUpgradesCount++;
if (context.Args.Contains(Execution::Args::Type::ListAll) || !packageIdsPrinted.count({ packageId, sourceIdentifier }))
{
// we should only add to the upgrade count if we actually showed the table entry.
availableUpgradesCount++;
}
}

// Always show the source for correlated packages
sourceName = latestVersion->GetProperty(PackageVersionProperty::SourceName);
}

table.OutputLine({
match.Package->GetProperty(PackageProperty::Name),
match.Package->GetProperty(PackageProperty::Id),
installedVersion->GetProperty(PackageVersionProperty::Version),
availableVersion,
shouldShowSource ? sourceName : ""s
});
if (context.Args.Contains(Execution::Args::Type::ListAll))
{
table.OutputLine({
installedVersion->GetProperty(PackageVersionProperty::Name),
match.Package->GetProperty(PackageProperty::Id),
installedVersion->GetProperty(PackageVersionProperty::Version),
availableVersion,
shouldShowSource ? sourceName : ""s
});
}
else
{
// we need to only list once per package
if (!packageIdsPrinted.count({ packageId, sourceIdentifier }))
{
packageIdsPrinted.insert({ packageId, sourceIdentifier });
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
packageIdsPrinted.insert({ packageId, sourceIdentifier });
packageIdsPrinted.emplace(std::move(thePair));

Assuming that you also took my other suggestion above about keeping these as a pair rather than separate. And you named it thePair, which I'm sure you would come up with a better name than that 😉

table.OutputLine({
match.Package->GetProperty(PackageProperty::Name),
match.Package->GetProperty(PackageProperty::Id),
installedVersion->GetProperty(PackageVersionProperty::Version),
availableVersion,
shouldShowSource ? sourceName : ""s
});
}
}

}
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1308,4 +1308,7 @@ Please specify one of them using the `--source` option to proceed.</value>
<data name="NoPackageSelectionArgumentProvided" xml:space="preserve">
<value>No package selection argument was provided; see the help for details about finding a package.</value>
</data>
<data name="ListAllArgumentDescription" xml:space="preserve">
<value>List all software installed by a package</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we try to make "package" the terminology for software across winget but I can see it's weird to say "all packages installed by a package"

Maybe "List all package components installed by a package" or "List all sub-packages installed by a package"

Open to any suggestions.

Copy link
Contributor Author

@jedieaston jedieaston May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@denelon, thoughts? I think "List all components installed by a package." makes the most sense.

Copy link
Member

Choose a reason for hiding this comment

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

I refer to these as "system artifacts" internally, but that probably isn't a great name for them publicly. I do like "components" or "items".

</data>
</root>