Mirrors/PowerToys
1
0
Fork 0
mirror of https://github.com/microsoft/PowerToys.git synced 2026-09-01 19:51:34 +02:00
Code Issues Packages Projects Releases Wiki Activity

CmdPal SDK: Fix weak command property subscriptions (#49731)

Browse Source 
<!-- 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

This PR fixes a weak-event subscription in the CmdPal toolkit that still
captured its owning `CommandItem` through an instance callback. That
strong reference defeated the weak listener, while command replacement
could also leave a stale handler attached to the outgoing command.

- Make the command property-change callback static.
- Resolve the owning `CommandItem` through the listener's weak
reference.
- Explicitly unsubscribe from the outgoing command during replacement.
- Retain the detach callback that removes dead listeners from long-lived
commands.
This commit is contained in:
Jiří Polášek
2026-08-07 19:28:40 +02:00
committed by GitHub
parent d70ab95355
commit a5b1ec8124
1 changed files with 16 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
src/modules/cmdpal/extensionsdk/Microsoft.CommandPalette.Extensions.Toolkit/CommandItem.cs
View File

@@ -55,8 +55,12 @@ public partial class CommandItem : BaseObservable, ICommandItem
var oldTitle = Title;
// Unsubscribe the outgoing command explicitly. OnDetachAction only
// runs once this CommandItem has been collected, so it can't cover
// the case where the command is simply replaced.
if (_commandListener is not null)
{
_command?.PropChanged -= _commandListener.OnEvent;
_commandListener.Detach();
_commandListener = null;
}
@@ -65,6 +69,16 @@ public partial class CommandItem : BaseObservable, ICommandItem
if (value is not null)
{
// OnCommandPropertyChanged must be static so the delegate's Target is null.
// An instance method group would bind `this` into OnEventAction,
// giving the listener a strong ref back to this CommandItem and
// defeating the weak reference. That was the actual leak.
//
// OnDetachAction does capture `value`, but that is not a leak: the
// only thing keeping the listener alive is `value`'s own PropChanged
// list, so the two form a cycle the GC reclaims together. Without it
// the listener could never unsubscribe itself, and every collected
// CommandItem would leave a dead handler on a long-lived command.
_commandListener = new(this, OnCommandPropertyChanged, listener => value.PropChanged -= listener.OnEvent);
value.PropChanged += _commandListener.OnEvent;
}
@@ -77,10 +91,10 @@ public partial class CommandItem : BaseObservable, ICommandItem
}
}
private void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args)
private static void OnCommandPropertyChanged(CommandItem instance, object source, IPropChangedEventArgs args)
{
// command's name affects Title only if Title wasn't explicitly set
if (args.PropertyName == nameof(ICommand.Name) && string.IsNullOrEmpty(_title))
if (args.PropertyName == nameof(ICommand.Name) && string.IsNullOrEmpty(instance._title))
{
instance.OnPropertyChanged(nameof(Title));
}