From 39fd3af9cadab99e5c9a29e55475071e4ce79964 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 10 Dec 2019 15:11:37 +0100 Subject: [PATCH 1/4] Added sanity test for logical parent. Allows early detection of issue #3328. --- src/Avalonia.Styling/StyledElement.cs | 6 +++++ .../StyledElementTests.cs | 23 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/Avalonia.Styling/StyledElement.cs b/src/Avalonia.Styling/StyledElement.cs index 5e1bcde2f67..9e61871f793 100644 --- a/src/Avalonia.Styling/StyledElement.cs +++ b/src/Avalonia.Styling/StyledElement.cs @@ -704,6 +704,12 @@ private static void ValidateLogicalChild(ILogical c) private void OnAttachedToLogicalTreeCore(LogicalTreeAttachmentEventArgs e) { + if (this.GetLogicalParent() == null && !(this is IStyleRoot)) + { + throw new InvalidOperationException( + $"AttachedToLogicalTreeCore called for '{GetType().Name}' but control has no logical parent."); + } + // This method can be called when a control is already attached to the logical tree // in the following scenario: // - ListBox gets assigned Items containing ListBoxItem diff --git a/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs b/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs index c40e6c735e0..8e08c5ce360 100644 --- a/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs +++ b/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs @@ -76,6 +76,29 @@ public void InheritanceParent_Should_Be_Cleared_When_Removed_From_Parent_When_Ha Assert.Null(target.InheritanceParent); } + [Fact] + public void Adding_Element_With_Null_Parent_To_Logical_Tree_Should_Throw() + { + var target = new Border(); + var visualParent = new Panel(); + var logicalParent = new Panel(); + var root = new TestRoot(); + + // Set the logical parent... + ((ISetLogicalParent)target).SetParent(logicalParent); + + // ...so that when it's added to `visualParent`, the parent won't be set again. + visualParent.Children.Add(target); + + // Clear the logical parent. It's now a logical child of `visualParent` but doesn't have + // a logical parent itself. + ((ISetLogicalParent)target).SetParent(null); + + // In this case, attaching the control to a logical tree should throw. + logicalParent.Children.Add(visualParent); + Assert.Throws(() => root.Child = logicalParent); + } + [Fact] public void AttachedToLogicalParent_Should_Be_Called_When_Added_To_Tree() { From a70da659f6f120bceddc0651bf66ba4447f8e472 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Tue, 10 Dec 2019 16:24:26 +0100 Subject: [PATCH 2/4] Reafactor logical tree attach/detach events. - When detaching set `Parent` to `null` before raising the `DetachedFromLogicalTree` event - To do this, we need to store the logical root in the control so that e.g. a child control can be detached in a `DetachedFromLogicalTree` handler - Add `Source` and `Parent` properties to `LogicalTreeAttachmentEventArgs` to give more context on what caused the attachment/detachment --- .../Primitives/VisualLayerManager.cs | 4 +- src/Avalonia.Controls/TopLevel.cs | 2 +- .../LogicalTreeAttachmentEventArgs.cs | 32 +++++++++- src/Avalonia.Styling/StyledElement.cs | 37 +++++------ .../TopLevelTests.cs | 25 ++++++++ .../StyledElementTests.cs | 63 +++++++++++++++++++ 6 files changed, 136 insertions(+), 27 deletions(-) diff --git a/src/Avalonia.Controls/Primitives/VisualLayerManager.cs b/src/Avalonia.Controls/Primitives/VisualLayerManager.cs index b7229eb1211..00652f21ba7 100644 --- a/src/Avalonia.Controls/Primitives/VisualLayerManager.cs +++ b/src/Avalonia.Controls/Primitives/VisualLayerManager.cs @@ -8,7 +8,7 @@ public class VisualLayerManager : Decorator { private const int AdornerZIndex = int.MaxValue - 100; private const int OverlayZIndex = int.MaxValue - 99; - private IStyleHost _styleRoot; + private IStyleRoot _styleRoot; private readonly List _layers = new List(); @@ -53,7 +53,7 @@ void AddLayer(Control layer, int zindex) layer.ZIndex = zindex; VisualChildren.Add(layer); if (((ILogical)this).IsAttachedToLogicalTree) - ((ILogical)layer).NotifyAttachedToLogicalTree(new LogicalTreeAttachmentEventArgs(_styleRoot)); + ((ILogical)layer).NotifyAttachedToLogicalTree(new LogicalTreeAttachmentEventArgs(_styleRoot, layer, this)); InvalidateArrange(); } diff --git a/src/Avalonia.Controls/TopLevel.cs b/src/Avalonia.Controls/TopLevel.cs index 5a8711a21c0..799e6c81d61 100644 --- a/src/Avalonia.Controls/TopLevel.cs +++ b/src/Avalonia.Controls/TopLevel.cs @@ -266,7 +266,7 @@ protected virtual void HandlePaint(Rect rect) /// protected virtual void HandleClosed() { - var logicalArgs = new LogicalTreeAttachmentEventArgs(this); + var logicalArgs = new LogicalTreeAttachmentEventArgs(this, this, null); ((ILogical)this).NotifyDetachedFromLogicalTree(logicalArgs); var visualArgs = new VisualTreeAttachmentEventArgs(this, this); diff --git a/src/Avalonia.Styling/LogicalTree/LogicalTreeAttachmentEventArgs.cs b/src/Avalonia.Styling/LogicalTree/LogicalTreeAttachmentEventArgs.cs index 1b0eb2b61be..dc1b801a5c2 100644 --- a/src/Avalonia.Styling/LogicalTree/LogicalTreeAttachmentEventArgs.cs +++ b/src/Avalonia.Styling/LogicalTree/LogicalTreeAttachmentEventArgs.cs @@ -16,16 +16,44 @@ public class LogicalTreeAttachmentEventArgs : EventArgs /// Initializes a new instance of the class. /// /// The root of the logical tree. - public LogicalTreeAttachmentEventArgs(IStyleHost root) + /// The control being attached/detached. + /// The . + public LogicalTreeAttachmentEventArgs( + IStyleRoot root, + ILogical source, + ILogical parent) { Contract.Requires(root != null); + Contract.Requires(source != null); Root = root; + Source = source; + Parent = parent; } /// /// Gets the root of the logical tree that the control is being attached to or detached from. /// - public IStyleHost Root { get; } + public IStyleRoot Root { get; } + + /// + /// Gets the control that was attached or detached from the logical tree. + /// + /// + /// Logical tree attachment events travel down the attached logical tree from the point of + /// attachment/detachment, so this control may be different from the control that the + /// event is being raised on. + /// + public ILogical Source { get; } + + /// + /// Gets the control that is being attached to or detached from. + /// + /// + /// For logical tree attachment, holds the new logical parent of . For + /// detachment, holds the old logical parent of . If the detachment event + /// was caused by a top-level control being closed, then this property will be null. + /// + public ILogical Parent { get; } } } diff --git a/src/Avalonia.Styling/StyledElement.cs b/src/Avalonia.Styling/StyledElement.cs index 9e61871f793..69c2d93247a 100644 --- a/src/Avalonia.Styling/StyledElement.cs +++ b/src/Avalonia.Styling/StyledElement.cs @@ -59,7 +59,7 @@ public class StyledElement : Animatable, IDataContextProvider, IStyledElement, I private int _initCount; private string _name; private readonly Classes _classes = new Classes(); - private bool _isAttachedToLogicalTree; + private IStyleRoot _styleRoot; private IAvaloniaList _logicalChildren; private IResourceDictionary _resources; private Styles _styles; @@ -81,7 +81,7 @@ static StyledElement() /// public StyledElement() { - _isAttachedToLogicalTree = this is IStyleRoot; + _styleRoot = this as IStyleRoot; } /// @@ -307,7 +307,7 @@ protected IAvaloniaList LogicalChildren /// /// Gets a value indicating whether the element is attached to a rooted logical tree. /// - bool ILogical.IsAttachedToLogicalTree => _isAttachedToLogicalTree; + bool ILogical.IsAttachedToLogicalTree => _styleRoot != null; /// /// Gets the styled element's logical parent. @@ -367,7 +367,7 @@ public virtual void EndInit() throw new InvalidOperationException("BeginInit was not called."); } - if (--_initCount == 0 && _isAttachedToLogicalTree) + if (--_initCount == 0 && _styleRoot != null) { InitializeStylesIfNeeded(); @@ -435,19 +435,6 @@ void ISetLogicalParent.SetParent(ILogical parent) throw new InvalidOperationException("The Control already has a parent."); } - if (_isAttachedToLogicalTree) - { - var oldRoot = FindStyleRoot(old) ?? this as IStyleRoot; - - if (oldRoot == null) - { - throw new AvaloniaInternalException("Was attached to logical tree but cannot find root."); - } - - var e = new LogicalTreeAttachmentEventArgs(oldRoot); - OnDetachedFromLogicalTreeCore(e); - } - if (InheritanceParent == null || parent == null) { InheritanceParent = parent as AvaloniaObject; @@ -455,6 +442,12 @@ void ISetLogicalParent.SetParent(ILogical parent) Parent = (IStyledElement)parent; + if (_styleRoot != null) + { + var e = new LogicalTreeAttachmentEventArgs(_styleRoot, this, old); + OnDetachedFromLogicalTreeCore(e); + } + if (old != null) { old.ResourcesChanged -= ThisResourcesChanged; @@ -474,7 +467,7 @@ void ISetLogicalParent.SetParent(ILogical parent) throw new AvaloniaInternalException("Parent is attached to logical tree but cannot find root."); } - var e = new LogicalTreeAttachmentEventArgs(newRoot); + var e = new LogicalTreeAttachmentEventArgs(newRoot, this, parent); OnAttachedToLogicalTreeCore(e); } @@ -716,9 +709,9 @@ private void OnAttachedToLogicalTreeCore(LogicalTreeAttachmentEventArgs e) // - ListBox makes ListBoxItem a logical child // - ListBox template gets applied; making its Panel get attached to logical tree // - That AttachedToLogicalTree signal travels down to the ListBoxItem - if (!_isAttachedToLogicalTree) + if (_styleRoot == null) { - _isAttachedToLogicalTree = true; + _styleRoot = e.Root; InitializeStylesIfNeeded(true); @@ -734,9 +727,9 @@ private void OnAttachedToLogicalTreeCore(LogicalTreeAttachmentEventArgs e) private void OnDetachedFromLogicalTreeCore(LogicalTreeAttachmentEventArgs e) { - if (_isAttachedToLogicalTree) + if (_styleRoot != null) { - _isAttachedToLogicalTree = false; + _styleRoot = null; _styleDetach.OnNext(this); OnDetachedFromLogicalTree(e); DetachedFromLogicalTree?.Invoke(this, e); diff --git a/tests/Avalonia.Controls.UnitTests/TopLevelTests.cs b/tests/Avalonia.Controls.UnitTests/TopLevelTests.cs index 72c81659c3b..91592b1ff72 100644 --- a/tests/Avalonia.Controls.UnitTests/TopLevelTests.cs +++ b/tests/Avalonia.Controls.UnitTests/TopLevelTests.cs @@ -166,6 +166,31 @@ public void Impl_Close_Should_Call_Raise_Closed_Event() } } + [Fact] + public void Impl_Close_Should_Raise_DetachedFromLogicalTree_Event() + { + using (UnitTestApplication.Start(TestServices.StyledWindow)) + { + var impl = new Mock(); + impl.SetupAllProperties(); + + var target = new TestTopLevel(impl.Object); + var raised = 0; + + target.DetachedFromLogicalTree += (s, e) => + { + Assert.Same(target, e.Root); + Assert.Same(target, e.Source); + Assert.Null(e.Parent); + ++raised; + }; + + impl.Object.Closed(); + + Assert.Equal(1, raised); + } + } + [Fact] public void Impl_Input_Should_Pass_Input_To_InputManager() { diff --git a/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs b/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs index 8e08c5ce360..ae519ed6747 100644 --- a/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs +++ b/tests/Avalonia.Styling.UnitTests/StyledElementTests.cs @@ -167,6 +167,50 @@ public void AttachedToLogicalParent_Should_Not_Be_Called_With_GlobalStyles_As_Ro Assert.True(raised); } + [Fact] + public void AttachedToLogicalParent_Should_Have_Source_Set() + { + var root = new TestRoot(); + var canvas = new Canvas(); + var border = new Border { Child = canvas }; + var raised = 0; + + void Attached(object sender, LogicalTreeAttachmentEventArgs e) + { + Assert.Same(border, e.Source); + ++raised; + } + + border.AttachedToLogicalTree += Attached; + canvas.AttachedToLogicalTree += Attached; + + root.Child = border; + + Assert.Equal(2, raised); + } + + [Fact] + public void AttachedToLogicalParent_Should_Have_Parent_Set() + { + var root = new TestRoot(); + var canvas = new Canvas(); + var border = new Border { Child = canvas }; + var raised = 0; + + void Attached(object sender, LogicalTreeAttachmentEventArgs e) + { + Assert.Same(root, e.Parent); + ++raised; + } + + border.AttachedToLogicalTree += Attached; + canvas.AttachedToLogicalTree += Attached; + + root.Child = border; + + Assert.Equal(2, raised); + } + [Fact] public void DetachedFromLogicalParent_Should_Be_Called_When_Removed_From_Tree() { @@ -213,6 +257,25 @@ public void DetachedFromLogicalParent_Should_Not_Be_Called_With_GlobalStyles_As_ Assert.True(raised); } + [Fact] + public void Parent_Should_Be_Null_When_DetachedFromLogicalParent_Called() + { + var target = new TestControl(); + var root = new TestRoot(target); + var called = 0; + + target.DetachedFromLogicalTree += (s, e) => + { + Assert.Null(target.Parent); + Assert.Null(target.InheritanceParent); + ++called; + }; + + root.Child = null; + + Assert.Equal(1, called); + } + [Fact] public void Adding_Tree_To_IStyleRoot_Should_Style_Controls() { From 14ed1fd571f3a5434d5e0d3eca7705626f8d6ca5 Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 11 Dec 2019 09:31:54 +0100 Subject: [PATCH 3/4] Added failing test for #3328. --- .../TreeViewTests.cs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/Avalonia.Controls.UnitTests/TreeViewTests.cs b/tests/Avalonia.Controls.UnitTests/TreeViewTests.cs index ed8a39d0635..a2892be5c25 100644 --- a/tests/Avalonia.Controls.UnitTests/TreeViewTests.cs +++ b/tests/Avalonia.Controls.UnitTests/TreeViewTests.cs @@ -971,6 +971,40 @@ public void Auto_Expanding_In_Style_Should_Not_Break_Range_Selection() } } + [Fact] + public void Removing_TreeView_From_Root_Should_Preserve_TreeViewItems() + { + // Issue #3328 + var tree = CreateTestTreeData(); + var target = new TreeView + { + Template = CreateTreeViewTemplate(), + Items = tree, + }; + + var root = new TestRoot(); + root.Child = target; + + CreateNodeDataTemplate(target); + ApplyTemplates(target); + ExpandAll(target); + + Assert.Equal(5, target.ItemContainerGenerator.Index.Containers.Count()); + + root.Child = null; + + Assert.Equal(5, target.ItemContainerGenerator.Index.Containers.Count()); + Assert.Equal(1, target.Presenter.Panel.Children.Count); + + var rootNode = Assert.IsType(target.Presenter.Panel.Children[0]); + Assert.Equal(3, rootNode.ItemContainerGenerator.Containers.Count()); + Assert.Equal(3, rootNode.Presenter.Panel.Children.Count); + + var child2Node = Assert.IsType(rootNode.Presenter.Panel.Children[1]); + Assert.Equal(1, child2Node.ItemContainerGenerator.Containers.Count()); + Assert.Equal(1, child2Node.Presenter.Panel.Children.Count); + } + private void ApplyTemplates(TreeView tree) { tree.ApplyTemplate(); From 5a67237be344a1062fb198d9bc7df982a5b8f08b Mon Sep 17 00:00:00 2001 From: Steven Kirk Date: Wed, 11 Dec 2019 09:32:59 +0100 Subject: [PATCH 4/4] Fix updating index when TreeView reparented. When an entire treeview is reparented, don't detach the child `TreeViewItem`s. Fixes #3328 --- .../Generators/TreeItemContainerGenerator.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Avalonia.Controls/Generators/TreeItemContainerGenerator.cs b/src/Avalonia.Controls/Generators/TreeItemContainerGenerator.cs index 9200490668b..22e06a40b1a 100644 --- a/src/Avalonia.Controls/Generators/TreeItemContainerGenerator.cs +++ b/src/Avalonia.Controls/Generators/TreeItemContainerGenerator.cs @@ -127,7 +127,7 @@ public void UpdateIndex() Index = new TreeContainerIndex(); _treeView = treeViewOwner; } - else if (Owner.IsAttachedToLogicalTree) + else { var treeView = Owner.GetSelfAndLogicalAncestors().OfType().FirstOrDefault(); @@ -138,12 +138,6 @@ public void UpdateIndex() _treeView = treeView; } } - else - { - Clear(); - Index = null; - _treeView = null; - } } class WrapperTreeDataTemplate : ITreeDataTemplate