[TransitioningContentControl] Manage his LogicalChildren

[danielmayost]

What does the pull request do?

There are two problems with TransitioningContentControl:

• Duplicated logical children, this is because ContentPresenter manages his host's LogicalChildren (in this case TCC), and in addition ContentControl also manages LogicalChildren, which causes logical children to be duplicated.
• The first ContentPresenter not register the TCC as its Host, which causes it to not manage the LogicalChildren of the TCC.

We need to decide who will manage the logical children, CC (ContentControl) or TCC, in this case it is clear that TCC should do it, because CC is only adapted to one child, whereas TCC can have two (during animation).

The implementation was carried out by adding a BypassLogicalChildrenManagement property to the CC that allows the controls inheriting from it to manage the LogicalChildren themselves, in this case, TCC does not need to do anything, his presenters do it for him. see comment below.

The second problem is a bug in the RegisterContentPresenter implementation that returned false even if Presenter is PART_ContentPresenter, which causes the Host registration to be skipped:

Avalonia/src/Avalonia.Controls/Presenters/ContentPresenter.cs

Lines 702 to 706 in 124f124

	private void TemplatedParentChanged(AvaloniaPropertyChangedEventArgs e)
	{
	var host = e.NewValue as IContentPresenterHost;
	Host = host?.RegisterContentPresenter(this) == true ? host : null;
	}

The fixes is pretty obvious.

Checklist

• Added unit tests (if possible)?
• Added XML documentation to any related classes?
• Consider submitting a PR to https://github.com/AvaloniaUI/Documentation with user documentation

Breaking changes

No.

Fixed issues

Fixes #12158

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0037671-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0037673-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[MrJul]

Nice one!

ContentPresenter automatically setting the ContentControl logical children makes sense, but it's really easy to forget about this relationship.

I think it would be nice to have debug warnings in the future when a logical child is added to a parent while it already has one. Even if there are valid use cases for this (as discussed with grokys previously in #10413), it could help catching this kind of problem sooner.

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0037696-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[X9VoiD]

Can confirm it works! 👍

The ContentChanged could probably be a protected virtual. I'm not familiar with Avalonia's codebase tho and I might just be missing information so feel free to disregard what I said.

[danielmayost]

The ContentChanged could probably be a protected virtual.

Then when we override it in the inheriting class without implementing anything, it won't be so clear why. I thought that way it would be clearer.

[MrJul]

Then when we override it in the inheriting class without implementing anything, it won't be so clear why. I thought that way it would be clearer.

A simple comment could work. Alternatively, ContentControl can be changed to also use OnPropertyChanged, with TransitioningContentControl doing the same without calling the base for ContentProperty (with a comment, still). No new public members this way.

[danielmayost]

A simple comment could work.

IMO it doesn't look aesthetic, but you decide.

Alternatively, ContentControl can be changed to also use OnPropertyChanged, with TransitioningContentControl doing the same without calling the base for ContentProperty (with a comment, still).

Oh, this is a bad idea IMO, for two reasons:

• Avalonia's analyzer doesn't like it (actually throws a compilation error), to overriding OnPropertyChanged and not calling the base class.
• This is a problematic practice, you can't know who is in the hierarchy chain that OnPropertyChanged must be called, if you don't want to call the one in front of you it doesn't mean you don't want to call the one that's twice behind it.

[MrJul]

I think there's a misunderstanding here, I was definitely not advocating on never calling the base.OnPropertyChanged, that would indeed break everything! Sorry if I wasn't clear.

Not calling the base was only for ContentProperty in TCC, and after checking the code it's already the very case today:

protected override void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
{
	if (change.Property == ContentProperty)
	{
		UpdateContent(true);
		return;
	}

	base.OnPropertyChanged(change);
}

(ContentControl would need to use OnPropertyChanged instead of AddClassHandler.)

[danielmayost]

Got it, so what's better?
I think the second option is better (OnPropertyChanged).

Edit: On second thought it still sounds like a bad idea to me. Consider the following case:

public class Class1
{
    protected void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
    {
        base.OnPropertyChanged(change);

        if (change.Property == ContentProperty)
        {
            // Do something that must be done
        }
}

public class Class2 : Class1
{
    protected void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
    {
        base.OnPropertyChanged(change);

        if (change.Property == ContentProperty)
        {
            // Do something that doesn't have to be done
        }
}

public class Class3 : Class2
{
    protected void OnPropertyChanged(AvaloniaPropertyChangedEventArgs change)
    {
        if (change.Property == ContentProperty)
        {
            // Skip what doesn't have to be done
            return;
        }

        base.OnPropertyChanged(change);
}

The code in Class1 will not be executed, we just wanted to skip on the Class2 code.

[MrJul]

Yes, that's a general problems with overrides and I agree it can be a pain when you're class 3 and need to repeat a behavior from class 1 that have been inhibited by class 2. Unfortunately C# doesn't allow calling an arbitrary ancestor's virtual method non-virtually (it's possible in IL).

In this particular case, the Content property is defined by ContentControl: TransitioningContentControl being a direct inheritor, there's no class before or between, plus both classes are in the same codebase: everything is in control. (The problem might apply to a potential inheritor of TransitioningContentControl, but I don't think that's a scenario we need to support.)

To me it's a win as it avoids defining a specific public flag that will only be used by TransitioningContentControl in practice. That said, I'll let an OG team member decides what's best.

[danielmayost]

@MrJul No matter, It's done, can it be merged now?

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0038197-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[avaloniaui-team]

You can test this PR using the following package version. 11.0.999-cibuild0038428-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

[MrJul]

LGTM, thanks!