CmdPal: Add support for List parameters#47885
Conversation
michaeljolley
left a comment
There was a problem hiding this comment.
Review Findings
1. [HIGH] Dual event handling — possible double-advance on value pick
Both ItemPropertyChanged (on NeedsValue change) and ListParamValueChanged fire when an extension confirms a value. Both independently marshal to the UI thread via DoOnUiThread and call SetActiveListParameter(null) + FocusNextParameter(). Since they are dispatched as separate UI-thread callbacks, there is a race where both could execute before the other's SetActiveListParameter(null) takes effect.
The cmdParam == _activeListParam guard should catch this (the second dispatch would see _activeListParam as null and skip), but this depends on both dispatched callbacks running sequentially on the UI thread. I'd recommend either:
- Adding a unit test that verifies no double-advance occurs when both events fire for the same value pick, or
- Consolidating the advance logic into a single code path so only one of the two handlers calls
FocusNextParameter()
3. [MEDIUM] ListParameter_LostFocus suppression during editing
When IsEditing is true, ListParameter_LostFocus in SearchBar.xaml.cs intentionally does not clear the active list parameter — this is by design since clicking a list item in ParametersPage steals focus from the textbox. However, if the user tabs away to a different parameter without picking a value, the list panel remains visible because nothing clears IsEditing or the active list.
Consider handling VirtualKey.Tab in ListParameter_PreviewKeyDown to call CancelEditing() and SetActiveListParameter(null), similar to how Escape is already handled. This would ensure the list dismisses when the user explicitly navigates away via keyboard.
The placeholders page for a bookmark is already goofy. It's a seemingly simple form, for just filling in something that really should be an inline value. So let's do that! This moves the placeholders out of a whole adaptive card, and into an inline parameter. Targets #47885 <img width="790" height="635" alt="image" src="https://github.com/user-attachments/assets/d0faa5a2-e967-4860-b3d2-b6bcb0c91c0b" /> <img width="787" height="798" alt="image" src="https://github.com/user-attachments/assets/36ff41c3-8c6e-48bf-a141-9655b463d049" />
This PR is stacked on top of #47826
This augments the previous PR to add support for List parameters too. This allows extensions to add parameters where the "command" to select a value is just a
IListPage. That lets the extension author specify a set of possible values, and let us handle the rendering of that list.The canonical example is the
CreateNoteParametersPage, inParameterSamples.cs. In that sample, there's a set of possible values for the folder param. The extension exposes that to us, and we can then let the user navigate thatlist just like any other list page.
params-list-000.mp4
Unfortunately, this kinda means that we need to refactor ListPage a bit. We need to slice it, so that we can have the list itself as it's own control, separate from the "page". This lets us easily add the
ListItemsViewto the parameters page too. All of that code should basically be copied verbatim. Sorry that github will have a hard time comparing the two.