Add interactive selection for ambiguous package matches - #6575
AmirMS (AmelBawa-msft) wants to merge 3 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
JohnMcPMS
left a comment
JohnMcPMS
left a comment
There was a problem hiding this comment.
It feels like the code should be structured as such:
- WorkflowBase :: Does the top-level decision on flow support for selection, generates table and decides on strings.
- PromptFlow :: Takes in table+strings and determines if prompting is allowed/possible. This is a generic selection flow task. Handles output of the values, input validation and repitition.
- Reporter :: Handles improved input with support for cancellation. Performs no output.
Yes, this also means refactoring the table output into non-templated functions. That is probably for the best anyway.
|
|
||
| ## UI/UX Design | ||
|
|
||
| Use the existing search table layout with a leading selection number. Show the Source column only when candidates span sources. For example: |
There was a problem hiding this comment.
Disagree; we should show the source regardless. Just because they all come from FOO doesn't mean I as the user know they come from FOO.
| Workflow::SearchSourceForSingle << | ||
| Workflow::HandleSearchResultFailures << | ||
| Workflow::EnsureOneMatchFromSearchResult(OperationType::Download) << | ||
| Workflow::EnsureOneMatchFromSearchResult(OperationType::Download, true) << |
There was a problem hiding this comment.
I would greatly prefer this to be an enum so we know what we are asking for instead of just true. And to be clear and complete, I mean replace the use of bool for this semantic globally with something like enum class PackageSelectionBehavior.
| namespace AppInstaller::CLI::Workflow | ||
| { | ||
| namespace | ||
| bool IsInteractivityAllowed(Execution::Context& context) |
There was a problem hiding this comment.
This was intentionally hidden here so that all prompt flows would be in this path. I would consider this feature to be introducing a prompt. Why is it not in this file?
| context << | ||
| SearchSourceForSingle << | ||
| SelectSinglePackageVersionForInstallOrUpgrade(m_operationType) << | ||
| SelectSinglePackageVersionForInstallOrUpgrade(m_operationType, false, m_allowSelection) << |
There was a problem hiding this comment.
Put new non-optional parameters before optional parameters. You are supplying it here unconditionally, so it isn't really optional.
| return m_consoleStreams && Info().IsEnabled(); | ||
| } | ||
|
|
||
| std::optional<size_t> Reporter::PromptForSelection(size_t count, std::function<bool()> isCancelled) |
There was a problem hiding this comment.
This function should not be in the Reporter; we have PromptFlow.cpp. It requests user input already. If there are improvements to be made around waiting for input, it makes sense for the Reporter to own some of that but it should never have a function that is outputting resource strings for specific workflows.
| bool selectionSupported = m_allowSelection && | ||
| (m_operationType == OperationType::Install || m_operationType == OperationType::Show || m_operationType == OperationType::Download); | ||
| bool canSelect = selectionSupported && !searchResult.Truncated && | ||
| !context.Args.Contains(Execution::Args::Type::Silent) && |
There was a problem hiding this comment.
Do we have prior art for having the silent option suppressing interactive prompts? I think maybe the 🤖 thought it meant something it doesn't.
| auto source = available->GetSource(); | ||
| std::string versionString = version ? version->GetProperty(PackageVersionProperty::Version).get() : std::string{}; | ||
| std::string sourceName = source ? source.GetDetails().Name : std::string{}; | ||
| line[3] = versionString.empty() ? Resource::LocString{ Resource::String::Unavailable }.get() : versionString; |
There was a problem hiding this comment.
It starts as the Unavailable value; don't assign that again.
| line[3] = versionString.empty() ? Resource::LocString{ Resource::String::Unavailable }.get() : versionString; | ||
| line[4] = sourceName.empty() ? Resource::LocString{ Resource::String::Unavailable }.get() : sourceName; | ||
| lines.emplace_back(line); | ||
| line[0].clear(); |
| std::string sourceName = source ? source.GetDetails().Name : std::string{}; | ||
| line[3] = versionString.empty() ? Resource::LocString{ Resource::String::Unavailable }.get() : versionString; | ||
| line[4] = sourceName.empty() ? Resource::LocString{ Resource::String::Unavailable }.get() : sourceName; | ||
| lines.emplace_back(line); |
2 participants
📖 Description
Let users resolve ambiguous matches without restarting single-package
install,show, anddownloadcommands.0or Ctrl+C.Includes localized strings, regression coverage, release notes, and a UX specification. Narrow terminals use existing table truncation.
🎞️ Demo
🔗 References
After #6514 merges, rebase and retarget this PR to
feature/multi-source-deduplication.🔍 Validation
wingetdevpassed 13 Windows ConPTY scenarios covering selection, versions, invalid/empty/long input, narrow-terminal output, cancellation, and noninteractive behavior.Full CI and manual GUI-terminal/accessibility testing remain outstanding.
✅ Checklist
🤖 AI Assistance
GitHub Copilot assisted with implementation, tests, documentation, code review, and local validation.
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow