From 49ae6549ff80e96348fefa6674f08f070aa89285 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Tue, 28 Apr 2026 16:18:11 -0700 Subject: [PATCH 01/16] [ListSort, Workflow] Add sort logic for winget list output --- .../AppInstallerCLICore.vcxproj | 1 + .../AppInstallerCLICore.vcxproj.filters | 3 + .../Commands/ListCommand.cpp | 4 +- .../Workflows/ListSortHelper.h | 102 ++++++++++++++++ .../Workflows/WorkflowBase.cpp | 110 ++++++++++++++++++ .../Public/winget/UserSettings.h | 6 +- 6 files changed, 222 insertions(+), 4 deletions(-) create mode 100644 src/AppInstallerCLICore/Workflows/ListSortHelper.h diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj index 2106c1faba..910938ccec 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj @@ -366,6 +366,7 @@ + diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters index 3a939771c6..80dfb42dd5 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters @@ -50,6 +50,9 @@ Workflows + + Workflows + Workflows diff --git a/src/AppInstallerCLICore/Commands/ListCommand.cpp b/src/AppInstallerCLICore/Commands/ListCommand.cpp index acb12bdd46..b663ab1462 100644 --- a/src/AppInstallerCLICore/Commands/ListCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ListCommand.cpp @@ -32,7 +32,9 @@ namespace AppInstaller::CLI Argument{ Execution::Args::Type::IncludeUnknown, Resource::String::IncludeUnknownInListArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::IncludePinned, Resource::String::IncludePinnedInListArgumentDescription, ArgumentType::Flag}, Argument::ForType(Execution::Args::Type::ListDetails), - Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }, + // 7 = one more than the number of user-facing SortField values (Relevance, Name, Id, Version, Source, Available), + // allowing all fields plus a small buffer for future additions. + Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(7), Argument{ Execution::Args::Type::SortAscending, Resource::String::SortAscendingArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::SortDescending, Resource::String::SortDescendingArgumentDescription, ArgumentType::Flag }, }; diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.h b/src/AppInstallerCLICore/Workflows/ListSortHelper.h new file mode 100644 index 0000000000..88fdc2b3bb --- /dev/null +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.h @@ -0,0 +1,102 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#pragma once +#include +#include +#include +#include + +#include +#include +#include + +namespace AppInstaller::CLI::Workflow +{ + // Lightweight sortable representation of a package line in list output. + // Decoupled from ICompositePackage/IPackageVersion to enable unit testing. + struct SortablePackageEntry + { + Utility::LocIndString Name; + Utility::LocIndString Id; + Utility::LocIndString InstalledVersion; + Utility::LocIndString AvailableVersion; + Utility::LocIndString Source; + }; + + // Compares two sortable entries by the given field. + // Returns negative if a < b, positive if a > b, 0 if equal. + inline int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) + { + using Settings::SortField; + + switch (field) + { + case SortField::Name: + return Utility::FoldCase(std::string_view{ a.Name.get() }).compare(Utility::FoldCase(std::string_view{ b.Name.get() })); + case SortField::Id: + return Utility::FoldCase(std::string_view{ a.Id.get() }).compare(Utility::FoldCase(std::string_view{ b.Id.get() })); + case SortField::Version: + { + Utility::Version va(a.InstalledVersion.get()); + Utility::Version vb(b.InstalledVersion.get()); + if (va < vb) return -1; + if (vb < va) return 1; + return 0; + } + case SortField::Source: + return Utility::FoldCase(std::string_view{ a.Source.get() }).compare(Utility::FoldCase(std::string_view{ b.Source.get() })); + case SortField::Available: + { + bool aHas = !a.AvailableVersion.get().empty(); + bool bHas = !b.AvailableVersion.get().empty(); + if (aHas != bHas) + { + // Ascending: has-update sorts before no-update + return aHas ? -1 : 1; + } + if (aHas && bHas) + { + Utility::Version va(a.AvailableVersion.get()); + Utility::Version vb(b.AvailableVersion.get()); + if (va < vb) return -1; + if (vb < va) return 1; + } + return 0; + } + default: + return 0; + } + } + + // Sorts a vector of sortable entries by the given fields and direction. + inline void SortEntries( + std::vector& entries, + const std::vector& sortFields, + Settings::SortDirection direction) + { + if (entries.size() <= 1 || sortFields.empty()) + { + return; + } + + // Relevance-only means no sorting + if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) + { + return; + } + + std::stable_sort(entries.begin(), entries.end(), + [&sortFields, direction](const SortablePackageEntry& a, const SortablePackageEntry& b) + { + for (const auto& field : sortFields) + { + int cmp = CompareByField(a, b, field); + if (cmp != 0) + { + return direction == Settings::SortDirection::Ascending ? (cmp < 0) : (cmp > 0); + } + } + return false; + }); + } +} diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 9ee72f47db..3b8629e7d2 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -3,6 +3,8 @@ #include "pch.h" #include "WorkflowBase.h" #include "ExecutionContext.h" +#include +#include "Workflows/ListSortHelper.h" #include #include "PromptFlow.h" #include "ShowFlow.h" @@ -459,6 +461,111 @@ namespace AppInstaller::CLI::Workflow } } + // Compares two InstalledPackagesTableLine values by the given field. + int CompareByField(const InstalledPackagesTableLine& a, const InstalledPackagesTableLine& b, SortField field) + { + // Create SortablePackageEntry copies for comparison — fields are layout-compatible + // (both have Name, Id, InstalledVersion, AvailableVersion, Source as LocIndString in same order). + SortablePackageEntry entryA{ a.Name, a.Id, a.InstalledVersion, a.AvailableVersion, a.Source }; + SortablePackageEntry entryB{ b.Name, b.Id, b.InstalledVersion, b.AvailableVersion, b.Source }; + return Workflow::CompareByField(entryA, entryB, field); + } + + // Returns true if the execution context contains filter arguments that + // produce relevance-ordered results (query, name, id, moniker, tag, command). + bool HasRelevanceAffectingArgs(const Execution::Context& context) + { + return context.Args.Contains(Execution::Args::Type::Query) || + context.Args.Contains(Execution::Args::Type::Id) || + context.Args.Contains(Execution::Args::Type::Name) || + context.Args.Contains(Execution::Args::Type::Moniker) || + context.Args.Contains(Execution::Args::Type::Tag) || + context.Args.Contains(Execution::Args::Type::Command); + } + + // Sorts a vector of InstalledPackagesTableLine according to the user's sort preferences. + // Resolution order: CLI args (--sort) > settings (output.sortOrder) > query-aware default. + void SortInstalledPackagesTableLines(Execution::Context& context, std::vector& lines) + { + if (lines.size() <= 1) + { + return; + } + + // 1. Determine sort fields: CLI --sort overrides everything + std::vector sortFields; + bool hasExplicitSort = context.Args.Contains(Execution::Args::Type::Sort); + + if (hasExplicitSort) + { + const auto* sortArgs = context.Args.GetArgs(Execution::Args::Type::Sort); + if (sortArgs) + { + for (const auto& arg : *sortArgs) + { + auto field = ConvertToSortField(arg); + if (field) + { + sortFields.emplace_back(field.value()); + } + // Invalid values are silently skipped; argument validation + // should catch these before we get here in a future PR. + } + } + } + else if (HasRelevanceAffectingArgs(context)) + { + // Design: when query/filter arguments are present, relevance ordering is + // always preserved unless the user explicitly passes --sort on the CLI. + // Settings sortOrder intentionally does NOT override relevance for queries, + // because query results are ranked by match quality and reordering them + // would hide the best matches. Only --sort (explicit user intent per-command) + // can override this. + return; + } + else + { + sortFields = User().Get(); + } + + // Empty sort fields or relevance-only means no sorting + if (sortFields.empty() || + (sortFields.size() == 1 && sortFields[0] == SortField::Relevance)) + { + return; + } + + // 2. Determine direction: CLI flags override settings + SortDirection direction = SortDirection::Ascending; + if (context.Args.Contains(Execution::Args::Type::SortDescending)) + { + direction = SortDirection::Descending; + } + else if (context.Args.Contains(Execution::Args::Type::SortAscending)) + { + direction = SortDirection::Ascending; + } + else + { + direction = User().Get(); + } + + // 3. Multi-field cascading sort with stable_sort + std::stable_sort(lines.begin(), lines.end(), + [&sortFields, direction](const InstalledPackagesTableLine& a, const InstalledPackagesTableLine& b) + { + for (const auto& field : sortFields) + { + int cmp = CompareByField(a, b, field); + if (cmp != 0) + { + return direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); + } + } + return false; + }); + } + void OutputInstalledPackages(Execution::Context& context, std::vector& lines) { if (context.Args.Contains(Execution::Args::Type::ListDetails)) @@ -1226,6 +1333,7 @@ namespace AppInstaller::CLI::Workflow } } + SortInstalledPackagesTableLines(context, lines); OutputInstalledPackages(context, lines); if (lines.empty()) @@ -1248,12 +1356,14 @@ namespace AppInstaller::CLI::Workflow if (!linesForExplicitUpgrade.empty()) { context.Reporter.Info() << std::endl << Resource::String::UpgradeAvailableForPinned << std::endl; + SortInstalledPackagesTableLines(context, linesForExplicitUpgrade); OutputInstalledPackages(context, linesForExplicitUpgrade); } if (!linesForPins.empty()) { context.Reporter.Info() << std::endl << Resource::String::UpgradeBlockedByPinCount(linesForPins.size()) << std::endl; + SortInstalledPackagesTableLines(context, linesForPins); OutputInstalledPackages(context, linesForPins); } diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 59a22d1913..882f713622 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -58,6 +58,9 @@ namespace AppInstaller::Settings Available, }; + // Converts a string to SortField. Returns std::nullopt for unrecognized values. + std::optional ConvertToSortField(std::string_view value); + // Sort direction for output ordering. enum class SortDirection { @@ -65,9 +68,6 @@ namespace AppInstaller::Settings Descending, }; - // Converts a string to SortField. Returns std::nullopt for unrecognized values. - std::optional ConvertToSortField(std::string_view value); - // The download code to use for *installers*. enum class InstallerDownloader { From a2104239af0f2918a8e1f65b8d551e98549dd73e Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Tue, 28 Apr 2026 16:18:17 -0700 Subject: [PATCH 02/16] [ListSortTests, Refactor] Extract sort logic into testable header and add comprehensive tests --- .../AppInstallerCLITests.vcxproj | 1 + .../AppInstallerCLITests.vcxproj.filters | 3 + src/AppInstallerCLITests/ListSort.cpp | 286 ++++++++++++++++++ 3 files changed, 290 insertions(+) create mode 100644 src/AppInstallerCLITests/ListSort.cpp diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index f796e3d925..71665033fd 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -302,6 +302,7 @@ + diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 5ac6008d1d..896e3062ae 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -236,6 +236,9 @@ Source Files + + Source Files\CLI + Source Files\Common diff --git a/src/AppInstallerCLITests/ListSort.cpp b/src/AppInstallerCLITests/ListSort.cpp new file mode 100644 index 0000000000..1be1b51e11 --- /dev/null +++ b/src/AppInstallerCLITests/ListSort.cpp @@ -0,0 +1,286 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "TestCommon.h" +#include "Workflows/ListSortHelper.h" + +using namespace AppInstaller::CLI::Workflow; +using namespace AppInstaller::Settings; +using namespace AppInstaller::Utility::literals; + +namespace +{ + SortablePackageEntry MakeEntry(std::string name, std::string id, std::string version, std::string available = {}, std::string source = {}) + { + return SortablePackageEntry{ + AppInstaller::Utility::LocIndString{ std::move(name) }, + AppInstaller::Utility::LocIndString{ std::move(id) }, + AppInstaller::Utility::LocIndString{ std::move(version) }, + AppInstaller::Utility::LocIndString{ std::move(available) }, + AppInstaller::Utility::LocIndString{ std::move(source) }, + }; + } + + std::vector GetNames(const std::vector& entries) + { + std::vector names; + for (const auto& e : entries) { names.push_back(e.Name.get()); } + return names; + } + + std::vector GetIds(const std::vector& entries) + { + std::vector ids; + for (const auto& e : entries) { ids.push_back(e.Id.get()); } + return ids; + } +} + +TEST_CASE("ListSort_CompareByField_Name", "[listsort]") +{ + auto a = MakeEntry("Alpha", "a.id", "1.0"); + auto b = MakeEntry("Beta", "b.id", "1.0"); + + SECTION("Less than") + { + REQUIRE(CompareByField(a, b, SortField::Name) < 0); + } + SECTION("Greater than") + { + REQUIRE(CompareByField(b, a, SortField::Name) > 0); + } + SECTION("Equal") + { + REQUIRE(CompareByField(a, a, SortField::Name) == 0); + } + SECTION("Case-insensitive") + { + auto upper = MakeEntry("ALPHA", "a.id", "1.0"); + REQUIRE(CompareByField(a, upper, SortField::Name) == 0); + } +} + +TEST_CASE("ListSort_CompareByField_Id", "[listsort]") +{ + auto a = MakeEntry("Name", "com.alpha", "1.0"); + auto b = MakeEntry("Name", "com.beta", "1.0"); + + REQUIRE(CompareByField(a, b, SortField::Id) < 0); + REQUIRE(CompareByField(b, a, SortField::Id) > 0); + + SECTION("Case-insensitive") + { + auto upper = MakeEntry("Name", "COM.ALPHA", "1.0"); + REQUIRE(CompareByField(a, upper, SortField::Id) == 0); + } +} + +TEST_CASE("ListSort_CompareByField_Version", "[listsort]") +{ + auto v1 = MakeEntry("App", "app", "1.0.0"); + auto v2 = MakeEntry("App", "app", "2.0.0"); + auto v10 = MakeEntry("App", "app", "10.0.0"); + + SECTION("Semantic ordering") + { + REQUIRE(CompareByField(v1, v2, SortField::Version) < 0); + REQUIRE(CompareByField(v2, v1, SortField::Version) > 0); + } + SECTION("Numeric not lexicographic - 10.0 > 2.0") + { + REQUIRE(CompareByField(v10, v2, SortField::Version) > 0); + } + SECTION("Equal versions") + { + REQUIRE(CompareByField(v1, v1, SortField::Version) == 0); + } +} + +TEST_CASE("ListSort_CompareByField_Source", "[listsort]") +{ + auto a = MakeEntry("App", "app", "1.0", "", "msstore"); + auto b = MakeEntry("App", "app", "1.0", "", "winget"); + + REQUIRE(CompareByField(a, b, SortField::Source) < 0); + REQUIRE(CompareByField(b, a, SortField::Source) > 0); +} + +TEST_CASE("ListSort_CompareByField_Available", "[listsort]") +{ + auto withUpdate = MakeEntry("App", "app", "1.0", "2.0"); + auto noUpdate = MakeEntry("App", "app", "1.0", ""); + auto higherUpdate = MakeEntry("App", "app", "1.0", "3.0"); + + SECTION("Has-update before no-update in ascending") + { + REQUIRE(CompareByField(withUpdate, noUpdate, SortField::Available) < 0); + REQUIRE(CompareByField(noUpdate, withUpdate, SortField::Available) > 0); + } + SECTION("Both have updates - compare versions") + { + REQUIRE(CompareByField(withUpdate, higherUpdate, SortField::Available) < 0); + REQUIRE(CompareByField(higherUpdate, withUpdate, SortField::Available) > 0); + } + SECTION("Both empty - equal") + { + REQUIRE(CompareByField(noUpdate, noUpdate, SortField::Available) == 0); + } +} + +TEST_CASE("ListSort_SortEntries_ByName_Ascending", "[listsort]") +{ + std::vector entries = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + }; + + SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + + auto names = GetNames(entries); + REQUIRE(names == std::vector{ "Alpha", "Beta", "Charlie" }); +} + +TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") +{ + std::vector entries = { + MakeEntry("Alpha", "a", "1.0"), + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Beta", "b", "1.0"), + }; + + SortEntries(entries, { SortField::Name }, SortDirection::Descending); + + auto names = GetNames(entries); + REQUIRE(names == std::vector{ "Charlie", "Beta", "Alpha" }); +} + +TEST_CASE("ListSort_SortEntries_ByName_CaseInsensitive", "[listsort]") +{ + std::vector entries = { + MakeEntry("charlie", "c", "1.0"), + MakeEntry("ALPHA", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + }; + + SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + + auto ids = GetIds(entries); + REQUIRE(ids == std::vector{ "a", "b", "c" }); +} + +TEST_CASE("ListSort_SortEntries_ById", "[listsort]") +{ + std::vector entries = { + MakeEntry("Z App", "com.zeta", "1.0"), + MakeEntry("A App", "com.alpha", "1.0"), + MakeEntry("M App", "com.mu", "1.0"), + }; + + SortEntries(entries, { SortField::Id }, SortDirection::Ascending); + + auto ids = GetIds(entries); + REQUIRE(ids == std::vector{ "com.alpha", "com.mu", "com.zeta" }); +} + +TEST_CASE("ListSort_SortEntries_ByVersion", "[listsort]") +{ + std::vector entries = { + MakeEntry("App", "app", "10.0.0"), + MakeEntry("App", "app", "2.0.0"), + MakeEntry("App", "app", "1.0.0"), + }; + + SortEntries(entries, { SortField::Version }, SortDirection::Ascending); + + std::vector versions; + for (const auto& e : entries) { versions.push_back(e.InstalledVersion.get()); } + REQUIRE(versions == std::vector{ "1.0.0", "2.0.0", "10.0.0" }); +} + +TEST_CASE("ListSort_SortEntries_MultiField", "[listsort]") +{ + std::vector entries = { + MakeEntry("Beta", "b.2", "2.0"), + MakeEntry("Alpha", "a.1", "1.0"), + MakeEntry("Beta", "b.1", "1.0"), + MakeEntry("Alpha", "a.2", "2.0"), + }; + + SortEntries(entries, { SortField::Name, SortField::Id }, SortDirection::Ascending); + + auto ids = GetIds(entries); + REQUIRE(ids == std::vector{ "a.1", "a.2", "b.1", "b.2" }); +} + +TEST_CASE("ListSort_SortEntries_Available_GroupsByPresence", "[listsort]") +{ + std::vector entries = { + MakeEntry("NoUpdate1", "n1", "1.0", ""), + MakeEntry("HasUpdate", "h1", "1.0", "2.0"), + MakeEntry("NoUpdate2", "n2", "1.0", ""), + }; + + SortEntries(entries, { SortField::Available }, SortDirection::Ascending); + + auto names = GetNames(entries); + // Has-update first, then no-update (stable within groups) + REQUIRE(names[0] == "HasUpdate"); + REQUIRE(names[1] == "NoUpdate1"); + REQUIRE(names[2] == "NoUpdate2"); +} + +TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") +{ + std::vector entries = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + }; + + // Relevance means preserve original order + SortEntries(entries, { SortField::Relevance }, SortDirection::Ascending); + + auto names = GetNames(entries); + REQUIRE(names == std::vector{ "Charlie", "Alpha", "Beta" }); +} + +TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") +{ + std::vector entries = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + }; + + // Empty sort fields means no sorting + SortEntries(entries, {}, SortDirection::Ascending); + + auto names = GetNames(entries); + REQUIRE(names == std::vector{ "Charlie", "Alpha" }); +} + +TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") +{ + std::vector entries = { + MakeEntry("Only", "only", "1.0"), + }; + + SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + + REQUIRE(entries.size() == 1); + REQUIRE(entries[0].Name.get() == "Only"); +} + +TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") +{ + // Two entries with same Name — stable sort preserves original order + std::vector entries = { + MakeEntry("Same", "first", "1.0"), + MakeEntry("Same", "second", "1.0"), + }; + + SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + + REQUIRE(entries[0].Id.get() == "first"); + REQUIRE(entries[1].Id.get() == "second"); +} From 918fe254875bf9c74cfc64d5ae4accc54a8a542c Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Tue, 28 Apr 2026 17:28:10 -0700 Subject: [PATCH 03/16] [ListSortHelper, Refactor] Move sort implementations from header to cpp file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Every other Workflow header in the project follows the declarations-only convention with a matching .cpp file. ListSortHelper.h was the only outlier using inline implementations. This refactor aligns it with the codebase convention and eliminates the duplicated sort loop between ListSortHelper.h and WorkflowBase.cpp. ## Changes - **ListSortHelper.h → declarations only**: Remove inline implementations of CompareByField and SortEntries, keeping only the struct definition and function signatures - **ListSortHelper.cpp**: New compilation unit with the extracted implementations of CompareByField and SortEntries - **WorkflowBase.cpp sort integration**: Replace the duplicated inline sort loop with an index-based permutation sort that projects InstalledPackagesTableLine into SortablePackageEntry and delegates field comparison to the shared CompareByField - Remove the local CompareByField adapter that was creating temporary SortablePackageEntry copies per comparison call - Align include path and grouping with repo conventions (bare sibling name, local before system) - Simplify GetArgs access to match established MultiQueryFlow pattern ## Impact - Sort logic now lives in a single shared location, reusable by future commands (search, upgrade, font list) without duplication - Follows the established Workflows/ header convention (24/24 other headers use .h + .cpp) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../AppInstallerCLICore.vcxproj | 1 + .../AppInstallerCLICore.vcxproj.filters | 3 + .../Workflows/ListSortHelper.cpp | 87 +++++++++++++++++++ .../Workflows/ListSortHelper.h | 81 ++--------------- .../Workflows/WorkflowBase.cpp | 60 +++++++------ 5 files changed, 131 insertions(+), 101 deletions(-) create mode 100644 src/AppInstallerCLICore/Workflows/ListSortHelper.cpp diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj index 910938ccec..054ae64ae1 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj @@ -460,6 +460,7 @@ + diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters index 80dfb42dd5..bec035ff99 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters @@ -325,6 +325,9 @@ Workflows + + Workflows + Commands diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp b/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp new file mode 100644 index 0000000000..5739f2f150 --- /dev/null +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. +#include "pch.h" +#include "ListSortHelper.h" +#include +#include + +namespace AppInstaller::CLI::Workflow +{ + int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) + { + using Settings::SortField; + + switch (field) + { + case SortField::Name: + return Utility::FoldCase(std::string_view{ a.Name.get() }).compare(Utility::FoldCase(std::string_view{ b.Name.get() })); + case SortField::Id: + return Utility::FoldCase(std::string_view{ a.Id.get() }).compare(Utility::FoldCase(std::string_view{ b.Id.get() })); + case SortField::Version: + { + Utility::Version va(a.InstalledVersion.get()); + Utility::Version vb(b.InstalledVersion.get()); + if (va < vb) return -1; + if (vb < va) return 1; + return 0; + } + case SortField::Source: + return Utility::FoldCase(std::string_view{ a.Source.get() }).compare(Utility::FoldCase(std::string_view{ b.Source.get() })); + case SortField::Available: + { + bool aHas = !a.AvailableVersion.get().empty(); + bool bHas = !b.AvailableVersion.get().empty(); + if (aHas != bHas) + { + // Ascending: has-update sorts before no-update + return aHas ? -1 : 1; + } + if (aHas && bHas) + { + Utility::Version va(a.AvailableVersion.get()); + Utility::Version vb(b.AvailableVersion.get()); + if (va < vb) return -1; + if (vb < va) return 1; + } + return 0; + } + default: + return 0; + } + } + + // Currently used in tests; production code uses CompareByField with an index-based + // permutation sort because workflow-specific row types carry additional fields. + // Intended for reuse by commands (e.g., search, upgrade) whose data maps directly + // to SortablePackageEntry without extra fields. + void SortEntries( + std::vector& entries, + const std::vector& sortFields, + Settings::SortDirection direction) + { + if (entries.size() <= 1 || sortFields.empty()) + { + return; + } + + // Relevance-only means no sorting + if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) + { + return; + } + + std::stable_sort(entries.begin(), entries.end(), + [&sortFields, direction](const SortablePackageEntry& a, const SortablePackageEntry& b) + { + for (const auto& field : sortFields) + { + int cmp = CompareByField(a, b, field); + if (cmp != 0) + { + return direction == Settings::SortDirection::Ascending ? (cmp < 0) : (cmp > 0); + } + } + return false; + }); + } +} diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.h b/src/AppInstallerCLICore/Workflows/ListSortHelper.h index 88fdc2b3bb..eb56429d12 100644 --- a/src/AppInstallerCLICore/Workflows/ListSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.h @@ -3,11 +3,7 @@ #pragma once #include #include -#include -#include -#include -#include #include namespace AppInstaller::CLI::Workflow @@ -25,78 +21,15 @@ namespace AppInstaller::CLI::Workflow // Compares two sortable entries by the given field. // Returns negative if a < b, positive if a > b, 0 if equal. - inline int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) - { - using Settings::SortField; - - switch (field) - { - case SortField::Name: - return Utility::FoldCase(std::string_view{ a.Name.get() }).compare(Utility::FoldCase(std::string_view{ b.Name.get() })); - case SortField::Id: - return Utility::FoldCase(std::string_view{ a.Id.get() }).compare(Utility::FoldCase(std::string_view{ b.Id.get() })); - case SortField::Version: - { - Utility::Version va(a.InstalledVersion.get()); - Utility::Version vb(b.InstalledVersion.get()); - if (va < vb) return -1; - if (vb < va) return 1; - return 0; - } - case SortField::Source: - return Utility::FoldCase(std::string_view{ a.Source.get() }).compare(Utility::FoldCase(std::string_view{ b.Source.get() })); - case SortField::Available: - { - bool aHas = !a.AvailableVersion.get().empty(); - bool bHas = !b.AvailableVersion.get().empty(); - if (aHas != bHas) - { - // Ascending: has-update sorts before no-update - return aHas ? -1 : 1; - } - if (aHas && bHas) - { - Utility::Version va(a.AvailableVersion.get()); - Utility::Version vb(b.AvailableVersion.get()); - if (va < vb) return -1; - if (vb < va) return 1; - } - return 0; - } - default: - return 0; - } - } + int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field); // Sorts a vector of sortable entries by the given fields and direction. - inline void SortEntries( + // Currently used in tests; production code uses CompareByField with an index-based + // permutation sort because workflow-specific row types carry additional fields. + // Intended for reuse by commands (e.g., search, upgrade) whose data maps directly + // to SortablePackageEntry without extra fields. + void SortEntries( std::vector& entries, const std::vector& sortFields, - Settings::SortDirection direction) - { - if (entries.size() <= 1 || sortFields.empty()) - { - return; - } - - // Relevance-only means no sorting - if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) - { - return; - } - - std::stable_sort(entries.begin(), entries.end(), - [&sortFields, direction](const SortablePackageEntry& a, const SortablePackageEntry& b) - { - for (const auto& field : sortFields) - { - int cmp = CompareByField(a, b, field); - if (cmp != 0) - { - return direction == Settings::SortDirection::Ascending ? (cmp < 0) : (cmp > 0); - } - } - return false; - }); - } + Settings::SortDirection direction); } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 3b8629e7d2..c147125693 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -3,13 +3,13 @@ #include "pch.h" #include "WorkflowBase.h" #include "ExecutionContext.h" -#include -#include "Workflows/ListSortHelper.h" -#include +#include "ListSortHelper.h" #include "PromptFlow.h" #include "ShowFlow.h" #include "Sixel.h" #include "TableOutput.h" +#include +#include #include #include #include @@ -461,16 +461,6 @@ namespace AppInstaller::CLI::Workflow } } - // Compares two InstalledPackagesTableLine values by the given field. - int CompareByField(const InstalledPackagesTableLine& a, const InstalledPackagesTableLine& b, SortField field) - { - // Create SortablePackageEntry copies for comparison — fields are layout-compatible - // (both have Name, Id, InstalledVersion, AvailableVersion, Source as LocIndString in same order). - SortablePackageEntry entryA{ a.Name, a.Id, a.InstalledVersion, a.AvailableVersion, a.Source }; - SortablePackageEntry entryB{ b.Name, b.Id, b.InstalledVersion, b.AvailableVersion, b.Source }; - return Workflow::CompareByField(entryA, entryB, field); - } - // Returns true if the execution context contains filter arguments that // produce relevance-ordered results (query, name, id, moniker, tag, command). bool HasRelevanceAffectingArgs(const Execution::Context& context) @@ -498,19 +488,15 @@ namespace AppInstaller::CLI::Workflow if (hasExplicitSort) { - const auto* sortArgs = context.Args.GetArgs(Execution::Args::Type::Sort); - if (sortArgs) + for (const auto& arg : *context.Args.GetArgs(Execution::Args::Type::Sort)) { - for (const auto& arg : *sortArgs) + auto field = ConvertToSortField(arg); + if (field) { - auto field = ConvertToSortField(arg); - if (field) - { - sortFields.emplace_back(field.value()); - } - // Invalid values are silently skipped; argument validation - // should catch these before we get here in a future PR. + sortFields.emplace_back(field.value()); } + // Invalid values are silently skipped; argument validation + // should catch these before we get here in a future PR. } } else if (HasRelevanceAffectingArgs(context)) @@ -550,13 +536,24 @@ namespace AppInstaller::CLI::Workflow direction = User().Get(); } - // 3. Multi-field cascading sort with stable_sort - std::stable_sort(lines.begin(), lines.end(), - [&sortFields, direction](const InstalledPackagesTableLine& a, const InstalledPackagesTableLine& b) + // 3. Project into SortablePackageEntry, sort, then apply the permutation back + std::vector sortable; + sortable.reserve(lines.size()); + for (const auto& line : lines) + { + sortable.push_back({ line.Name, line.Id, line.InstalledVersion, line.AvailableVersion, line.Source }); + } + + // Build index array to track the permutation + std::vector indices(lines.size()); + std::iota(indices.begin(), indices.end(), 0); + + std::stable_sort(indices.begin(), indices.end(), + [&sortable, &sortFields, direction](size_t lhs, size_t rhs) { for (const auto& field : sortFields) { - int cmp = CompareByField(a, b, field); + int cmp = CompareByField(sortable[lhs], sortable[rhs], field); if (cmp != 0) { return direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); @@ -564,6 +561,15 @@ namespace AppInstaller::CLI::Workflow } return false; }); + + // Apply permutation + std::vector sorted; + sorted.reserve(lines.size()); + for (size_t i : indices) + { + sorted.push_back(std::move(lines[i])); + } + lines = std::move(sorted); } void OutputInstalledPackages(Execution::Context& context, std::vector& lines) From 74696082f3d36386776877d76ce6fd6829b1139c Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 13:39:00 -0700 Subject: [PATCH 04/16] [ListSort, Doc] Update list command documentation with sort options Add --sort, --ascending, and --descending options to the options table. Add new 'Sorting output' section documenting: - CLI sort arguments with multi-field examples - Settings-based sortOrder configuration - Resolution order (CLI > settings > default) - Relevance protection behavior for queries --- doc/windows/package-manager/winget/list.md | 43 ++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/doc/windows/package-manager/winget/list.md b/doc/windows/package-manager/winget/list.md index af918a0e38..ba134215d8 100644 --- a/doc/windows/package-manager/winget/list.md +++ b/doc/windows/package-manager/winget/list.md @@ -50,6 +50,9 @@ The options allow you to customize the list experience to meet your needs. | **--upgrade-available** | Lists only packages which have an upgrade available. | | **-u,--unknown,--include-unknown** | List packages even if their current version cannot be determined. Can only be used with the --upgrade-available argument. | | **--pinned,--include-pinned** | List packages even if they have a pin that prevents upgrade. Can only be used with the --upgrade-available argument. | +| **--sort** | Sort results by a property. Can be repeated for multi-field sorting (e.g., `--sort source --sort name`). Valid values: `name`, `id`, `version`, `source`, `available`, `relevance`. | +| **--ascending,--asc** | Sort results in ascending order (default). | +| **--descending,--desc** | Sort results in descending order. | | **-?,--help** | Get additional help on this command. | | **--wait** | Prompts the user to press any key before exiting. | | **--logs,--open-logs** | Open the default logs location. | @@ -70,6 +73,46 @@ The following example limits the output of list to 9 apps. ![list count command](images/list-count.png) +## Sorting output + +By default, results are displayed in the order returned by the package source (no sorting applied). You can control sorting through command-line arguments or user settings. + +### Sort via command-line arguments + +Use `--sort` to sort by one or more fields. When multiple `--sort` options are specified, results are sorted by the first field, then ties are broken by the second field, and so on. + +```cmd +winget list --sort name +winget list --sort source --sort name +winget list --sort name --descending +``` + +### Sort via user settings + +You can set a default sort order in your [settings](https://aka.ms/winget-settings) under `output.sortOrder`: + +```json +{ + "output": { + "sortOrder": ["source", "name"] + } +} +``` + +An empty array (`[]`) results in default sorting (no sorting applied). + +### Resolution order + +When both settings and command-line arguments are present, the following priority applies: + +1. **`--sort` command-line argument** — takes highest priority, overrides settings. +2. **`output.sortOrder` in settings** — used when no `--sort` argument is provided. +3. **Default** — no sorting (source order preserved). + +### Relevance protection + +When a query or filter argument is specified (e.g., `--query`, `--name`, `--id`), the source returns results in relevance order. To preserve this relevance ranking, `output.sortOrder` from settings is **not** applied. Only an explicit `--sort` on the command line can override relevance ordering for queries. This ensures that a global sort preference does not silently change the most relevant results for targeted searches. + ## List with Update As stated above, the **list** command allows you to see what apps you have installed that have updates available. From 6f7773f15a08408db2448f78db205697cdedacb6 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 14:01:03 -0700 Subject: [PATCH 05/16] [ListSort, Validation] Add argument validation for --sort values Validate --sort argument values in Command::ValidateArguments following the same pattern as --scope and --authentication-mode validation. Invalid values produce a clear error message listing valid options (name, id, version, source, available, relevance). Centralized in Command.cpp so any future command adding --sort gets validation automatically. --- src/AppInstallerCLICore/Command.cpp | 13 +++++++++++++ src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 4 ++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/AppInstallerCLICore/Command.cpp b/src/AppInstallerCLICore/Command.cpp index 7da363c616..b24ce3050d 100644 --- a/src/AppInstallerCLICore/Command.cpp +++ b/src/AppInstallerCLICore/Command.cpp @@ -806,6 +806,19 @@ namespace AppInstaller::CLI } } + if (execArgs.Contains(Execution::Args::Type::Sort)) + { + for (const auto& arg : *execArgs.GetArgs(Execution::Args::Type::Sort)) + { + if (!Settings::ConvertToSortField(arg)) + { + auto validOptions = Utility::Join(", "_liv, std::vector{ + "name"_lis, "id"_lis, "version"_lis, "source"_lis, "available"_lis, "relevance"_lis }); + throw CommandException(Resource::String::InvalidArgumentValueError(ArgumentCommon::ForType(Execution::Args::Type::Sort).Name, validOptions)); + } + } + } + Argument::ValidateExclusiveArguments(execArgs); ValidateArgumentsInternal(execArgs); diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index c147125693..473d2caec6 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -495,8 +495,8 @@ namespace AppInstaller::CLI::Workflow { sortFields.emplace_back(field.value()); } - // Invalid values are silently skipped; argument validation - // should catch these before we get here in a future PR. + // Invalid values should not reach here; ValidateArgumentsInternal + // rejects them with an error before workflow execution begins. } } else if (HasRelevanceAffectingArgs(context)) From 813c8af1daad08840907fdcba7a8d096648f4d4f Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 14:17:47 -0700 Subject: [PATCH 06/16] [ListSort, SpellCheck] Add listsort to spelling expect list Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/actions/spelling/expect.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index 53175f0afa..2e16b1d0c4 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -290,6 +290,7 @@ LEBOM lhs LIBYAML liv +listsort liwpx localizationpriority localsource From a32a676d6e69333a76e9ce310feb56b527ce3bfc Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 16:08:13 -0700 Subject: [PATCH 07/16] [ListSort, Design] Respect settings sort with queries per reviewer feedback - Remove HasRelevanceAffectingArgs function (overly broad filter) - Narrow relevance preservation to only the free-text query argument - Settings output.sortOrder now applies even when query is present (user explicitly configured sort preference takes priority) - Filter args (--id, --name, --tag etc.) no longer block settings sort since they use exact/substring matching with no meaningful relevance - Update list.md documentation to reflect new behavior - Resolution: CLI --sort > settings (always) > default relevance (query only) --- doc/windows/package-manager/winget/list.md | 9 +++-- .../Workflows/WorkflowBase.cpp | 33 ++++++++----------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/doc/windows/package-manager/winget/list.md b/doc/windows/package-manager/winget/list.md index ba134215d8..3fd7dc8a23 100644 --- a/doc/windows/package-manager/winget/list.md +++ b/doc/windows/package-manager/winget/list.md @@ -106,12 +106,15 @@ An empty array (`[]`) results in default sorting (no sorting applied). When both settings and command-line arguments are present, the following priority applies: 1. **`--sort` command-line argument** — takes highest priority, overrides settings. -2. **`output.sortOrder` in settings** — used when no `--sort` argument is provided. +2. **`output.sortOrder` in settings** — used when no `--sort` argument is provided. If the user has configured a sort order in settings, it is applied even when a query is present. 3. **Default** — no sorting (source order preserved). -### Relevance protection +### Default relevance preservation -When a query or filter argument is specified (e.g., `--query`, `--name`, `--id`), the source returns results in relevance order. To preserve this relevance ranking, `output.sortOrder` from settings is **not** applied. Only an explicit `--sort` on the command line can override relevance ordering for queries. This ensures that a global sort preference does not silently change the most relevant results for targeted searches. +When using a free-text query (e.g., `winget list Microsoft`) and no sort preference is configured in settings, results are displayed in relevance order as returned by the source. This preserves the most relevant matches at the top by default. If you configure `output.sortOrder` in settings, that preference is respected even for queries. An explicit `--sort` on the command line always overrides both settings and the default relevance ordering. + +> [!NOTE] +> Only the free-text query argument (`-q`/`--query`) has meaningful relevance ranking. Filter arguments like `--id`, `--name`, or `--tag` use exact or substring matching where results have equivalent relevance, so settings sort always applies for those. ## List with Update diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 473d2caec6..0e91eb2066 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -463,16 +463,6 @@ namespace AppInstaller::CLI::Workflow // Returns true if the execution context contains filter arguments that // produce relevance-ordered results (query, name, id, moniker, tag, command). - bool HasRelevanceAffectingArgs(const Execution::Context& context) - { - return context.Args.Contains(Execution::Args::Type::Query) || - context.Args.Contains(Execution::Args::Type::Id) || - context.Args.Contains(Execution::Args::Type::Name) || - context.Args.Contains(Execution::Args::Type::Moniker) || - context.Args.Contains(Execution::Args::Type::Tag) || - context.Args.Contains(Execution::Args::Type::Command); - } - // Sorts a vector of InstalledPackagesTableLine according to the user's sort preferences. // Resolution order: CLI args (--sort) > settings (output.sortOrder) > query-aware default. void SortInstalledPackagesTableLines(Execution::Context& context, std::vector& lines) @@ -499,19 +489,22 @@ namespace AppInstaller::CLI::Workflow // rejects them with an error before workflow execution begins. } } - else if (HasRelevanceAffectingArgs(context)) - { - // Design: when query/filter arguments are present, relevance ordering is - // always preserved unless the user explicitly passes --sort on the CLI. - // Settings sortOrder intentionally does NOT override relevance for queries, - // because query results are ranked by match quality and reordering them - // would hide the best matches. Only --sort (explicit user intent per-command) - // can override this. - return; - } else { sortFields = User().Get(); + + // When the free-text query argument is present and the user has NOT + // configured a sort preference in settings, preserve relevance ordering. + // Only the positional query argument populates searchRequest.Query and + // produces meaningful relevance ranking; filter arguments like --id, + // --name, --tag etc. use exact/substring matching where all results + // have equivalent relevance. + // If the user explicitly configured output.sortOrder, respect it even + // with queries — that is an explicit user preference. + if (sortFields.empty() && context.Args.Contains(Execution::Args::Type::Query)) + { + return; + } } // Empty sort fields or relevance-only means no sorting From 46a57073e33d3220e2adf8f3a7cad56921d23d70 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 21:36:37 -0700 Subject: [PATCH 08/16] [ListSort, Fix] Code correctness per reviewer feedback - Fix comment: 'enable' -> 'ease' unit testing (not impossible, just decoupled) - Remove stale comment from deleted HasRelevanceAffectingArgs function - Add THROW_HR(E_UNEXPECTED) assertion for invalid sort values that bypass validation - Change --sort count limit from 7 to 6 (matches actual SortField enum count) --- src/AppInstallerCLICore/Commands/ListCommand.cpp | 4 +--- src/AppInstallerCLICore/Workflows/ListSortHelper.h | 2 +- src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 10 ++++++---- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/ListCommand.cpp b/src/AppInstallerCLICore/Commands/ListCommand.cpp index b663ab1462..310a8038e0 100644 --- a/src/AppInstallerCLICore/Commands/ListCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ListCommand.cpp @@ -32,9 +32,7 @@ namespace AppInstaller::CLI Argument{ Execution::Args::Type::IncludeUnknown, Resource::String::IncludeUnknownInListArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::IncludePinned, Resource::String::IncludePinnedInListArgumentDescription, ArgumentType::Flag}, Argument::ForType(Execution::Args::Type::ListDetails), - // 7 = one more than the number of user-facing SortField values (Relevance, Name, Id, Version, Source, Available), - // allowing all fields plus a small buffer for future additions. - Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(7), + Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(6), Argument{ Execution::Args::Type::SortAscending, Resource::String::SortAscendingArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::SortDescending, Resource::String::SortDescendingArgumentDescription, ArgumentType::Flag }, }; diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.h b/src/AppInstallerCLICore/Workflows/ListSortHelper.h index eb56429d12..3e42cc5a77 100644 --- a/src/AppInstallerCLICore/Workflows/ListSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.h @@ -9,7 +9,7 @@ namespace AppInstaller::CLI::Workflow { // Lightweight sortable representation of a package line in list output. - // Decoupled from ICompositePackage/IPackageVersion to enable unit testing. + // Decoupled from ICompositePackage/IPackageVersion to ease unit testing. struct SortablePackageEntry { Utility::LocIndString Name; diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 0e91eb2066..f25461fe1c 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -461,8 +461,6 @@ namespace AppInstaller::CLI::Workflow } } - // Returns true if the execution context contains filter arguments that - // produce relevance-ordered results (query, name, id, moniker, tag, command). // Sorts a vector of InstalledPackagesTableLine according to the user's sort preferences. // Resolution order: CLI args (--sort) > settings (output.sortOrder) > query-aware default. void SortInstalledPackagesTableLines(Execution::Context& context, std::vector& lines) @@ -485,8 +483,12 @@ namespace AppInstaller::CLI::Workflow { sortFields.emplace_back(field.value()); } - // Invalid values should not reach here; ValidateArgumentsInternal - // rejects them with an error before workflow execution begins. + else + { + // Invalid values should not reach here; ValidateArgumentsInternal + // rejects them with an error before workflow execution begins. + THROW_HR(E_UNEXPECTED); + } } } else From dc6578840ec1812ae3fd77da26064fd936c206e2 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 21:37:10 -0700 Subject: [PATCH 09/16] [ListSort, Design] Default sort by name when no settings or query When no sort preferences are configured (no CLI --sort, no settings) and no positional query is present, default to sorting by name ascending for deterministic, user-friendly output. With a positional query, relevance ordering is preserved unless the user explicitly configures sort preferences. Update list.md to document this conditional default behavior. --- doc/windows/package-manager/winget/list.md | 2 +- src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/doc/windows/package-manager/winget/list.md b/doc/windows/package-manager/winget/list.md index 3fd7dc8a23..cffbc3fd21 100644 --- a/doc/windows/package-manager/winget/list.md +++ b/doc/windows/package-manager/winget/list.md @@ -75,7 +75,7 @@ The following example limits the output of list to 9 apps. ## Sorting output -By default, results are displayed in the order returned by the package source (no sorting applied). You can control sorting through command-line arguments or user settings. +By default, results are sorted by name in ascending order. When a query argument is used (for example, `winget list foo`), results preserve relevance ordering from the package source. You can override either default through command-line arguments or user settings. ### Sort via command-line arguments diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index f25461fe1c..c6830c4f72 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -507,6 +507,13 @@ namespace AppInstaller::CLI::Workflow { return; } + + // No settings configured and no query — apply default sort by name + // so that output is deterministic and user-friendly. + if (sortFields.empty()) + { + sortFields.emplace_back(SortField::Name); + } } // Empty sort fields or relevance-only means no sorting From ea81abd058fe6bca09a151e0d79a32bab800ecc8 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 21:41:09 -0700 Subject: [PATCH 10/16] [ListSort, Refactor] Redesign helper as production sort pipeline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restructure ListSortHelper to own the production sort path: - SortablePackageEntry now precomputes case-folded strings and parsed versions at construction time, avoiding repeated FoldCase/Version parsing during comparisons. - Add OriginalIndex field so entries track their source position. - Add SortBy template that converts arbitrary item vectors into SortablePackageEntry projections, sorts, then reorders the source vector to match. This is the production code path. - WorkflowBase calls SortBy with a conversion lambda, replacing the inline permutation sort. - SortEntries is no longer test-only — it is the same code path that production uses via SortBy. - Add SortBy template tests to validate the end-to-end pipeline with custom source types. --- .../Workflows/ListSortHelper.cpp | 52 +++++++----- .../Workflows/ListSortHelper.h | 78 +++++++++++++++--- .../Workflows/WorkflowBase.cpp | 44 +++------- src/AppInstallerCLITests/ListSort.cpp | 82 +++++++++++++------ 4 files changed, 165 insertions(+), 91 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp b/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp index 5739f2f150..e90fbd7701 100644 --- a/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp @@ -2,11 +2,29 @@ // Licensed under the MIT License. #include "pch.h" #include "ListSortHelper.h" -#include -#include namespace AppInstaller::CLI::Workflow { + SortablePackageEntry::SortablePackageEntry( + size_t originalIndex, + std::string_view name, + std::string_view id, + std::string_view installedVersion, + std::string_view availableVersion, + std::string_view source) + : OriginalIndex(originalIndex), + FoldedName(Utility::FoldCase(name)), + FoldedId(Utility::FoldCase(id)), + FoldedSource(Utility::FoldCase(source)), + ParsedInstalledVersion(std::string{ installedVersion }), + HasAvailableVersion(!availableVersion.empty()) + { + if (HasAvailableVersion) + { + ParsedAvailableVersion = Utility::Version{ std::string{ availableVersion } }; + } + } + int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) { using Settings::SortField; @@ -14,34 +32,28 @@ namespace AppInstaller::CLI::Workflow switch (field) { case SortField::Name: - return Utility::FoldCase(std::string_view{ a.Name.get() }).compare(Utility::FoldCase(std::string_view{ b.Name.get() })); + return a.FoldedName.compare(b.FoldedName); case SortField::Id: - return Utility::FoldCase(std::string_view{ a.Id.get() }).compare(Utility::FoldCase(std::string_view{ b.Id.get() })); + return a.FoldedId.compare(b.FoldedId); case SortField::Version: { - Utility::Version va(a.InstalledVersion.get()); - Utility::Version vb(b.InstalledVersion.get()); - if (va < vb) return -1; - if (vb < va) return 1; + if (a.ParsedInstalledVersion < b.ParsedInstalledVersion) return -1; + if (b.ParsedInstalledVersion < a.ParsedInstalledVersion) return 1; return 0; } case SortField::Source: - return Utility::FoldCase(std::string_view{ a.Source.get() }).compare(Utility::FoldCase(std::string_view{ b.Source.get() })); + return a.FoldedSource.compare(b.FoldedSource); case SortField::Available: { - bool aHas = !a.AvailableVersion.get().empty(); - bool bHas = !b.AvailableVersion.get().empty(); - if (aHas != bHas) + if (a.HasAvailableVersion != b.HasAvailableVersion) { // Ascending: has-update sorts before no-update - return aHas ? -1 : 1; + return a.HasAvailableVersion ? -1 : 1; } - if (aHas && bHas) + if (a.HasAvailableVersion && b.HasAvailableVersion) { - Utility::Version va(a.AvailableVersion.get()); - Utility::Version vb(b.AvailableVersion.get()); - if (va < vb) return -1; - if (vb < va) return 1; + if (a.ParsedAvailableVersion < b.ParsedAvailableVersion) return -1; + if (b.ParsedAvailableVersion < a.ParsedAvailableVersion) return 1; } return 0; } @@ -50,10 +62,6 @@ namespace AppInstaller::CLI::Workflow } } - // Currently used in tests; production code uses CompareByField with an index-based - // permutation sort because workflow-specific row types carry additional fields. - // Intended for reuse by commands (e.g., search, upgrade) whose data maps directly - // to SortablePackageEntry without extra fields. void SortEntries( std::vector& entries, const std::vector& sortFields, diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.h b/src/AppInstallerCLICore/Workflows/ListSortHelper.h index 3e42cc5a77..7c55f9c407 100644 --- a/src/AppInstallerCLICore/Workflows/ListSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/ListSortHelper.h @@ -1,35 +1,89 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #pragma once -#include +#include +#include #include +#include +#include #include namespace AppInstaller::CLI::Workflow { - // Lightweight sortable representation of a package line in list output. + // Lightweight sortable representation of a package row with precomputed sort keys. // Decoupled from ICompositePackage/IPackageVersion to ease unit testing. struct SortablePackageEntry { - Utility::LocIndString Name; - Utility::LocIndString Id; - Utility::LocIndString InstalledVersion; - Utility::LocIndString AvailableVersion; - Utility::LocIndString Source; + size_t OriginalIndex = 0; + + // Precomputed case-folded sort keys + std::string FoldedName; + std::string FoldedId; + std::string FoldedSource; + + // Precomputed parsed versions + Utility::Version ParsedInstalledVersion; + Utility::Version ParsedAvailableVersion; + bool HasAvailableVersion = false; + + SortablePackageEntry() = default; + + SortablePackageEntry( + size_t originalIndex, + std::string_view name, + std::string_view id, + std::string_view installedVersion, + std::string_view availableVersion, + std::string_view source); }; - // Compares two sortable entries by the given field. + // Compares two sortable entries by the given field using precomputed sort keys. // Returns negative if a < b, positive if a > b, 0 if equal. int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field); // Sorts a vector of sortable entries by the given fields and direction. - // Currently used in tests; production code uses CompareByField with an index-based - // permutation sort because workflow-specific row types carry additional fields. - // Intended for reuse by commands (e.g., search, upgrade) whose data maps directly - // to SortablePackageEntry without extra fields. void SortEntries( std::vector& entries, const std::vector& sortFields, Settings::SortDirection direction); + + // Sorts a vector of arbitrary items by projecting each into a SortablePackageEntry + // via a caller-supplied converter, sorting the projections, then reordering the + // source vector to match. The converter signature is: + // SortablePackageEntry converter(const T& item, size_t index) + template + void SortBy( + std::vector& items, + Converter&& converter, + const std::vector& sortFields, + Settings::SortDirection direction) + { + if (items.size() <= 1 || sortFields.empty()) + { + return; + } + + if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) + { + return; + } + + std::vector entries; + entries.reserve(items.size()); + for (size_t i = 0; i < items.size(); ++i) + { + entries.push_back(converter(items[i], i)); + } + + SortEntries(entries, sortFields, direction); + + std::vector sorted; + sorted.reserve(items.size()); + for (const auto& entry : entries) + { + sorted.push_back(std::move(items[entry.OriginalIndex])); + } + items = std::move(sorted); + } } diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index c6830c4f72..aefed436d9 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -538,40 +538,16 @@ namespace AppInstaller::CLI::Workflow direction = User().Get(); } - // 3. Project into SortablePackageEntry, sort, then apply the permutation back - std::vector sortable; - sortable.reserve(lines.size()); - for (const auto& line : lines) - { - sortable.push_back({ line.Name, line.Id, line.InstalledVersion, line.AvailableVersion, line.Source }); - } - - // Build index array to track the permutation - std::vector indices(lines.size()); - std::iota(indices.begin(), indices.end(), 0); - - std::stable_sort(indices.begin(), indices.end(), - [&sortable, &sortFields, direction](size_t lhs, size_t rhs) - { - for (const auto& field : sortFields) - { - int cmp = CompareByField(sortable[lhs], sortable[rhs], field); - if (cmp != 0) - { - return direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); - } - } - return false; - }); - - // Apply permutation - std::vector sorted; - sorted.reserve(lines.size()); - for (size_t i : indices) - { - sorted.push_back(std::move(lines[i])); - } - lines = std::move(sorted); + // 3. Sort using the helper's production pipeline + SortBy(lines, + [](const InstalledPackagesTableLine& line, size_t index) { + return SortablePackageEntry( + index, + line.Name.get(), line.Id.get(), + line.InstalledVersion.get(), line.AvailableVersion.get(), + line.Source.get()); + }, + sortFields, direction); } void OutputInstalledPackages(Execution::Context& context, std::vector& lines) diff --git a/src/AppInstallerCLITests/ListSort.cpp b/src/AppInstallerCLITests/ListSort.cpp index 1be1b51e11..663a9c7a45 100644 --- a/src/AppInstallerCLITests/ListSort.cpp +++ b/src/AppInstallerCLITests/ListSort.cpp @@ -6,32 +6,25 @@ using namespace AppInstaller::CLI::Workflow; using namespace AppInstaller::Settings; -using namespace AppInstaller::Utility::literals; namespace { SortablePackageEntry MakeEntry(std::string name, std::string id, std::string version, std::string available = {}, std::string source = {}) { - return SortablePackageEntry{ - AppInstaller::Utility::LocIndString{ std::move(name) }, - AppInstaller::Utility::LocIndString{ std::move(id) }, - AppInstaller::Utility::LocIndString{ std::move(version) }, - AppInstaller::Utility::LocIndString{ std::move(available) }, - AppInstaller::Utility::LocIndString{ std::move(source) }, - }; + return SortablePackageEntry{ 0, name, id, version, available, source }; } std::vector GetNames(const std::vector& entries) { std::vector names; - for (const auto& e : entries) { names.push_back(e.Name.get()); } + for (const auto& e : entries) { names.push_back(e.FoldedName); } return names; } std::vector GetIds(const std::vector& entries) { std::vector ids; - for (const auto& e : entries) { ids.push_back(e.Id.get()); } + for (const auto& e : entries) { ids.push_back(e.FoldedId); } return ids; } } @@ -138,7 +131,7 @@ TEST_CASE("ListSort_SortEntries_ByName_Ascending", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); auto names = GetNames(entries); - REQUIRE(names == std::vector{ "Alpha", "Beta", "Charlie" }); + REQUIRE(names == std::vector{ "alpha", "beta", "charlie" }); } TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") @@ -152,7 +145,7 @@ TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Descending); auto names = GetNames(entries); - REQUIRE(names == std::vector{ "Charlie", "Beta", "Alpha" }); + REQUIRE(names == std::vector{ "charlie", "beta", "alpha" }); } TEST_CASE("ListSort_SortEntries_ByName_CaseInsensitive", "[listsort]") @@ -193,9 +186,9 @@ TEST_CASE("ListSort_SortEntries_ByVersion", "[listsort]") SortEntries(entries, { SortField::Version }, SortDirection::Ascending); - std::vector versions; - for (const auto& e : entries) { versions.push_back(e.InstalledVersion.get()); } - REQUIRE(versions == std::vector{ "1.0.0", "2.0.0", "10.0.0" }); + // Version is precomputed, verify order via ParsedInstalledVersion + REQUIRE(entries[0].ParsedInstalledVersion < entries[1].ParsedInstalledVersion); + REQUIRE(entries[1].ParsedInstalledVersion < entries[2].ParsedInstalledVersion); } TEST_CASE("ListSort_SortEntries_MultiField", "[listsort]") @@ -225,9 +218,9 @@ TEST_CASE("ListSort_SortEntries_Available_GroupsByPresence", "[listsort]") auto names = GetNames(entries); // Has-update first, then no-update (stable within groups) - REQUIRE(names[0] == "HasUpdate"); - REQUIRE(names[1] == "NoUpdate1"); - REQUIRE(names[2] == "NoUpdate2"); + REQUIRE(names[0] == "hasupdate"); + REQUIRE(names[1] == "noupdate1"); + REQUIRE(names[2] == "noupdate2"); } TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") @@ -242,7 +235,7 @@ TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") SortEntries(entries, { SortField::Relevance }, SortDirection::Ascending); auto names = GetNames(entries); - REQUIRE(names == std::vector{ "Charlie", "Alpha", "Beta" }); + REQUIRE(names == std::vector{ "charlie", "alpha", "beta" }); } TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") @@ -256,7 +249,7 @@ TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") SortEntries(entries, {}, SortDirection::Ascending); auto names = GetNames(entries); - REQUIRE(names == std::vector{ "Charlie", "Alpha" }); + REQUIRE(names == std::vector{ "charlie", "alpha" }); } TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") @@ -268,7 +261,7 @@ TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); REQUIRE(entries.size() == 1); - REQUIRE(entries[0].Name.get() == "Only"); + REQUIRE(entries[0].FoldedName == "only"); } TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") @@ -281,6 +274,49 @@ TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); - REQUIRE(entries[0].Id.get() == "first"); - REQUIRE(entries[1].Id.get() == "second"); + REQUIRE(entries[0].FoldedId == "first"); + REQUIRE(entries[1].FoldedId == "second"); +} + +// Tests for SortBy template — validates the production sort pipeline +// that converts arbitrary types to SortablePackageEntry and reorders in place. +TEST_CASE("ListSort_SortBy_ReordersSourceItems", "[listsort]") +{ + struct Row { std::string name; std::string id; std::string ver; int extra; }; + + std::vector rows = { + { "Charlie", "c", "1.0", 100 }, + { "Alpha", "a", "1.0", 200 }, + { "Beta", "b", "1.0", 300 }, + }; + + SortBy(rows, + [](const Row& r, size_t i) { + return SortablePackageEntry(i, r.name, r.id, r.ver, "", ""); + }, + { SortField::Name }, SortDirection::Ascending); + + REQUIRE(rows[0].name == "Alpha"); + REQUIRE(rows[0].extra == 200); + REQUIRE(rows[1].name == "Beta"); + REQUIRE(rows[2].name == "Charlie"); +} + +TEST_CASE("ListSort_SortBy_PreservesExtraFields", "[listsort]") +{ + struct Row { std::string name; std::string payload; }; + + std::vector rows = { + { "Zeta", "payload-z" }, + { "Alpha", "payload-a" }, + }; + + SortBy(rows, + [](const Row& r, size_t i) { + return SortablePackageEntry(i, r.name, "", "", "", ""); + }, + { SortField::Name }, SortDirection::Ascending); + + REQUIRE(rows[0].payload == "payload-a"); + REQUIRE(rows[1].payload == "payload-z"); } From 0d86ce9513bd0316f4f4d92af659f66b93c6b7e4 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 29 Apr 2026 21:42:07 -0700 Subject: [PATCH 11/16] [ListSort, Rename] Rename ListSortHelper to PackageTableSortHelper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename to reflect broader scope — the helper sorts package table rows for any table-based command (list, search, upgrade), not just list. --- src/AppInstallerCLICore/AppInstallerCLICore.vcxproj | 4 ++-- src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters | 4 ++-- .../{ListSortHelper.cpp => PackageTableSortHelper.cpp} | 2 +- .../Workflows/{ListSortHelper.h => PackageTableSortHelper.h} | 0 src/AppInstallerCLICore/Workflows/WorkflowBase.cpp | 2 +- src/AppInstallerCLITests/ListSort.cpp | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) rename src/AppInstallerCLICore/Workflows/{ListSortHelper.cpp => PackageTableSortHelper.cpp} (95%) rename src/AppInstallerCLICore/Workflows/{ListSortHelper.h => PackageTableSortHelper.h} (100%) diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj index 054ae64ae1..19a4f62cc3 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj @@ -366,7 +366,7 @@ - + @@ -460,7 +460,7 @@ - + diff --git a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters index bec035ff99..f6e9ca7dd2 100644 --- a/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters +++ b/src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters @@ -50,7 +50,7 @@ Workflows - + Workflows @@ -325,7 +325,7 @@ Workflows - + Workflows diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp similarity index 95% rename from src/AppInstallerCLICore/Workflows/ListSortHelper.cpp rename to src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp index e90fbd7701..8066c5a6fa 100644 --- a/src/AppInstallerCLICore/Workflows/ListSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp @@ -1,7 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. #include "pch.h" -#include "ListSortHelper.h" +#include "PackageTableSortHelper.h" namespace AppInstaller::CLI::Workflow { diff --git a/src/AppInstallerCLICore/Workflows/ListSortHelper.h b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h similarity index 100% rename from src/AppInstallerCLICore/Workflows/ListSortHelper.h rename to src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index aefed436d9..fe71e98a5c 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -3,7 +3,7 @@ #include "pch.h" #include "WorkflowBase.h" #include "ExecutionContext.h" -#include "ListSortHelper.h" +#include "PackageTableSortHelper.h" #include "PromptFlow.h" #include "ShowFlow.h" #include "Sixel.h" diff --git a/src/AppInstallerCLITests/ListSort.cpp b/src/AppInstallerCLITests/ListSort.cpp index 663a9c7a45..48883ed8d1 100644 --- a/src/AppInstallerCLITests/ListSort.cpp +++ b/src/AppInstallerCLITests/ListSort.cpp @@ -2,7 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "TestCommon.h" -#include "Workflows/ListSortHelper.h" +#include "Workflows/PackageTableSortHelper.h" using namespace AppInstaller::CLI::Workflow; using namespace AppInstaller::Settings; From aaf8e9c7a2b71920995576e8fb2f839b889d86be Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Fri, 1 May 2026 12:00:56 -0700 Subject: [PATCH 12/16] [ListSort, Optimize] Skip unused field computation via bitmask Change SortField to flag-bit enum (uint32_t) and add DEFINE_ENUM_FLAG_OPERATORS so sort fields compose into a bitmask. SortBy computes the mask once and passes it through the converter to SortablePackageEntry's constructor, which now only precomputes fields that are actually needed for the requested sort. --- .../Workflows/PackageTableSortHelper.cpp | 34 ++++++++++++++----- .../Workflows/PackageTableSortHelper.h | 18 ++++++++-- .../Workflows/WorkflowBase.cpp | 5 +-- src/AppInstallerCLITests/ListSort.cpp | 12 +++++-- .../Public/winget/UserSettings.h | 19 ++++++----- 5 files changed, 64 insertions(+), 24 deletions(-) diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp index 8066c5a6fa..1482d0038d 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp @@ -11,17 +11,33 @@ namespace AppInstaller::CLI::Workflow std::string_view id, std::string_view installedVersion, std::string_view availableVersion, - std::string_view source) - : OriginalIndex(originalIndex), - FoldedName(Utility::FoldCase(name)), - FoldedId(Utility::FoldCase(id)), - FoldedSource(Utility::FoldCase(source)), - ParsedInstalledVersion(std::string{ installedVersion }), - HasAvailableVersion(!availableVersion.empty()) + std::string_view source, + Settings::SortField fieldMask) + : OriginalIndex(originalIndex) { - if (HasAvailableVersion) + if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Name)) { - ParsedAvailableVersion = Utility::Version{ std::string{ availableVersion } }; + FoldedName = Utility::FoldCase(name); + } + if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Id)) + { + FoldedId = Utility::FoldCase(id); + } + if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Source)) + { + FoldedSource = Utility::FoldCase(source); + } + if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Version)) + { + ParsedInstalledVersion = Utility::Version{ std::string{ installedVersion } }; + } + if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Available)) + { + HasAvailableVersion = !availableVersion.empty(); + if (HasAvailableVersion) + { + ParsedAvailableVersion = Utility::Version{ std::string{ availableVersion } }; + } } } diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h index 7c55f9c407..941f6706bb 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h @@ -6,7 +6,6 @@ #include #include -#include #include namespace AppInstaller::CLI::Workflow @@ -35,7 +34,8 @@ namespace AppInstaller::CLI::Workflow std::string_view id, std::string_view installedVersion, std::string_view availableVersion, - std::string_view source); + std::string_view source, + Settings::SortField fieldMask); }; // Compares two sortable entries by the given field using precomputed sort keys. @@ -48,10 +48,24 @@ namespace AppInstaller::CLI::Workflow const std::vector& sortFields, Settings::SortDirection direction); + // Computes a bitmask of all sort fields so the constructor can skip unused fields. + inline Settings::SortField ComputeSortFieldMask(const std::vector& sortFields) + { + Settings::SortField mask = Settings::SortField::Relevance; + for (const auto& f : sortFields) + { + mask |= f; + } + return mask; + } + // Sorts a vector of arbitrary items by projecting each into a SortablePackageEntry // via a caller-supplied converter, sorting the projections, then reordering the // source vector to match. The converter signature is: // SortablePackageEntry converter(const T& item, size_t index) + // The caller is responsible for pre-computing the SortFieldMask and capturing + // it in the converter closure so the SortablePackageEntry constructor only + // initializes the fields actually needed for comparison. template void SortBy( std::vector& items, diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index fe71e98a5c..031578eb87 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -539,13 +539,14 @@ namespace AppInstaller::CLI::Workflow } // 3. Sort using the helper's production pipeline + const SortField mask = ComputeSortFieldMask(sortFields); SortBy(lines, - [](const InstalledPackagesTableLine& line, size_t index) { + [mask](const InstalledPackagesTableLine& line, size_t index) { return SortablePackageEntry( index, line.Name.get(), line.Id.get(), line.InstalledVersion.get(), line.AvailableVersion.get(), - line.Source.get()); + line.Source.get(), mask); }, sortFields, direction); } diff --git a/src/AppInstallerCLITests/ListSort.cpp b/src/AppInstallerCLITests/ListSort.cpp index 48883ed8d1..15c2bf19cc 100644 --- a/src/AppInstallerCLITests/ListSort.cpp +++ b/src/AppInstallerCLITests/ListSort.cpp @@ -9,9 +9,15 @@ using namespace AppInstaller::Settings; namespace { + // Use all-fields mask for tests so every field is precomputed. + constexpr SortField AllFieldsMask = + SortField::Name | SortField::Id | + SortField::Version | SortField::Source | + SortField::Available; + SortablePackageEntry MakeEntry(std::string name, std::string id, std::string version, std::string available = {}, std::string source = {}) { - return SortablePackageEntry{ 0, name, id, version, available, source }; + return SortablePackageEntry{ 0, name, id, version, available, source, AllFieldsMask }; } std::vector GetNames(const std::vector& entries) @@ -292,7 +298,7 @@ TEST_CASE("ListSort_SortBy_ReordersSourceItems", "[listsort]") SortBy(rows, [](const Row& r, size_t i) { - return SortablePackageEntry(i, r.name, r.id, r.ver, "", ""); + return SortablePackageEntry(i, r.name, r.id, r.ver, "", "", AllFieldsMask); }, { SortField::Name }, SortDirection::Ascending); @@ -313,7 +319,7 @@ TEST_CASE("ListSort_SortBy_PreservesExtraFields", "[listsort]") SortBy(rows, [](const Row& r, size_t i) { - return SortablePackageEntry(i, r.name, "", "", "", ""); + return SortablePackageEntry(i, r.name, "", "", "", "", AllFieldsMask); }, { SortField::Name }, SortDirection::Ascending); diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 882f713622..9b378b3347 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -47,17 +47,20 @@ namespace AppInstaller::Settings Disabled, }; - // Sort field for output ordering. - enum class SortField + // Sort field for output ordering. Flag-bit values enable bitmask composition + // via ComputeSortFieldMask, so the constructor can skip unused field computation. + enum class SortField : uint32_t { - Relevance, // Preserves current natural order (source-defined relevance ranking) - Name, - Id, - Version, - Source, - Available, + Relevance = 0x0, // Preserves current natural order (source-defined relevance ranking) + Name = 0x1, + Id = 0x2, + Version = 0x4, + Source = 0x8, + Available = 0x10, }; + DEFINE_ENUM_FLAG_OPERATORS(SortField); + // Converts a string to SortField. Returns std::nullopt for unrecognized values. std::optional ConvertToSortField(std::string_view value); From 8ac341df7283b430e82b638efce502d0f8fbf7bb Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Fri, 1 May 2026 12:21:03 -0700 Subject: [PATCH 13/16] [ListSort, Polish] Compile-time drift guard, code cleanup, and docs - Add static_assert on SortField::Available at enum definition site to catch new values that bypass constructor handling. - Replace invalid-sort assertion with FAIL_FAST_MSG for clearer diagnostics. - Collapse two consecutive sortFields.empty() checks into a single branch. - Format SortablePackageEntry constructor arguments one-per-line. - Add --query example to list.md Example queries section. --- doc/windows/package-manager/winget/list.md | 11 +--- .../Workflows/WorkflowBase.cpp | 50 ++++++++--------- .../AppInstallerCLITests.vcxproj | 2 +- .../AppInstallerCLITests.vcxproj.filters | 2 +- src/AppInstallerCLITests/Command.cpp | 54 +++++++++++++++++++ ...istSort.cpp => PackageTableSortHelper.cpp} | 0 6 files changed, 84 insertions(+), 35 deletions(-) rename src/AppInstallerCLITests/{ListSort.cpp => PackageTableSortHelper.cpp} (100%) diff --git a/doc/windows/package-manager/winget/list.md b/doc/windows/package-manager/winget/list.md index cffbc3fd21..81a91dc5b5 100644 --- a/doc/windows/package-manager/winget/list.md +++ b/doc/windows/package-manager/winget/list.md @@ -99,7 +99,7 @@ You can set a default sort order in your [settings](https://aka.ms/winget-settin } ``` -An empty array (`[]`) results in default sorting (no sorting applied). +An empty array (`[]`) results in default sorting (sorted by name when listing, relevance preserved when querying). ### Resolution order @@ -107,14 +107,7 @@ When both settings and command-line arguments are present, the following priorit 1. **`--sort` command-line argument** — takes highest priority, overrides settings. 2. **`output.sortOrder` in settings** — used when no `--sort` argument is provided. If the user has configured a sort order in settings, it is applied even when a query is present. -3. **Default** — no sorting (source order preserved). - -### Default relevance preservation - -When using a free-text query (e.g., `winget list Microsoft`) and no sort preference is configured in settings, results are displayed in relevance order as returned by the source. This preserves the most relevant matches at the top by default. If you configure `output.sortOrder` in settings, that preference is respected even for queries. An explicit `--sort` on the command line always overrides both settings and the default relevance ordering. - -> [!NOTE] -> Only the free-text query argument (`-q`/`--query`) has meaningful relevance ranking. Filter arguments like `--id`, `--name`, or `--tag` use exact or substring matching where results have equivalent relevance, so settings sort always applies for those. +3. **Default** — sorted by name in ascending order. When a query is used, relevance ordering is preserved instead. ## List with Update diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 031578eb87..837fb30596 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -485,9 +485,9 @@ namespace AppInstaller::CLI::Workflow } else { - // Invalid values should not reach here; ValidateArgumentsInternal - // rejects them with an error before workflow execution begins. - THROW_HR(E_UNEXPECTED); + // Invalid values should not reach here; ValidateArguments + // rejects them with a CommandException before workflow execution begins. + FAIL_FAST_MSG("Unexpected sort field value reached workflow; validation should have caught this."); } } } @@ -495,30 +495,29 @@ namespace AppInstaller::CLI::Workflow { sortFields = User().Get(); - // When the free-text query argument is present and the user has NOT - // configured a sort preference in settings, preserve relevance ordering. - // Only the positional query argument populates searchRequest.Query and - // produces meaningful relevance ranking; filter arguments like --id, - // --name, --tag etc. use exact/substring matching where all results - // have equivalent relevance. - // If the user explicitly configured output.sortOrder, respect it even - // with queries — that is an explicit user preference. - if (sortFields.empty() && context.Args.Contains(Execution::Args::Type::Query)) - { - return; - } - - // No settings configured and no query — apply default sort by name - // so that output is deterministic and user-friendly. if (sortFields.empty()) { + if (context.Args.Contains(Execution::Args::Type::Query)) + { + // When the free-text query argument is present and the user has NOT + // configured a sort preference in settings, preserve relevance ordering. + // Only the positional query argument populates searchRequest.Query and + // produces meaningful relevance ranking; filter arguments like --id, + // --name, --tag etc. use exact/substring matching where all results + // have equivalent relevance. + // If the user explicitly configured output.sortOrder, respect it even + // with queries — that is an explicit user preference. + return; + } + + // No settings configured and no query — apply default sort by name + // so that output is deterministic and user-friendly. sortFields.emplace_back(SortField::Name); } } - // Empty sort fields or relevance-only means no sorting - if (sortFields.empty() || - (sortFields.size() == 1 && sortFields[0] == SortField::Relevance)) + // Relevance-only means preserve source ordering — no sorting needed. + if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) { return; } @@ -544,9 +543,12 @@ namespace AppInstaller::CLI::Workflow [mask](const InstalledPackagesTableLine& line, size_t index) { return SortablePackageEntry( index, - line.Name.get(), line.Id.get(), - line.InstalledVersion.get(), line.AvailableVersion.get(), - line.Source.get(), mask); + line.Name.get(), + line.Id.get(), + line.InstalledVersion.get(), + line.AvailableVersion.get(), + line.Source.get(), + mask); }, sortFields, direction); } diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj index 71665033fd..95b088d983 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj @@ -302,7 +302,7 @@ - + diff --git a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters index 896e3062ae..d04ebb0d02 100644 --- a/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters +++ b/src/AppInstallerCLITests/AppInstallerCLITests.vcxproj.filters @@ -236,7 +236,7 @@ Source Files - + Source Files\CLI diff --git a/src/AppInstallerCLITests/Command.cpp b/src/AppInstallerCLITests/Command.cpp index 13345d67c6..f80cf66cfc 100644 --- a/src/AppInstallerCLITests/Command.cpp +++ b/src/AppInstallerCLITests/Command.cpp @@ -4,7 +4,9 @@ #include "TestCommon.h" #include #include +#include #include +#include using namespace std::string_literals; using namespace std::string_view_literals; @@ -665,3 +667,55 @@ TEST_CASE("ParseArguments_PositionalWithTooManyValues", "[command]") REQUIRE_COMMAND_EXCEPTION(command.ParseArguments(inv, args), CLI::Resource::String::ExtraPositionalError(Utility::LocIndView{ values.back() })); } + +TEST_CASE("EnsureListSortFieldCountMatchesLimit", "[command]") +{ + // Verifies that the ListCommand --sort argument count limit matches + // the number of valid SortField enum values. If a new SortField is + // added but SetCountLimit is not updated (or vice versa), this fails. + ListCommand listCmd(""); + auto args = listCmd.GetArguments(); + + size_t sortLimit = 0; + for (const auto& arg : args) + { + if (arg.ExecArgType() == Execution::Args::Type::Sort) + { + sortLimit = arg.Limit(); + break; + } + } + REQUIRE(sortLimit > 0); + + // Every valid sort field string must convert successfully. + std::vector allFieldNames = { + "relevance", "name", "id", "version", "source", "available" + }; + + REQUIRE(allFieldNames.size() == sortLimit); + + for (const auto& name : allFieldNames) + { + auto field = Settings::ConvertToSortField(name); + REQUIRE(field.has_value()); + } +} + +TEST_CASE("ValidateArguments_StandardArgExceedsCountLimit", "[command]") +{ + Args args; + TestCommand command({ + Argument{ "sort", 's', Args::Type::Sort, DefaultDesc, ArgumentType::Standard }.SetCountLimit(6), + }); + + // All 6 valid sort fields plus a duplicate — 7 total exceeds the limit of 6. + Invocation inv{ std::vector{ + "--sort", "name", "--sort", "id", "--sort", "version", + "--sort", "source", "--sort", "available", "--sort", "relevance", + "--sort", "name" } }; + + command.ParseArguments(inv, args); + REQUIRE(args.GetCount(Args::Type::Sort) == 7); + + REQUIRE_COMMAND_EXCEPTION(command.ValidateArguments(args), CLI::Resource::String::TooManyArgError(ArgumentCommon::ForType(Args::Type::Sort).Name)); +} \ No newline at end of file diff --git a/src/AppInstallerCLITests/ListSort.cpp b/src/AppInstallerCLITests/PackageTableSortHelper.cpp similarity index 100% rename from src/AppInstallerCLITests/ListSort.cpp rename to src/AppInstallerCLITests/PackageTableSortHelper.cpp From c19ce84a7eef9e99d5cd770ef07ee1cd1c949776 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Wed, 6 May 2026 16:16:01 -0700 Subject: [PATCH 14/16] [ListSort] Address PR review feedback for sort implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `WI_IsAnyFlagSet` was used for single-flag checks where `WI_IsFlagSet` is the correct API - `ComputeSortFieldMask` was defined inline in the header instead of the .cpp file - `SortInstalledPackagesTableLines` was called at three separate call sites instead of being encapsulated inside `OutputInstalledPackages` - Default-sort bypass only checked `Query` argument, missing the `MultiQuery` path - Test assertions validated only a single field (name or ID), providing no cross-field verification of sort correctness - Replace all five `WI_IsAnyFlagSet` calls with `WI_IsFlagSet` for single-flag conditionals - Move `ComputeSortFieldMask` body to PackageTableSortHelper.cpp, leaving only the declaration in the header - Consolidate sort invocation inside `OutputInstalledPackages`, reducing three call sites to one - Add `MultiQuery` argument check alongside `Query` for relevance-preserving default - Replace `GetNames`/`GetIds` test helpers with `ValidateSortResult` that asserts all six precomputed fields (FoldedName, FoldedId, FoldedSource, HasAvailableVersion, ParsedInstalledVersion, ParsedAvailableVersion) per entry - Add release notes entry for sortable list output - No behavioral change to end users — fixes are code quality improvements and correctness for the `--query` multi-value path - Test suite now validates complete sort state rather than a single projection, catching regressions in any field computation Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- doc/ReleaseNotes.md | 4 + .../Workflows/PackageTableSortHelper.cpp | 20 ++- .../Workflows/PackageTableSortHelper.h | 10 +- .../Workflows/WorkflowBase.cpp | 8 +- .../PackageTableSortHelper.cpp | 135 +++++++++++++----- 5 files changed, 120 insertions(+), 57 deletions(-) diff --git a/doc/ReleaseNotes.md b/doc/ReleaseNotes.md index ad5dd4dc9a..0cd5e0c75c 100644 --- a/doc/ReleaseNotes.md +++ b/doc/ReleaseNotes.md @@ -73,6 +73,10 @@ Added a user setting (`logging.fileNameStrategy`) for controlling the default na | guid | The log name is a GUID | | shortguid | The log name is the first 8 characters of a GUID | +### Sortable `list` output + +`winget list` now supports sorting results via `--sort ` (repeatable for multi-field sorting), `--ascending`/`--descending` direction flags, and a persistent `output.sortOrder` setting. Available sort fields: `name`, `id`, `version`, `source`, `available`, `relevance`. By default, results are sorted alphabetically by name when no query is present; use `--sort relevance` to preserve the previous source-determined ordering. + ## Bug Fixes * `winget export` now works when the destination path is a hidden file diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp index 1482d0038d..2f06cf9550 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp @@ -15,23 +15,23 @@ namespace AppInstaller::CLI::Workflow Settings::SortField fieldMask) : OriginalIndex(originalIndex) { - if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Name)) + if (WI_IsFlagSet(fieldMask, Settings::SortField::Name)) { FoldedName = Utility::FoldCase(name); } - if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Id)) + if (WI_IsFlagSet(fieldMask, Settings::SortField::Id)) { FoldedId = Utility::FoldCase(id); } - if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Source)) + if (WI_IsFlagSet(fieldMask, Settings::SortField::Source)) { FoldedSource = Utility::FoldCase(source); } - if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Version)) + if (WI_IsFlagSet(fieldMask, Settings::SortField::Version)) { ParsedInstalledVersion = Utility::Version{ std::string{ installedVersion } }; } - if (WI_IsAnyFlagSet(fieldMask, Settings::SortField::Available)) + if (WI_IsFlagSet(fieldMask, Settings::SortField::Available)) { HasAvailableVersion = !availableVersion.empty(); if (HasAvailableVersion) @@ -41,6 +41,16 @@ namespace AppInstaller::CLI::Workflow } } + Settings::SortField ComputeSortFieldMask(const std::vector& sortFields) + { + Settings::SortField mask = Settings::SortField::Relevance; + for (const auto& f : sortFields) + { + mask |= f; + } + return mask; + } + int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) { using Settings::SortField; diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h index 941f6706bb..8ea2ad11ff 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h @@ -49,15 +49,7 @@ namespace AppInstaller::CLI::Workflow Settings::SortDirection direction); // Computes a bitmask of all sort fields so the constructor can skip unused fields. - inline Settings::SortField ComputeSortFieldMask(const std::vector& sortFields) - { - Settings::SortField mask = Settings::SortField::Relevance; - for (const auto& f : sortFields) - { - mask |= f; - } - return mask; - } + Settings::SortField ComputeSortFieldMask(const std::vector& sortFields); // Sorts a vector of arbitrary items by projecting each into a SortablePackageEntry // via a caller-supplied converter, sorting the projections, then reordering the diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 837fb30596..7cf2f8644d 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -497,7 +497,8 @@ namespace AppInstaller::CLI::Workflow if (sortFields.empty()) { - if (context.Args.Contains(Execution::Args::Type::Query)) + if (context.Args.Contains(Execution::Args::Type::Query) || + context.Args.Contains(Execution::Args::Type::MultiQuery)) { // When the free-text query argument is present and the user has NOT // configured a sort preference in settings, preserve relevance ordering. @@ -555,6 +556,8 @@ namespace AppInstaller::CLI::Workflow void OutputInstalledPackages(Execution::Context& context, std::vector& lines) { + SortInstalledPackagesTableLines(context, lines); + if (context.Args.Contains(Execution::Args::Type::ListDetails)) { OutputInstalledPackagesDetails(context, lines); @@ -1320,7 +1323,6 @@ namespace AppInstaller::CLI::Workflow } } - SortInstalledPackagesTableLines(context, lines); OutputInstalledPackages(context, lines); if (lines.empty()) @@ -1343,14 +1345,12 @@ namespace AppInstaller::CLI::Workflow if (!linesForExplicitUpgrade.empty()) { context.Reporter.Info() << std::endl << Resource::String::UpgradeAvailableForPinned << std::endl; - SortInstalledPackagesTableLines(context, linesForExplicitUpgrade); OutputInstalledPackages(context, linesForExplicitUpgrade); } if (!linesForPins.empty()) { context.Reporter.Info() << std::endl << Resource::String::UpgradeBlockedByPinCount(linesForPins.size()) << std::endl; - SortInstalledPackagesTableLines(context, linesForPins); OutputInstalledPackages(context, linesForPins); } diff --git a/src/AppInstallerCLITests/PackageTableSortHelper.cpp b/src/AppInstallerCLITests/PackageTableSortHelper.cpp index 15c2bf19cc..eda092d92c 100644 --- a/src/AppInstallerCLITests/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLITests/PackageTableSortHelper.cpp @@ -20,19 +20,24 @@ namespace return SortablePackageEntry{ 0, name, id, version, available, source, AllFieldsMask }; } - std::vector GetNames(const std::vector& entries) + // Validates that all precomputed fields in actual match expected, entry by entry. + void ValidateSortResult( + const std::vector& actual, + const std::vector& expected) { - std::vector names; - for (const auto& e : entries) { names.push_back(e.FoldedName); } - return names; + REQUIRE(actual.size() == expected.size()); + for (size_t i = 0; i < actual.size(); ++i) + { + INFO("Entry index: " << i); + REQUIRE(actual[i].FoldedName == expected[i].FoldedName); + REQUIRE(actual[i].FoldedId == expected[i].FoldedId); + REQUIRE(actual[i].FoldedSource == expected[i].FoldedSource); + REQUIRE(actual[i].HasAvailableVersion == expected[i].HasAvailableVersion); + REQUIRE(actual[i].ParsedInstalledVersion == expected[i].ParsedInstalledVersion); + REQUIRE(actual[i].ParsedAvailableVersion == expected[i].ParsedAvailableVersion); + } } - std::vector GetIds(const std::vector& entries) - { - std::vector ids; - for (const auto& e : entries) { ids.push_back(e.FoldedId); } - return ids; - } } TEST_CASE("ListSort_CompareByField_Name", "[listsort]") @@ -136,8 +141,12 @@ TEST_CASE("ListSort_SortEntries_ByName_Ascending", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); - auto names = GetNames(entries); - REQUIRE(names == std::vector{ "alpha", "beta", "charlie" }); + std::vector expected = { + MakeEntry("Alpha", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + MakeEntry("Charlie", "c", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") @@ -150,8 +159,12 @@ TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Descending); - auto names = GetNames(entries); - REQUIRE(names == std::vector{ "charlie", "beta", "alpha" }); + std::vector expected = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Beta", "b", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_ByName_CaseInsensitive", "[listsort]") @@ -164,8 +177,13 @@ TEST_CASE("ListSort_SortEntries_ByName_CaseInsensitive", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); - auto ids = GetIds(entries); - REQUIRE(ids == std::vector{ "a", "b", "c" }); + // Expected uses same casing as input — ValidateSortResult compares folded values + std::vector expected = { + MakeEntry("ALPHA", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + MakeEntry("charlie", "c", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_ById", "[listsort]") @@ -178,23 +196,30 @@ TEST_CASE("ListSort_SortEntries_ById", "[listsort]") SortEntries(entries, { SortField::Id }, SortDirection::Ascending); - auto ids = GetIds(entries); - REQUIRE(ids == std::vector{ "com.alpha", "com.mu", "com.zeta" }); + std::vector expected = { + MakeEntry("A App", "com.alpha", "1.0"), + MakeEntry("M App", "com.mu", "1.0"), + MakeEntry("Z App", "com.zeta", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_ByVersion", "[listsort]") { std::vector entries = { - MakeEntry("App", "app", "10.0.0"), - MakeEntry("App", "app", "2.0.0"), - MakeEntry("App", "app", "1.0.0"), + MakeEntry("App C", "c", "10.0.0"), + MakeEntry("App A", "a", "2.0.0"), + MakeEntry("App B", "b", "1.0.0"), }; SortEntries(entries, { SortField::Version }, SortDirection::Ascending); - // Version is precomputed, verify order via ParsedInstalledVersion - REQUIRE(entries[0].ParsedInstalledVersion < entries[1].ParsedInstalledVersion); - REQUIRE(entries[1].ParsedInstalledVersion < entries[2].ParsedInstalledVersion); + std::vector expected = { + MakeEntry("App B", "b", "1.0.0"), + MakeEntry("App A", "a", "2.0.0"), + MakeEntry("App C", "c", "10.0.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_MultiField", "[listsort]") @@ -208,8 +233,13 @@ TEST_CASE("ListSort_SortEntries_MultiField", "[listsort]") SortEntries(entries, { SortField::Name, SortField::Id }, SortDirection::Ascending); - auto ids = GetIds(entries); - REQUIRE(ids == std::vector{ "a.1", "a.2", "b.1", "b.2" }); + std::vector expected = { + MakeEntry("Alpha", "a.1", "1.0"), + MakeEntry("Alpha", "a.2", "2.0"), + MakeEntry("Beta", "b.1", "1.0"), + MakeEntry("Beta", "b.2", "2.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_Available_GroupsByPresence", "[listsort]") @@ -222,11 +252,12 @@ TEST_CASE("ListSort_SortEntries_Available_GroupsByPresence", "[listsort]") SortEntries(entries, { SortField::Available }, SortDirection::Ascending); - auto names = GetNames(entries); - // Has-update first, then no-update (stable within groups) - REQUIRE(names[0] == "hasupdate"); - REQUIRE(names[1] == "noupdate1"); - REQUIRE(names[2] == "noupdate2"); + std::vector expected = { + MakeEntry("HasUpdate", "h1", "1.0", "2.0"), + MakeEntry("NoUpdate1", "n1", "1.0", ""), + MakeEntry("NoUpdate2", "n2", "1.0", ""), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") @@ -240,8 +271,12 @@ TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") // Relevance means preserve original order SortEntries(entries, { SortField::Relevance }, SortDirection::Ascending); - auto names = GetNames(entries); - REQUIRE(names == std::vector{ "charlie", "alpha", "beta" }); + std::vector expected = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + MakeEntry("Beta", "b", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") @@ -254,8 +289,11 @@ TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") // Empty sort fields means no sorting SortEntries(entries, {}, SortDirection::Ascending); - auto names = GetNames(entries); - REQUIRE(names == std::vector{ "charlie", "alpha" }); + std::vector expected = { + MakeEntry("Charlie", "c", "1.0"), + MakeEntry("Alpha", "a", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") @@ -266,8 +304,10 @@ TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); - REQUIRE(entries.size() == 1); - REQUIRE(entries[0].FoldedName == "only"); + std::vector expected = { + MakeEntry("Only", "only", "1.0"), + }; + ValidateSortResult(entries, expected); } TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") @@ -280,8 +320,11 @@ TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") SortEntries(entries, { SortField::Name }, SortDirection::Ascending); - REQUIRE(entries[0].FoldedId == "first"); - REQUIRE(entries[1].FoldedId == "second"); + std::vector expected = { + MakeEntry("Same", "first", "1.0"), + MakeEntry("Same", "second", "1.0"), + }; + ValidateSortResult(entries, expected); } // Tests for SortBy template — validates the production sort pipeline @@ -302,10 +345,20 @@ TEST_CASE("ListSort_SortBy_ReordersSourceItems", "[listsort]") }, { SortField::Name }, SortDirection::Ascending); + // Verify all fields of each row after sort + REQUIRE(rows.size() == 3); REQUIRE(rows[0].name == "Alpha"); + REQUIRE(rows[0].id == "a"); + REQUIRE(rows[0].ver == "1.0"); REQUIRE(rows[0].extra == 200); REQUIRE(rows[1].name == "Beta"); + REQUIRE(rows[1].id == "b"); + REQUIRE(rows[1].ver == "1.0"); + REQUIRE(rows[1].extra == 300); REQUIRE(rows[2].name == "Charlie"); + REQUIRE(rows[2].id == "c"); + REQUIRE(rows[2].ver == "1.0"); + REQUIRE(rows[2].extra == 100); } TEST_CASE("ListSort_SortBy_PreservesExtraFields", "[listsort]") @@ -323,6 +376,10 @@ TEST_CASE("ListSort_SortBy_PreservesExtraFields", "[listsort]") }, { SortField::Name }, SortDirection::Ascending); + // Verify all fields preserved after sort + REQUIRE(rows.size() == 2); + REQUIRE(rows[0].name == "Alpha"); REQUIRE(rows[0].payload == "payload-a"); + REQUIRE(rows[1].name == "Zeta"); REQUIRE(rows[1].payload == "payload-z"); } From 4065b6460509b182530b72fc2341d2b6946960d4 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Thu, 7 May 2026 16:18:19 -0700 Subject: [PATCH 15/16] [ListSort] Address round 2 PR review feedback ## Problem - Sort argument count limit was hardcoded to a magic number (6), creating drift risk when new SortField values are added - SortField::Relevance occupied 0x0, conflicting with bitmask zero-initialization semantics - Sort resolution logic was inlined in WorkflowBase.cpp, making it untestable in isolation - Available version comparison used a separate bool flag instead of leveraging std::optional semantics - Missing WI_ASSERT guards on default switch cases in CompareByField ## Solution - Replace hardcoded SetCountLimit(6) with dynamic computation via GetAllExponentialEnumValues - Add SortField::None = 0x0 for bitmask initialization, move Relevance to 0x20, add Max = 0x40 sentinel - Extract ResolveSortParameters() into PackageTableSortHelper as a self-contained testable function - Replace HasAvailableVersion bool with std::optional throughout sort pipeline - Add WI_ASSERT(false) to default cases in both GetSortKey and CompareByField - Simplify EnsureListSortFieldCountMatchesLimit test to assert (1u << limit) == Max - Remove redundant ValidateArguments_StandardArgExceedsCountLimit test (covered by framework) - Add using-declaration for Settings namespace to reduce qualification noise ## How Validated - Full solution build (AppInstallerCLICore + AppInstallerCLITests): clean, zero warnings - All 19 sort-related test cases pass (199 assertions) - Spell check: no CI violations - End-to-end wingetdev deployment verified across 12 scenarios including settings, query, multi-sort, order direction, and invalid input edge cases --- .../Commands/ListCommand.cpp | 3 +- .../Workflows/PackageTableSortHelper.cpp | 115 ++++++++++++++---- .../Workflows/PackageTableSortHelper.h | 26 +++- .../Workflows/WorkflowBase.cpp | 73 +++-------- src/AppInstallerCLITests/Command.cpp | 43 +------ .../PackageTableSortHelper.cpp | 7 +- .../Public/winget/UserSettings.h | 4 +- 7 files changed, 147 insertions(+), 124 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/ListCommand.cpp b/src/AppInstallerCLICore/Commands/ListCommand.cpp index 310a8038e0..f46bed4d2d 100644 --- a/src/AppInstallerCLICore/Commands/ListCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ListCommand.cpp @@ -5,6 +5,7 @@ #include "Workflows/CompletionFlow.h" #include "Workflows/WorkflowBase.h" #include "Resources.h" +#include namespace AppInstaller::CLI { @@ -32,7 +33,7 @@ namespace AppInstaller::CLI Argument{ Execution::Args::Type::IncludeUnknown, Resource::String::IncludeUnknownInListArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::IncludePinned, Resource::String::IncludePinnedInListArgumentDescription, ArgumentType::Flag}, Argument::ForType(Execution::Args::Type::ListDetails), - Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(6), + Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(AppInstaller::GetAllExponentialEnumValues(Settings::SortField::None).size()), Argument{ Execution::Args::Type::SortAscending, Resource::String::SortAscendingArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::SortDescending, Resource::String::SortDescendingArgumentDescription, ArgumentType::Flag }, }; diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp index 2f06cf9550..cd97475559 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp @@ -5,6 +5,8 @@ namespace AppInstaller::CLI::Workflow { + using namespace Settings; + SortablePackageEntry::SortablePackageEntry( size_t originalIndex, std::string_view name, @@ -12,38 +14,37 @@ namespace AppInstaller::CLI::Workflow std::string_view installedVersion, std::string_view availableVersion, std::string_view source, - Settings::SortField fieldMask) + SortField fieldMask) : OriginalIndex(originalIndex) { - if (WI_IsFlagSet(fieldMask, Settings::SortField::Name)) + if (WI_IsFlagSet(fieldMask, SortField::Name)) { FoldedName = Utility::FoldCase(name); } - if (WI_IsFlagSet(fieldMask, Settings::SortField::Id)) + if (WI_IsFlagSet(fieldMask, SortField::Id)) { FoldedId = Utility::FoldCase(id); } - if (WI_IsFlagSet(fieldMask, Settings::SortField::Source)) + if (WI_IsFlagSet(fieldMask, SortField::Source)) { FoldedSource = Utility::FoldCase(source); } - if (WI_IsFlagSet(fieldMask, Settings::SortField::Version)) + if (WI_IsFlagSet(fieldMask, SortField::Version)) { ParsedInstalledVersion = Utility::Version{ std::string{ installedVersion } }; } - if (WI_IsFlagSet(fieldMask, Settings::SortField::Available)) + if (WI_IsFlagSet(fieldMask, SortField::Available)) { - HasAvailableVersion = !availableVersion.empty(); - if (HasAvailableVersion) + if (!availableVersion.empty()) { ParsedAvailableVersion = Utility::Version{ std::string{ availableVersion } }; } } } - Settings::SortField ComputeSortFieldMask(const std::vector& sortFields) + SortField ComputeSortFieldMask(const std::vector& sortFields) { - Settings::SortField mask = Settings::SortField::Relevance; + SortField mask = SortField::None; for (const auto& f : sortFields) { mask |= f; @@ -51,10 +52,74 @@ namespace AppInstaller::CLI::Workflow return mask; } - int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field) + SortParameters ResolveSortParameters( + const std::vector& explicitSortArgs, + bool hasQuery, + bool hasExplicitAscending, + bool hasExplicitDescending) { - using Settings::SortField; + SortParameters result; + std::vector sortFields; + + if (!explicitSortArgs.empty()) + { + for (const auto& arg : explicitSortArgs) + { + auto field = ConvertToSortField(arg); + WI_ASSERT(field.has_value()); + if (field.has_value()) + { + sortFields.emplace_back(field.value()); + } + } + } + else + { + sortFields = User().Get(); + + if (sortFields.empty()) + { + if (hasQuery) + { + // Preserve relevance ordering when a free-text query is present + // and no explicit sort preference is configured. + return result; // ShouldSort = false + } + + // No settings, no query — default to name sort. + sortFields.emplace_back(SortField::Name); + } + } + + // Relevance-only means preserve source ordering. + if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) + { + return result; // ShouldSort = false + } + // Resolve direction + SortDirection direction = SortDirection::Ascending; + if (hasExplicitDescending) + { + direction = SortDirection::Descending; + } + else if (hasExplicitAscending) + { + direction = SortDirection::Ascending; + } + else + { + direction = User().Get(); + } + + result.ShouldSort = true; + result.Fields = std::move(sortFields); + result.Direction = direction; + return result; + } + + int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, SortField field) + { switch (field) { case SortField::Name: @@ -71,27 +136,33 @@ namespace AppInstaller::CLI::Workflow return a.FoldedSource.compare(b.FoldedSource); case SortField::Available: { - if (a.HasAvailableVersion != b.HasAvailableVersion) + // std::optional comparison: nullopt < any value. + // We want has-version to sort before no-version in ascending order, + // so reverse the comparison operands. + if (a.ParsedAvailableVersion.has_value() != b.ParsedAvailableVersion.has_value()) { - // Ascending: has-update sorts before no-update - return a.HasAvailableVersion ? -1 : 1; + return a.ParsedAvailableVersion.has_value() ? -1 : 1; } - if (a.HasAvailableVersion && b.HasAvailableVersion) + if (a.ParsedAvailableVersion.has_value() && b.ParsedAvailableVersion.has_value()) { - if (a.ParsedAvailableVersion < b.ParsedAvailableVersion) return -1; - if (b.ParsedAvailableVersion < a.ParsedAvailableVersion) return 1; + if (a.ParsedAvailableVersion.value() < b.ParsedAvailableVersion.value()) return -1; + if (b.ParsedAvailableVersion.value() < a.ParsedAvailableVersion.value()) return 1; } return 0; } + case SortField::Relevance: + // Relevance has no precomputed sort key — preserve original ordering. + return 0; default: + WI_ASSERT(false); return 0; } } void SortEntries( std::vector& entries, - const std::vector& sortFields, - Settings::SortDirection direction) + const std::vector& sortFields, + SortDirection direction) { if (entries.size() <= 1 || sortFields.empty()) { @@ -99,7 +170,7 @@ namespace AppInstaller::CLI::Workflow } // Relevance-only means no sorting - if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) + if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) { return; } @@ -112,7 +183,7 @@ namespace AppInstaller::CLI::Workflow int cmp = CompareByField(a, b, field); if (cmp != 0) { - return direction == Settings::SortDirection::Ascending ? (cmp < 0) : (cmp > 0); + return direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); } } return false; diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h index 8ea2ad11ff..048d7f310f 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h @@ -6,6 +6,7 @@ #include #include +#include #include namespace AppInstaller::CLI::Workflow @@ -23,8 +24,7 @@ namespace AppInstaller::CLI::Workflow // Precomputed parsed versions Utility::Version ParsedInstalledVersion; - Utility::Version ParsedAvailableVersion; - bool HasAvailableVersion = false; + std::optional ParsedAvailableVersion; SortablePackageEntry() = default; @@ -51,6 +51,28 @@ namespace AppInstaller::CLI::Workflow // Computes a bitmask of all sort fields so the constructor can skip unused fields. Settings::SortField ComputeSortFieldMask(const std::vector& sortFields); + // Result of sort parameter resolution. If ShouldSort is false, the caller should + // preserve the current ordering (relevance or no-op). + struct SortParameters + { + bool ShouldSort = false; + std::vector Fields; + Settings::SortDirection Direction = Settings::SortDirection::Ascending; + }; + + // Resolves the effective sort parameters from explicit CLI args, user settings, + // and query context. Resolution order: explicit args > settings > query-aware default. + // Parameters: + // explicitSortArgs - raw string values from --sort (empty if not provided) + // hasQuery - whether a free-text query argument is present + // hasExplicitAscending - whether --ascending was passed + // hasExplicitDescending - whether --descending was passed + SortParameters ResolveSortParameters( + const std::vector& explicitSortArgs, + bool hasQuery, + bool hasExplicitAscending, + bool hasExplicitDescending); + // Sorts a vector of arbitrary items by projecting each into a SortablePackageEntry // via a caller-supplied converter, sorting the projections, then reordering the // source vector to match. The converter signature is: diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 7cf2f8644d..7f903f5d97 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -470,76 +470,31 @@ namespace AppInstaller::CLI::Workflow return; } - // 1. Determine sort fields: CLI --sort overrides everything - std::vector sortFields; - bool hasExplicitSort = context.Args.Contains(Execution::Args::Type::Sort); - - if (hasExplicitSort) + // Collect explicit --sort arg values (if any) + std::vector explicitSortArgs; + if (context.Args.Contains(Execution::Args::Type::Sort)) { for (const auto& arg : *context.Args.GetArgs(Execution::Args::Type::Sort)) { - auto field = ConvertToSortField(arg); - if (field) - { - sortFields.emplace_back(field.value()); - } - else - { - // Invalid values should not reach here; ValidateArguments - // rejects them with a CommandException before workflow execution begins. - FAIL_FAST_MSG("Unexpected sort field value reached workflow; validation should have caught this."); - } + explicitSortArgs.emplace_back(arg); } } - else - { - sortFields = User().Get(); - if (sortFields.empty()) - { - if (context.Args.Contains(Execution::Args::Type::Query) || - context.Args.Contains(Execution::Args::Type::MultiQuery)) - { - // When the free-text query argument is present and the user has NOT - // configured a sort preference in settings, preserve relevance ordering. - // Only the positional query argument populates searchRequest.Query and - // produces meaningful relevance ranking; filter arguments like --id, - // --name, --tag etc. use exact/substring matching where all results - // have equivalent relevance. - // If the user explicitly configured output.sortOrder, respect it even - // with queries — that is an explicit user preference. - return; - } + bool hasQuery = context.Args.Contains(Execution::Args::Type::Query) || + context.Args.Contains(Execution::Args::Type::MultiQuery); - // No settings configured and no query — apply default sort by name - // so that output is deterministic and user-friendly. - sortFields.emplace_back(SortField::Name); - } - } + auto params = ResolveSortParameters( + explicitSortArgs, + hasQuery, + context.Args.Contains(Execution::Args::Type::SortAscending), + context.Args.Contains(Execution::Args::Type::SortDescending)); - // Relevance-only means preserve source ordering — no sorting needed. - if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) + if (!params.ShouldSort) { return; } - // 2. Determine direction: CLI flags override settings - SortDirection direction = SortDirection::Ascending; - if (context.Args.Contains(Execution::Args::Type::SortDescending)) - { - direction = SortDirection::Descending; - } - else if (context.Args.Contains(Execution::Args::Type::SortAscending)) - { - direction = SortDirection::Ascending; - } - else - { - direction = User().Get(); - } - - // 3. Sort using the helper's production pipeline - const SortField mask = ComputeSortFieldMask(sortFields); + const SortField mask = ComputeSortFieldMask(params.Fields); SortBy(lines, [mask](const InstalledPackagesTableLine& line, size_t index) { return SortablePackageEntry( @@ -551,7 +506,7 @@ namespace AppInstaller::CLI::Workflow line.Source.get(), mask); }, - sortFields, direction); + params.Fields, params.Direction); } void OutputInstalledPackages(Execution::Context& context, std::vector& lines) diff --git a/src/AppInstallerCLITests/Command.cpp b/src/AppInstallerCLITests/Command.cpp index f80cf66cfc..943981889e 100644 --- a/src/AppInstallerCLITests/Command.cpp +++ b/src/AppInstallerCLITests/Command.cpp @@ -670,52 +670,21 @@ TEST_CASE("ParseArguments_PositionalWithTooManyValues", "[command]") TEST_CASE("EnsureListSortFieldCountMatchesLimit", "[command]") { - // Verifies that the ListCommand --sort argument count limit matches - // the number of valid SortField enum values. If a new SortField is - // added but SetCountLimit is not updated (or vice versa), this fails. - ListCommand listCmd(""); - auto args = listCmd.GetArguments(); + ListCommand listCommand({}); + const auto& args = listCommand.GetArguments(); size_t sortLimit = 0; for (const auto& arg : args) { - if (arg.ExecArgType() == Execution::Args::Type::Sort) + if (arg.ExecArgType() == Args::Type::Sort) { sortLimit = arg.Limit(); break; } } - REQUIRE(sortLimit > 0); - // Every valid sort field string must convert successfully. - std::vector allFieldNames = { - "relevance", "name", "id", "version", "source", "available" - }; - - REQUIRE(allFieldNames.size() == sortLimit); - - for (const auto& name : allFieldNames) - { - auto field = Settings::ConvertToSortField(name); - REQUIRE(field.has_value()); - } + // The product's configured limit must match Max: adding a new field + // without bumping Max (or changing limit) will fail this check. + REQUIRE((1u << sortLimit) == static_cast(Settings::SortField::Max)); } -TEST_CASE("ValidateArguments_StandardArgExceedsCountLimit", "[command]") -{ - Args args; - TestCommand command({ - Argument{ "sort", 's', Args::Type::Sort, DefaultDesc, ArgumentType::Standard }.SetCountLimit(6), - }); - - // All 6 valid sort fields plus a duplicate — 7 total exceeds the limit of 6. - Invocation inv{ std::vector{ - "--sort", "name", "--sort", "id", "--sort", "version", - "--sort", "source", "--sort", "available", "--sort", "relevance", - "--sort", "name" } }; - - command.ParseArguments(inv, args); - REQUIRE(args.GetCount(Args::Type::Sort) == 7); - - REQUIRE_COMMAND_EXCEPTION(command.ValidateArguments(args), CLI::Resource::String::TooManyArgError(ArgumentCommon::ForType(Args::Type::Sort).Name)); -} \ No newline at end of file diff --git a/src/AppInstallerCLITests/PackageTableSortHelper.cpp b/src/AppInstallerCLITests/PackageTableSortHelper.cpp index eda092d92c..b9ea478303 100644 --- a/src/AppInstallerCLITests/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLITests/PackageTableSortHelper.cpp @@ -32,9 +32,12 @@ namespace REQUIRE(actual[i].FoldedName == expected[i].FoldedName); REQUIRE(actual[i].FoldedId == expected[i].FoldedId); REQUIRE(actual[i].FoldedSource == expected[i].FoldedSource); - REQUIRE(actual[i].HasAvailableVersion == expected[i].HasAvailableVersion); + REQUIRE(actual[i].ParsedAvailableVersion.has_value() == expected[i].ParsedAvailableVersion.has_value()); REQUIRE(actual[i].ParsedInstalledVersion == expected[i].ParsedInstalledVersion); - REQUIRE(actual[i].ParsedAvailableVersion == expected[i].ParsedAvailableVersion); + if (actual[i].ParsedAvailableVersion.has_value() && expected[i].ParsedAvailableVersion.has_value()) + { + REQUIRE(actual[i].ParsedAvailableVersion.value() == expected[i].ParsedAvailableVersion.value()); + } } } diff --git a/src/AppInstallerCommonCore/Public/winget/UserSettings.h b/src/AppInstallerCommonCore/Public/winget/UserSettings.h index 9b378b3347..9909d9acd3 100644 --- a/src/AppInstallerCommonCore/Public/winget/UserSettings.h +++ b/src/AppInstallerCommonCore/Public/winget/UserSettings.h @@ -51,12 +51,14 @@ namespace AppInstaller::Settings // via ComputeSortFieldMask, so the constructor can skip unused field computation. enum class SortField : uint32_t { - Relevance = 0x0, // Preserves current natural order (source-defined relevance ranking) + None = 0x0, // Zero value for bitmask initialization; not a user-facing sort field Name = 0x1, Id = 0x2, Version = 0x4, Source = 0x8, Available = 0x10, + Relevance = 0x20, // Preserves current natural order (source-defined relevance ranking) + Max = 0x40, // Sentinel for iteration via GetAllExponentialEnumValues; not a valid sort field }; DEFINE_ENUM_FLAG_OPERATORS(SortField); From dc2542d0deeb4fae0897d5fc85c70db72f498a48 Mon Sep 17 00:00:00 2001 From: Madhusudhan Gumbalapura Sudarshan Date: Fri, 8 May 2026 14:36:22 -0700 Subject: [PATCH 16/16] [ListSort, Refactor] Consolidate sort parameter resolution into SortParameters constructor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Addresses remaining PR review feedback. The primary change moves all CLI arg and settings extraction out of WorkflowBase.cpp and the free function ResolveSortParameters into a SortParameters constructor that reads directly from the execution context, reducing the caller to a single line. ## Changes - **SortParameters constructor**: Replace the free function `ResolveSortParameters(args, hasQuery, hasAscending, hasDescending)` with `SortParameters(const Execution::Context&)` that reads CLI args, settings, and query presence directly from the context. Default constructor preserved for unit test use. - **SortEntries/SortBy signatures**: Change from `(sortFields, direction)` to `(const SortParameters& sortParams)`, consolidating three separate no-op guards (empty fields, relevance-only, size <= 1) into `!sortParams.ShouldSort`. - **Direction resolution**: Collapse 12-line if/else/else chain into a ternary expression. - **Available field comparison**: Extract `aHas`/`bHas` locals for readability in the optional presence check. - **Test helper**: Add `MakeSortParams` that mirrors the production constructor's ShouldSort logic, replacing manual struct construction across all 11 SortEntries and 2 SortBy call sites. - **Test validation**: Simplify `ValidateSortResult` by using `optional::operator==` instead of manual `has_value()` + `value()` comparison. - **Compile-time count**: Add `GetExponentialEnumValuesCount` constexpr template and use `static constexpr` local in `GetArguments()` for the `--sort` argument count limit, replacing a runtime `GetAllExponentialEnumValues().size()` call. - **WorkflowBase caller**: Reduce from 15 lines of arg extraction to `const SortParameters params(context)` with an early-exit comment explaining the optimization. ## Impact - No behavioral changes — all sort semantics preserved. - WorkflowBase.cpp caller reduced from ~20 lines to ~5 lines. - Forward declaration of `Execution::Context` in the header avoids circular includes. --- .../Commands/ListCommand.cpp | 6 +- .../Workflows/PackageTableSortHelper.cpp | 86 +++++++------------ .../Workflows/PackageTableSortHelper.h | 45 ++++------ .../Workflows/WorkflowBase.cpp | 23 +---- .../PackageTableSortHelper.cpp | 46 ++++++---- .../Public/AppInstallerLanguageUtilities.h | 14 +++ 6 files changed, 100 insertions(+), 120 deletions(-) diff --git a/src/AppInstallerCLICore/Commands/ListCommand.cpp b/src/AppInstallerCLICore/Commands/ListCommand.cpp index f46bed4d2d..a9a8d49165 100644 --- a/src/AppInstallerCLICore/Commands/ListCommand.cpp +++ b/src/AppInstallerCLICore/Commands/ListCommand.cpp @@ -5,7 +5,6 @@ #include "Workflows/CompletionFlow.h" #include "Workflows/WorkflowBase.h" #include "Resources.h" -#include namespace AppInstaller::CLI { @@ -14,6 +13,9 @@ namespace AppInstaller::CLI std::vector ListCommand::GetArguments() const { + // Compile-time count of sort field values, used to cap --sort argument count. + static constexpr size_t s_sortFieldCount = AppInstaller::GetExponentialEnumValuesCount(Settings::SortField::None); + return { Argument::ForType(Execution::Args::Type::Query), Argument::ForType(Execution::Args::Type::Id), @@ -33,7 +35,7 @@ namespace AppInstaller::CLI Argument{ Execution::Args::Type::IncludeUnknown, Resource::String::IncludeUnknownInListArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::IncludePinned, Resource::String::IncludePinnedInListArgumentDescription, ArgumentType::Flag}, Argument::ForType(Execution::Args::Type::ListDetails), - Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(AppInstaller::GetAllExponentialEnumValues(Settings::SortField::None).size()), + Argument{ Execution::Args::Type::Sort, Resource::String::SortArgumentDescription, ArgumentType::Standard }.SetCountLimit(s_sortFieldCount), Argument{ Execution::Args::Type::SortAscending, Resource::String::SortAscendingArgumentDescription, ArgumentType::Flag }, Argument{ Execution::Args::Type::SortDescending, Resource::String::SortDescendingArgumentDescription, ArgumentType::Flag }, }; diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp index cd97475559..3f6b59f4d8 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.cpp @@ -2,6 +2,7 @@ // Licensed under the MIT License. #include "pch.h" #include "PackageTableSortHelper.h" +#include "ExecutionContext.h" namespace AppInstaller::CLI::Workflow { @@ -52,70 +53,53 @@ namespace AppInstaller::CLI::Workflow return mask; } - SortParameters ResolveSortParameters( - const std::vector& explicitSortArgs, - bool hasQuery, - bool hasExplicitAscending, - bool hasExplicitDescending) + SortParameters::SortParameters(const Execution::Context& context) { - SortParameters result; - std::vector sortFields; - - if (!explicitSortArgs.empty()) + if (context.Args.Contains(Execution::Args::Type::Sort)) { - for (const auto& arg : explicitSortArgs) + for (const auto& arg : *context.Args.GetArgs(Execution::Args::Type::Sort)) { auto field = ConvertToSortField(arg); WI_ASSERT(field.has_value()); if (field.has_value()) { - sortFields.emplace_back(field.value()); + Fields.emplace_back(field.value()); } } } else { - sortFields = User().Get(); + Fields = User().Get(); - if (sortFields.empty()) + if (Fields.empty()) { + bool hasQuery = context.Args.Contains(Execution::Args::Type::Query) || + context.Args.Contains(Execution::Args::Type::MultiQuery); + if (hasQuery) { // Preserve relevance ordering when a free-text query is present // and no explicit sort preference is configured. - return result; // ShouldSort = false + return; // ShouldSort remains false } // No settings, no query — default to name sort. - sortFields.emplace_back(SortField::Name); + Fields.emplace_back(SortField::Name); } } // Relevance-only means preserve source ordering. - if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) + if (Fields.size() == 1 && Fields[0] == SortField::Relevance) { - return result; // ShouldSort = false + return; // ShouldSort remains false } - // Resolve direction - SortDirection direction = SortDirection::Ascending; - if (hasExplicitDescending) - { - direction = SortDirection::Descending; - } - else if (hasExplicitAscending) - { - direction = SortDirection::Ascending; - } - else - { - direction = User().Get(); - } + // Resolve direction: CLI flags override settings + Direction = context.Args.Contains(Execution::Args::Type::SortDescending) ? SortDirection::Descending : + context.Args.Contains(Execution::Args::Type::SortAscending) ? SortDirection::Ascending : + User().Get(); - result.ShouldSort = true; - result.Fields = std::move(sortFields); - result.Direction = direction; - return result; + ShouldSort = true; } int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, SortField field) @@ -136,14 +120,17 @@ namespace AppInstaller::CLI::Workflow return a.FoldedSource.compare(b.FoldedSource); case SortField::Available: { - // std::optional comparison: nullopt < any value. - // We want has-version to sort before no-version in ascending order, - // so reverse the comparison operands. - if (a.ParsedAvailableVersion.has_value() != b.ParsedAvailableVersion.has_value()) + bool aHas = a.ParsedAvailableVersion.has_value(); + bool bHas = b.ParsedAvailableVersion.has_value(); + + // Has-version sorts before no-version in ascending order. + if (aHas != bHas) { - return a.ParsedAvailableVersion.has_value() ? -1 : 1; + return aHas ? -1 : 1; } - if (a.ParsedAvailableVersion.has_value() && b.ParsedAvailableVersion.has_value()) + + // Both have versions — compare normally. + if (aHas && bHas) { if (a.ParsedAvailableVersion.value() < b.ParsedAvailableVersion.value()) return -1; if (b.ParsedAvailableVersion.value() < a.ParsedAvailableVersion.value()) return 1; @@ -161,29 +148,22 @@ namespace AppInstaller::CLI::Workflow void SortEntries( std::vector& entries, - const std::vector& sortFields, - SortDirection direction) + const SortParameters& sortParams) { - if (entries.size() <= 1 || sortFields.empty()) - { - return; - } - - // Relevance-only means no sorting - if (sortFields.size() == 1 && sortFields[0] == SortField::Relevance) + if (entries.size() <= 1 || !sortParams.ShouldSort) { return; } std::stable_sort(entries.begin(), entries.end(), - [&sortFields, direction](const SortablePackageEntry& a, const SortablePackageEntry& b) + [&sortParams](const SortablePackageEntry& a, const SortablePackageEntry& b) { - for (const auto& field : sortFields) + for (const auto& field : sortParams.Fields) { int cmp = CompareByField(a, b, field); if (cmp != 0) { - return direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); + return sortParams.Direction == SortDirection::Ascending ? (cmp < 0) : (cmp > 0); } } return false; diff --git a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h index 048d7f310f..018dc55c15 100644 --- a/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h +++ b/src/AppInstallerCLICore/Workflows/PackageTableSortHelper.h @@ -9,6 +9,8 @@ #include #include +namespace AppInstaller::CLI::Execution { struct Context; } + namespace AppInstaller::CLI::Workflow { // Lightweight sortable representation of a package row with precomputed sort keys. @@ -42,12 +44,6 @@ namespace AppInstaller::CLI::Workflow // Returns negative if a < b, positive if a > b, 0 if equal. int CompareByField(const SortablePackageEntry& a, const SortablePackageEntry& b, Settings::SortField field); - // Sorts a vector of sortable entries by the given fields and direction. - void SortEntries( - std::vector& entries, - const std::vector& sortFields, - Settings::SortDirection direction); - // Computes a bitmask of all sort fields so the constructor can skip unused fields. Settings::SortField ComputeSortFieldMask(const std::vector& sortFields); @@ -58,20 +54,21 @@ namespace AppInstaller::CLI::Workflow bool ShouldSort = false; std::vector Fields; Settings::SortDirection Direction = Settings::SortDirection::Ascending; + + // Default constructor leaves ShouldSort=false. Used by unit tests to exercise + // sort algorithm logic independently from CLI/settings parameter resolution. + SortParameters() = default; + + // Resolves the effective sort parameters by reading CLI args, user settings, + // and query context directly from the execution context. + // Resolution order: explicit --sort args > settings > query-aware default. + explicit SortParameters(const Execution::Context& context); }; - // Resolves the effective sort parameters from explicit CLI args, user settings, - // and query context. Resolution order: explicit args > settings > query-aware default. - // Parameters: - // explicitSortArgs - raw string values from --sort (empty if not provided) - // hasQuery - whether a free-text query argument is present - // hasExplicitAscending - whether --ascending was passed - // hasExplicitDescending - whether --descending was passed - SortParameters ResolveSortParameters( - const std::vector& explicitSortArgs, - bool hasQuery, - bool hasExplicitAscending, - bool hasExplicitDescending); + // Sorts a vector of sortable entries using the resolved sort parameters. + void SortEntries( + std::vector& entries, + const SortParameters& sortParams); // Sorts a vector of arbitrary items by projecting each into a SortablePackageEntry // via a caller-supplied converter, sorting the projections, then reordering the @@ -84,15 +81,9 @@ namespace AppInstaller::CLI::Workflow void SortBy( std::vector& items, Converter&& converter, - const std::vector& sortFields, - Settings::SortDirection direction) + const SortParameters& sortParams) { - if (items.size() <= 1 || sortFields.empty()) - { - return; - } - - if (sortFields.size() == 1 && sortFields[0] == Settings::SortField::Relevance) + if (items.size() <= 1 || !sortParams.ShouldSort) { return; } @@ -104,7 +95,7 @@ namespace AppInstaller::CLI::Workflow entries.push_back(converter(items[i], i)); } - SortEntries(entries, sortFields, direction); + SortEntries(entries, sortParams); std::vector sorted; sorted.reserve(items.size()); diff --git a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp index 7f903f5d97..dc60a6da58 100644 --- a/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp +++ b/src/AppInstallerCLICore/Workflows/WorkflowBase.cpp @@ -470,25 +470,10 @@ namespace AppInstaller::CLI::Workflow return; } - // Collect explicit --sort arg values (if any) - std::vector explicitSortArgs; - if (context.Args.Contains(Execution::Args::Type::Sort)) - { - for (const auto& arg : *context.Args.GetArgs(Execution::Args::Type::Sort)) - { - explicitSortArgs.emplace_back(arg); - } - } - - bool hasQuery = context.Args.Contains(Execution::Args::Type::Query) || - context.Args.Contains(Execution::Args::Type::MultiQuery); - - auto params = ResolveSortParameters( - explicitSortArgs, - hasQuery, - context.Args.Contains(Execution::Args::Type::SortAscending), - context.Args.Contains(Execution::Args::Type::SortDescending)); + const SortParameters params(context); + // Not strictly required — SortBy handles this internally — but avoids + // constructing the SortablePackageEntry vector when no sorting is needed. if (!params.ShouldSort) { return; @@ -506,7 +491,7 @@ namespace AppInstaller::CLI::Workflow line.Source.get(), mask); }, - params.Fields, params.Direction); + params); } void OutputInstalledPackages(Execution::Context& context, std::vector& lines) diff --git a/src/AppInstallerCLITests/PackageTableSortHelper.cpp b/src/AppInstallerCLITests/PackageTableSortHelper.cpp index b9ea478303..3497804ddd 100644 --- a/src/AppInstallerCLITests/PackageTableSortHelper.cpp +++ b/src/AppInstallerCLITests/PackageTableSortHelper.cpp @@ -32,15 +32,23 @@ namespace REQUIRE(actual[i].FoldedName == expected[i].FoldedName); REQUIRE(actual[i].FoldedId == expected[i].FoldedId); REQUIRE(actual[i].FoldedSource == expected[i].FoldedSource); - REQUIRE(actual[i].ParsedAvailableVersion.has_value() == expected[i].ParsedAvailableVersion.has_value()); REQUIRE(actual[i].ParsedInstalledVersion == expected[i].ParsedInstalledVersion); - if (actual[i].ParsedAvailableVersion.has_value() && expected[i].ParsedAvailableVersion.has_value()) - { - REQUIRE(actual[i].ParsedAvailableVersion.value() == expected[i].ParsedAvailableVersion.value()); - } + REQUIRE(actual[i].ParsedAvailableVersion == expected[i].ParsedAvailableVersion); } } + // Creates a SortParameters matching production constructor logic: + // ShouldSort is true only when fields contain non-relevance values. + SortParameters MakeSortParams(std::vector fields, SortDirection direction = SortDirection::Ascending) + { + SortParameters params; + params.Fields = std::move(fields); + params.Direction = direction; + params.ShouldSort = !params.Fields.empty() && + !(params.Fields.size() == 1 && params.Fields[0] == SortField::Relevance); + return params; + } + } TEST_CASE("ListSort_CompareByField_Name", "[listsort]") @@ -142,7 +150,7 @@ TEST_CASE("ListSort_SortEntries_ByName_Ascending", "[listsort]") MakeEntry("Beta", "b", "1.0"), }; - SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Name })); std::vector expected = { MakeEntry("Alpha", "a", "1.0"), @@ -160,7 +168,7 @@ TEST_CASE("ListSort_SortEntries_ByName_Descending", "[listsort]") MakeEntry("Beta", "b", "1.0"), }; - SortEntries(entries, { SortField::Name }, SortDirection::Descending); + SortEntries(entries, MakeSortParams({ SortField::Name }, SortDirection::Descending)); std::vector expected = { MakeEntry("Charlie", "c", "1.0"), @@ -178,7 +186,7 @@ TEST_CASE("ListSort_SortEntries_ByName_CaseInsensitive", "[listsort]") MakeEntry("Beta", "b", "1.0"), }; - SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Name })); // Expected uses same casing as input — ValidateSortResult compares folded values std::vector expected = { @@ -197,7 +205,7 @@ TEST_CASE("ListSort_SortEntries_ById", "[listsort]") MakeEntry("M App", "com.mu", "1.0"), }; - SortEntries(entries, { SortField::Id }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Id })); std::vector expected = { MakeEntry("A App", "com.alpha", "1.0"), @@ -215,7 +223,7 @@ TEST_CASE("ListSort_SortEntries_ByVersion", "[listsort]") MakeEntry("App B", "b", "1.0.0"), }; - SortEntries(entries, { SortField::Version }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Version })); std::vector expected = { MakeEntry("App B", "b", "1.0.0"), @@ -234,7 +242,7 @@ TEST_CASE("ListSort_SortEntries_MultiField", "[listsort]") MakeEntry("Alpha", "a.2", "2.0"), }; - SortEntries(entries, { SortField::Name, SortField::Id }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Name, SortField::Id })); std::vector expected = { MakeEntry("Alpha", "a.1", "1.0"), @@ -253,7 +261,7 @@ TEST_CASE("ListSort_SortEntries_Available_GroupsByPresence", "[listsort]") MakeEntry("NoUpdate2", "n2", "1.0", ""), }; - SortEntries(entries, { SortField::Available }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Available })); std::vector expected = { MakeEntry("HasUpdate", "h1", "1.0", "2.0"), @@ -271,8 +279,8 @@ TEST_CASE("ListSort_SortEntries_Relevance_NoOp", "[listsort]") MakeEntry("Beta", "b", "1.0"), }; - // Relevance means preserve original order - SortEntries(entries, { SortField::Relevance }, SortDirection::Ascending); + // Relevance means preserve original order — exercises CompareByField(Relevance) returning 0. + SortEntries(entries, MakeSortParams({ SortField::Relevance })); std::vector expected = { MakeEntry("Charlie", "c", "1.0"), @@ -290,7 +298,7 @@ TEST_CASE("ListSort_SortEntries_EmptyFields_NoOp", "[listsort]") }; // Empty sort fields means no sorting - SortEntries(entries, {}, SortDirection::Ascending); + SortEntries(entries, SortParameters{}); std::vector expected = { MakeEntry("Charlie", "c", "1.0"), @@ -305,7 +313,7 @@ TEST_CASE("ListSort_SortEntries_SingleElement", "[listsort]") MakeEntry("Only", "only", "1.0"), }; - SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Name })); std::vector expected = { MakeEntry("Only", "only", "1.0"), @@ -321,7 +329,7 @@ TEST_CASE("ListSort_SortEntries_StableSort", "[listsort]") MakeEntry("Same", "second", "1.0"), }; - SortEntries(entries, { SortField::Name }, SortDirection::Ascending); + SortEntries(entries, MakeSortParams({ SortField::Name })); std::vector expected = { MakeEntry("Same", "first", "1.0"), @@ -346,7 +354,7 @@ TEST_CASE("ListSort_SortBy_ReordersSourceItems", "[listsort]") [](const Row& r, size_t i) { return SortablePackageEntry(i, r.name, r.id, r.ver, "", "", AllFieldsMask); }, - { SortField::Name }, SortDirection::Ascending); + MakeSortParams({ SortField::Name })); // Verify all fields of each row after sort REQUIRE(rows.size() == 3); @@ -377,7 +385,7 @@ TEST_CASE("ListSort_SortBy_PreservesExtraFields", "[listsort]") [](const Row& r, size_t i) { return SortablePackageEntry(i, r.name, "", "", "", "", AllFieldsMask); }, - { SortField::Name }, SortDirection::Ascending); + MakeSortParams({ SortField::Name })); // Verify all fields preserved after sort REQUIRE(rows.size() == 2); diff --git a/src/AppInstallerSharedLib/Public/AppInstallerLanguageUtilities.h b/src/AppInstallerSharedLib/Public/AppInstallerLanguageUtilities.h index 187f76aaa3..7338886a17 100644 --- a/src/AppInstallerSharedLib/Public/AppInstallerLanguageUtilities.h +++ b/src/AppInstallerSharedLib/Public/AppInstallerLanguageUtilities.h @@ -211,6 +211,20 @@ namespace AppInstaller return result; } + + // Returns the number of flag-bit enum values between initialToSkip and Max, + // computed at compile time. + template + constexpr size_t GetExponentialEnumValuesCount(E initialToSkip) + { + using underlying_t = std::underlying_type_t; + size_t count = 0; + for (underlying_t i = 1 + static_cast(initialToSkip); i < static_cast(E::Max); i <<= 1) + { + ++count; + } + return count; + } } // Enable enums to be output generically (as their integral value).