Add interactive selection for ambiguous package matches - #6575
AmirMS (AmelBawa-msft) wants to merge 33 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ranm-msft
left a comment
There was a problem hiding this comment.
I checked this against the invariant that matters most for a new prompt: nothing that is not a real interactive console session should ever start waiting on input. That holds here. The disable-interactivity switch, the settings-level disable, hidden and non-output channels, and a redirected stdin or stdout all fall back to the existing multiple-applications-found error rather than prompting, and the unambiguous single-match path is untouched. Strings are in the resource file with the placeholders locked, and there are tests for both the non-interactive fallback and user cancellation.
Two things I would want settled before this goes in.
The first is evidence rather than design: the build and test job is skipped at the current head, so nothing has actually run the new tests. I would like a green run before merge, since the whole value of this change is that the prompt appears exactly when it should and never otherwise.
The second is the product question already on the thread. Deliberately letting the silent switch through to this prompt is defensible - silent describes installer behavior, not CLI interactivity - but it is the kind of decision that surprises someone scripting against the CLI. Routing a new interactive surface through the experimental feature process first would let us change our minds cheaply. I do not feel strongly about which way that lands, only that it should be an explicit call.
1de7935 to
b85ba3b
Compare
This comment was marked as resolved.
This comment was marked as resolved.
| // Outputs: None | ||
| void ReportMultiplePackageFoundResult(Execution::Context& context); | ||
| // Builds the ambiguity table for installed-package matches. | ||
| Execution::TableOutputBase GetMultiplePackageFoundResultTable(Execution::Context& context); |
There was a problem hiding this comment.
Do these need to be public? I didn't see any product use of them outside of the implementation .cpp.
| auto table = operationTargetsInstalled ? GetMultiplePackageFoundResultTable(context) : | ||
| GetMultiplePackageFoundResultTableWithSource(context); | ||
| bool selectionSupported = m_selectionBehavior == PackageSelectionBehavior::Prompt && | ||
| (m_operationType == OperationType::Install || m_operationType == OperationType::Show || m_operationType == OperationType::Download) && |
There was a problem hiding this comment.
With the change to use those existing tables, the operation type should no longer be a blocker. Unless the blocker was more about deciding which paths should enable selection.
| } | ||
| if (selectionSupported) | ||
| { | ||
| context.Reporter.Info() << Resource::String::PackageSelectionRefine << std::endl; |
There was a problem hiding this comment.
Should this just be output regardless of selection support?
📖 Description
Let users resolve ambiguous matches without restarting single-package
install,show, anddownloadcommands. Opt in withexperimentalFeatures.interactivePackageSelection.0or Ctrl+C.Includes localized strings, regression coverage, release notes, and a UX specification. Narrow terminals use existing table truncation.
🎞️ Demo
🔗 References
🔍 Validation
wingetdevfromd729b8baon the host. Focused native tests passed: 3,471 assertions across 92 test cases.--silentprompting, and 24-column tables.--no-vt. UI Automation exposed the table, prompt, retry feedback, and full selected ID.show,install, anddownloadreturnedE_ABORT. The slowest cancellation was about 0.118 seconds.Screen-reader speech was not tested.
✅ Checklist
🤖 AI Assistance
GitHub Copilot assisted with implementation, tests, documentation, code review, and local validation.
📋 Issue Type
Microsoft Reviewers: Open in CodeFlow