Button.IsCancel still works for detached button + MemoryLeak

[jinek]

Describe the bug
Button.StopListeningForCancel is called only when property IsCancel is set to false and is not called when button is detached from the visual tree.
Additionally that also means that root element holds reference to the button forewer:

        private void ListenForCancel(IInputElement root)
        {
            root.AddHandler(KeyDownEvent, RootCancelKeyDown);
        }

• Version 0.10.12

[robloo]

Note: This issue would also apply for IsDefault

I can't speak for the correctness of your solution. However, it should be easy to add the code from the OnPropertyChanged method here:

Avalonia/src/Avalonia.Controls/Button.cs

Lines 235 to 267 in d0f3fed

	protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
	{
	if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control
	{
	HotKey = _hotkey;
	}
	base.OnAttachedToLogicalTree(e);
	if (Command != null)
	{
	Command.CanExecuteChanged += CanExecuteChanged;
	CanExecuteChanged(this, EventArgs.Empty);
	}
	}
	/// <inheritdoc/>
	protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
	{
	// This will cause the hotkey manager to dispose the observer and the reference to this control
	if (HotKey != null)
	{
	_hotkey = HotKey;
	HotKey = null;
	}
	base.OnDetachedFromLogicalTree(e);
	if (Command != null)
	{
	Command.CanExecuteChanged -= CanExecuteChanged;
	}
	}

[jinek]

Note: This issue would also apply for IsDefault

I can't speak for the correctness of your solution. However, it should be easy to add the code from the OnPropertyChanged method here:

Avalonia/src/Avalonia.Controls/Button.cs

Lines 235 to 267 in d0f3fed

	protected override void OnAttachedToLogicalTree(LogicalTreeAttachmentEventArgs e)
	{
	if (_hotkey != null) // Control attached again, set Hotkey to create a hotkey manager for this control
	{
	HotKey = _hotkey;
	}
	base.OnAttachedToLogicalTree(e);
	if (Command != null)
	{
	Command.CanExecuteChanged += CanExecuteChanged;
	CanExecuteChanged(this, EventArgs.Empty);
	}
	}
	/// <inheritdoc/>
	protected override void OnDetachedFromLogicalTree(LogicalTreeAttachmentEventArgs e)
	{
	// This will cause the hotkey manager to dispose the observer and the reference to this control
	if (HotKey != null)
	{
	_hotkey = HotKey;
	HotKey = null;
	}
	base.OnDetachedFromLogicalTree(e);
	if (Command != null)
	{
	Command.CanExecuteChanged -= CanExecuteChanged;
	}
	}

Now I'm thinking may be that is conceptual issue? May be events automatically should unsubsribe when element is detached?

[robloo]

We'll need someone from the core team to weigh-in with how things are supposed to work. At first glance, it appears you are correct though and the button should be automatically unsubscribing when adding/removing from the logical tree -- not only when the property value changes. The Command is already handled this way.

Code I would be looking to add to OnAttached/DetachedLogicalTree (or perhaps VisualTree) methods is from the OnPropertyChanged method:

Avalonia/src/Avalonia.Controls/Button.cs

Lines 415 to 425 in d0f3fed

	if (VisualRoot is IInputElement inputRoot)
	{
	if (isCancel)
	{
	ListenForCancel(inputRoot);
	}
	else
	{
	StopListeningForCancel(inputRoot);
	}
	}

[maxkatz6]

Yes. Subscribing and unsubscribing should be done in OnAttached/Detached methods.
We can find it was already done for IsDefault and IsCancel in OnAttachedToVisualTree/OnDetachedFromVisualTree.
But cleanup is done only for IsDefault, IsCancel was missed. See https://github.com/AvaloniaUI/Avalonia/blame/master/src/Avalonia.Controls/Button.cs#L228-L234

[maxkatz6]

Solution is to unsubscribe IsCancel in OnDetachedFromVisualTree method.

[robloo]

@maxkatz6 Thanks for confirming. Since I've done some work with Button recently and know my way around this code I'll fix this one. No guarantees on timing but it will be done before 11.0.