mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-03 01:36:31 +02:00
CmdPal: Fix missing primary context command for late-bound items (#46131)
This PR does fix a bug where an item that starts with a null or empty primary command never adds that primary action to the context menu after the extension later provides a real command. - Creates the default primary context-menu item lazily when `Command` or `Command.Name` becomes available after `SlowInitializeProperties()` - Refreshes `AllCommands`, `SecondaryCommand`, and `HasMoreCommands` notifications for late command materialization and Show Details updates. - Adds unit tests to cover the fixed issue. <!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? --> ## Summary of the Pull Request <!-- Please review the items on the PR checklist before submitting--> ## PR Checklist - [x] Closes: #46129 <!-- - [ ] Closes: #yyy (add separate lines for additional resolved issues) --> - [ ] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [ ] **Tests:** Added/updated and all pass - [ ] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx <!-- Provide a more detailed description of the PR, other things fixed, or any additional comments/features here --> ## Detailed Description of the Pull Request / Additional comments <!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well --> ## Validation Steps Performed
This commit is contained in:
@@ -213,22 +213,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
|
|||||||
|
|
||||||
BuildAndInitMoreCommands();
|
BuildAndInitMoreCommands();
|
||||||
|
|
||||||
if (!string.IsNullOrEmpty(model.Command?.Name))
|
TryCreateDefaultCommandContextItem(model);
|
||||||
{
|
|
||||||
_defaultCommandContextItemViewModel = new CommandContextItemViewModel(new CommandContextItem(model.Command!), PageContext)
|
|
||||||
{
|
|
||||||
_itemTitle = Name,
|
|
||||||
Subtitle = Subtitle,
|
|
||||||
Command = Command,
|
|
||||||
|
|
||||||
// TODO this probably should just be a CommandContextItemViewModel(CommandItemViewModel) ctor, or a copy ctor or whatever
|
|
||||||
// Anything we set manually here must stay in sync with the corresponding properties on CommandItemViewModel.
|
|
||||||
};
|
|
||||||
|
|
||||||
// Only set the icon on the context item for us if our command didn't
|
|
||||||
// have its own icon
|
|
||||||
UpdateDefaultContextItemIcon();
|
|
||||||
}
|
|
||||||
|
|
||||||
lock (_moreCommandsLock)
|
lock (_moreCommandsLock)
|
||||||
{
|
{
|
||||||
@@ -238,6 +223,7 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
|
|||||||
Initialized |= InitializedState.SelectionInitialized;
|
Initialized |= InitializedState.SelectionInitialized;
|
||||||
UpdateProperty(nameof(MoreCommands));
|
UpdateProperty(nameof(MoreCommands));
|
||||||
UpdateProperty(nameof(AllCommands));
|
UpdateProperty(nameof(AllCommands));
|
||||||
|
UpdateProperty(nameof(SecondaryCommand), nameof(SecondaryCommandName), nameof(HasMoreCommands));
|
||||||
UpdateProperty(nameof(IsSelectedInitialized));
|
UpdateProperty(nameof(IsSelectedInitialized));
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -335,9 +321,16 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
|
|||||||
// or Command.Name change. This is a workaround to ensure that the Title is always up-to-date for extensions with old SDK.
|
// or Command.Name change. This is a workaround to ensure that the Title is always up-to-date for extensions with old SDK.
|
||||||
_itemTitle = model.Title;
|
_itemTitle = model.Title;
|
||||||
|
|
||||||
_defaultCommandContextItemViewModel?.Command = Command;
|
if (_defaultCommandContextItemViewModel is not null)
|
||||||
_defaultCommandContextItemViewModel?.UpdateTitle(_itemTitle);
|
{
|
||||||
UpdateDefaultContextItemIcon();
|
_defaultCommandContextItemViewModel.Command = Command;
|
||||||
|
_defaultCommandContextItemViewModel.UpdateTitle(_itemTitle);
|
||||||
|
UpdateDefaultContextItemIcon();
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
TryCreateDefaultCommandContextItem(model);
|
||||||
|
}
|
||||||
|
|
||||||
UpdateProperty(nameof(Name));
|
UpdateProperty(nameof(Name));
|
||||||
UpdateProperty(nameof(Title));
|
UpdateProperty(nameof(Title));
|
||||||
@@ -403,7 +396,15 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
|
|||||||
_titleCache.Invalidate();
|
_titleCache.Invalidate();
|
||||||
UpdateProperty(nameof(Title), nameof(Name));
|
UpdateProperty(nameof(Title), nameof(Name));
|
||||||
|
|
||||||
_defaultCommandContextItemViewModel?.UpdateTitle(model.Command.Name);
|
if (_defaultCommandContextItemViewModel is not null)
|
||||||
|
{
|
||||||
|
_defaultCommandContextItemViewModel.UpdateTitle(model.Command.Name);
|
||||||
|
}
|
||||||
|
else
|
||||||
|
{
|
||||||
|
TryCreateDefaultCommandContextItem(model);
|
||||||
|
}
|
||||||
|
|
||||||
break;
|
break;
|
||||||
|
|
||||||
case nameof(Command.Icon):
|
case nameof(Command.Icon):
|
||||||
@@ -413,6 +414,46 @@ public partial class CommandItemViewModel : ExtensionObjectViewModel, ICommandBa
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <summary>
|
||||||
|
/// Creates <see cref="_defaultCommandContextItemViewModel"/> when it does not exist
|
||||||
|
/// yet and the current command has a non-empty name. This covers the case
|
||||||
|
/// where an extension initially exposes a <c>NoOpCommand</c> (empty name)
|
||||||
|
/// and later switches to a concrete command after <see cref="SlowInitializeProperties"/> has already run.
|
||||||
|
/// When a new instance is created, the snapshot is refreshed and
|
||||||
|
/// <see cref="AllCommands"/> is notified.
|
||||||
|
/// </summary>
|
||||||
|
private void TryCreateDefaultCommandContextItem(ICommandItem model)
|
||||||
|
{
|
||||||
|
if (_defaultCommandContextItemViewModel is not null)
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (string.IsNullOrEmpty(model.Command?.Name))
|
||||||
|
{
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
|
||||||
|
_defaultCommandContextItemViewModel = new CommandContextItemViewModel(new CommandContextItem(model.Command!), PageContext)
|
||||||
|
{
|
||||||
|
_itemTitle = Name,
|
||||||
|
Subtitle = Subtitle,
|
||||||
|
Command = Command,
|
||||||
|
|
||||||
|
// TODO this probably should just be a CommandContextItemViewModel(CommandItemViewModel) ctor, or a copy ctor or whatever
|
||||||
|
// Anything we set manually here must stay in sync with the corresponding properties on CommandItemViewModel.
|
||||||
|
};
|
||||||
|
|
||||||
|
UpdateDefaultContextItemIcon();
|
||||||
|
|
||||||
|
lock (_moreCommandsLock)
|
||||||
|
{
|
||||||
|
RefreshMoreCommandStateUnsafe();
|
||||||
|
}
|
||||||
|
|
||||||
|
UpdateProperty(nameof(AllCommands));
|
||||||
|
}
|
||||||
|
|
||||||
private void UpdateDefaultContextItemIcon() =>
|
private void UpdateDefaultContextItemIcon() =>
|
||||||
|
|
||||||
// Command icon takes precedence over our icon on the primary command
|
// Command icon takes precedence over our icon on the primary command
|
||||||
|
|||||||
@@ -214,6 +214,7 @@ public partial class ListItemViewModel : CommandItemViewModel
|
|||||||
if (addedCommand)
|
if (addedCommand)
|
||||||
{
|
{
|
||||||
UpdateProperty(nameof(MoreCommands), nameof(AllCommands));
|
UpdateProperty(nameof(MoreCommands), nameof(AllCommands));
|
||||||
|
UpdateProperty(nameof(SecondaryCommand), nameof(SecondaryCommandName), nameof(HasMoreCommands));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
@@ -252,6 +253,7 @@ public partial class ListItemViewModel : CommandItemViewModel
|
|||||||
oldCommand?.SafeCleanup();
|
oldCommand?.SafeCleanup();
|
||||||
|
|
||||||
UpdateProperty(nameof(MoreCommands), nameof(AllCommands));
|
UpdateProperty(nameof(MoreCommands), nameof(AllCommands));
|
||||||
|
UpdateProperty(nameof(SecondaryCommand), nameof(SecondaryCommandName), nameof(HasMoreCommands));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,7 @@
|
|||||||
|
|
||||||
using System;
|
using System;
|
||||||
using System.Threading.Tasks;
|
using System.Threading.Tasks;
|
||||||
|
using Microsoft.CmdPal.Common.Text;
|
||||||
using Microsoft.CmdPal.UI.ViewModels.Models;
|
using Microsoft.CmdPal.UI.ViewModels.Models;
|
||||||
using Microsoft.CommandPalette.Extensions.Toolkit;
|
using Microsoft.CommandPalette.Extensions.Toolkit;
|
||||||
using Microsoft.VisualStudio.TestTools.UnitTesting;
|
using Microsoft.VisualStudio.TestTools.UnitTesting;
|
||||||
@@ -76,4 +77,64 @@ public class CommandItemViewModelTests
|
|||||||
Assert.IsNotNull(viewModel.SecondaryCommand);
|
Assert.IsNotNull(viewModel.SecondaryCommand);
|
||||||
Assert.AreEqual("Secondary", viewModel.SecondaryCommand.Name);
|
Assert.AreEqual("Secondary", viewModel.SecondaryCommand.Name);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[TestMethod]
|
||||||
|
public void LatePrimaryCommandCreation_AddsPrimaryToAllCommands()
|
||||||
|
{
|
||||||
|
// Reproduces issue where SlowInitializeProperties runs before a real primary command exists.
|
||||||
|
// The late-arriving command should still create the synthetic primary context item and prepend it to AllCommands.
|
||||||
|
var pageContext = new TestPageContext();
|
||||||
|
var item = new CommandItem()
|
||||||
|
{
|
||||||
|
Command = null,
|
||||||
|
MoreCommands =
|
||||||
|
[
|
||||||
|
new CommandContextItem(new NoOpCommand { Name = "Secondary" }),
|
||||||
|
],
|
||||||
|
};
|
||||||
|
|
||||||
|
var viewModel = new CommandItemViewModel(new(item), new(pageContext), DefaultContextMenuFactory.Instance);
|
||||||
|
viewModel.SlowInitializeProperties();
|
||||||
|
|
||||||
|
Assert.AreEqual(1, viewModel.AllCommands.Count);
|
||||||
|
Assert.AreEqual("Secondary", ((CommandContextItemViewModel)viewModel.AllCommands[0]).Name);
|
||||||
|
|
||||||
|
item.Command = new NoOpCommand { Name = "Primary" };
|
||||||
|
|
||||||
|
Assert.AreEqual(2, viewModel.AllCommands.Count);
|
||||||
|
Assert.AreEqual("Primary", ((CommandContextItemViewModel)viewModel.AllCommands[0]).Name);
|
||||||
|
Assert.AreEqual("Secondary", ((CommandContextItemViewModel)viewModel.AllCommands[1]).Name);
|
||||||
|
Assert.IsTrue(viewModel.HasMoreCommands);
|
||||||
|
Assert.AreEqual("Secondary", viewModel.SecondaryCommand?.Name);
|
||||||
|
}
|
||||||
|
|
||||||
|
[TestMethod]
|
||||||
|
public void SyntheticPrimaryContextItem_UpdatesSubtitleAndCachedSubtitleTarget()
|
||||||
|
{
|
||||||
|
// The synthetic primary context item copies subtitle state from the parent CommandItemViewModel.
|
||||||
|
// When subtitle changes later, both the exposed subtitle and its cached fuzzy-search target must refresh.
|
||||||
|
var pageContext = new TestPageContext();
|
||||||
|
var item = new CommandItem(new NoOpCommand { Name = "Primary" })
|
||||||
|
{
|
||||||
|
Subtitle = "before",
|
||||||
|
MoreCommands =
|
||||||
|
[
|
||||||
|
new CommandContextItem(new NoOpCommand { Name = "Secondary" }),
|
||||||
|
],
|
||||||
|
};
|
||||||
|
|
||||||
|
var viewModel = new CommandItemViewModel(new(item), new(pageContext), DefaultContextMenuFactory.Instance);
|
||||||
|
viewModel.SlowInitializeProperties();
|
||||||
|
|
||||||
|
var primaryContextItem = (CommandContextItemViewModel)viewModel.AllCommands[0];
|
||||||
|
var matcher = new PrecomputedFuzzyMatcher(new PrecomputedFuzzyMatcherOptions());
|
||||||
|
|
||||||
|
Assert.AreEqual("before", primaryContextItem.Subtitle);
|
||||||
|
Assert.AreEqual("before", primaryContextItem.GetSubtitleTarget(matcher).Original);
|
||||||
|
|
||||||
|
item.Subtitle = "after unique";
|
||||||
|
|
||||||
|
Assert.AreEqual("after unique", primaryContextItem.Subtitle);
|
||||||
|
Assert.AreEqual("after unique", primaryContextItem.GetSubtitleTarget(matcher).Original);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user