From 3d88f4f103293d32329b06cf7679ca6d54e3f926 Mon Sep 17 00:00:00 2001 From: Shanmukha Ranganath Date: Fri, 25 Oct 2019 19:07:24 +0530 Subject: [PATCH 1/6] Ordering children while adding --- .../Issue8129.cs | 176 ++++++++++++++++++ ...rin.Forms.Controls.Issues.Shared.projitems | 1 + .../VisualElementPackager.cs | 18 +- 3 files changed, 193 insertions(+), 2 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs new file mode 100644 index 00000000000..03ae0e6ad6c --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs @@ -0,0 +1,176 @@ +using System.Linq; +using System.Threading.Tasks; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using System; + +#if UITEST +using Xamarin.Forms.Core.UITests; +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ +#if UITEST + [Category(UITestCategories.RefreshView)] +#endif + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 8129, "[Bug] Adding children to iOS VisualElementPackager has O(N^2) performance and thrashes the native layer", PlatformAffected.iOS)] + public class Issue8129 : TestContentPage + { + RefreshView _refreshView; + Command _refreshCommand; + + public Issue8129() + { + } + + protected override void Init() + { + Title = "Refresh View with too many elements (2000) Tests"; + var scrollViewContent = new StackLayout(); + + Enumerable + .Range(0, 2000) + .Select(_ => new Label() { HeightRequest = 200, Text = $"Pull me down to refresh me - {_}" }) + .ForEach(x => scrollViewContent.Children.Add(x)); + + + bool canExecute = true; + _refreshCommand = new Command(async (parameter) => + { + if (!_refreshView.IsRefreshing) + { + throw new Exception("IsRefreshing should be true when command executes"); + } + + if (parameter != null && !(bool)parameter) + { + throw new Exception("Refresh command incorrectly firing with disabled parameter"); + } + + await Task.Delay(2000); + _refreshView.IsRefreshing = false; + }, (object parameter) => + { + return parameter != null && canExecute && (bool)parameter; + }); + + _refreshView = new RefreshView() + { + Content = new ScrollView() + { + HeightRequest = 2000, + BackgroundColor = Color.Green, + Content = scrollViewContent, + AutomationId = "LayoutContainer" + }, + Command = _refreshCommand, + CommandParameter = true + }; + + var isRefreshingLabel = new Label(); + + var label = new Label { BindingContext = _refreshView }; + isRefreshingLabel.SetBinding(Label.TextProperty, new Binding("IsRefreshing", stringFormat: "IsRefreshing: {0}", source: _refreshView)); + + var commandEnabledLabel = new Label { BindingContext = _refreshView }; + commandEnabledLabel.SetBinding(Label.TextProperty, new Binding("IsEnabled", stringFormat: "IsEnabled: {0}", source: _refreshView)); + + Content = new StackLayout() + { + Children = + { + isRefreshingLabel, + commandEnabledLabel, + new Button() + { + Text = "Toggle Refresh", + Command = new Command(() => + { + _refreshView.IsRefreshing = !_refreshView.IsRefreshing; + }) + }, + new Button() + { + Text = "Toggle Can Execute", + Command = new Command(() => + { + canExecute = !canExecute; + _refreshCommand.ChangeCanExecute(); + }), + AutomationId = "ToggleCanExecute" + }, + new Button() + { + Text = "Toggle Can Execute Parameter", + Command = new Command(() => + { + _refreshView.CommandParameter = !((bool)_refreshView.CommandParameter); + _refreshCommand.ChangeCanExecute(); + }), + AutomationId = "ToggleCanExecuteParameter" + }, + new Button() + { + Text = "Toggle Command Being Set", + Command = new Command(() => + { + if(_refreshView.Command != null) + _refreshView.Command = null; + else + _refreshView.Command = _refreshCommand; + }), + AutomationId = "ToggleCommandBeingSet" + }, + _refreshView + } + }; + } +#if UITEST + [Test] + public void IsRefreshingAndCommandTest() + { + RunningApp.Tap(q => q.Button("Toggle Refresh")); + RunningApp.WaitForElement(q => q.Marked("IsRefreshing: True")); + RunningApp.Screenshot("Refreshing"); + RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); + RunningApp.Screenshot("Refreshed"); + } + + [Test] + public void IsRefreshingAndCommandTest_SwipeDown() + { + RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); + + TriggerRefresh(); + RunningApp.WaitForElement(q => q.Marked("IsRefreshing: True")); + RunningApp.Screenshot("Refreshing"); + RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); + RunningApp.Screenshot("Refreshed"); + } + + [Test] + public void RefreshDisablesWithCommand() + { + RunningApp.WaitForElement("IsRefreshing: False"); + RunningApp.Tap("ToggleCanExecute"); + RunningApp.WaitForElement("IsEnabled: False"); + TriggerRefresh(); + + var results = RunningApp.Query("IsRefreshing: True"); + Assert.AreEqual(0, results.Length); + results = RunningApp.Query("IsRefreshing: True"); + Assert.AreEqual(0, results.Length); + } + + void TriggerRefresh() + { + var container = RunningApp.WaitForElement("LayoutContainer")[0]; + RunningApp.Pan(new Drag(container.Rect, Drag.Direction.TopToBottom, Drag.DragLength.Medium)); + + } +#endif + } +} \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index 471fe5b59d4..0a290bc519c 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -1749,6 +1749,7 @@ + diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 8593fc0e827..87d84c4648c 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -46,7 +46,23 @@ public void Load() { var child = ElementController.LogicalChildren[i] as VisualElement; if (child != null) + { OnChildAdded(child); + + if (!CompressedLayout.GetIsHeadless(child)) + { + var childRenderer = Platform.GetRenderer(child); + + if (childRenderer == null) + continue; + + var nativeControl = childRenderer.NativeView; +#if __MOBILE__ + Renderer.NativeView.BringSubviewToFront(nativeControl); +#endif + nativeControl.Layer.ZPosition = i * 1000; + } + } } } @@ -120,8 +136,6 @@ protected virtual void OnChildAdded(VisualElement view) if (Renderer.ViewController != null && viewRenderer.ViewController != null) Renderer.ViewController.AddChildViewController(viewRenderer.ViewController); - - EnsureChildrenOrder(); } Performance.Stop(reference); From c70f7837516e5bcd82ce71a95384748c6303a46a Mon Sep 17 00:00:00 2001 From: Shanmukha Ranganath Date: Tue, 29 Oct 2019 17:43:04 +0530 Subject: [PATCH 2/6] Abstract OrderElement --- .../VisualElementPackager.cs | 37 ++++++++----------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 87d84c4648c..6ed04bd48ac 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -48,20 +48,7 @@ public void Load() if (child != null) { OnChildAdded(child); - - if (!CompressedLayout.GetIsHeadless(child)) - { - var childRenderer = Platform.GetRenderer(child); - - if (childRenderer == null) - continue; - - var nativeControl = childRenderer.NativeView; -#if __MOBILE__ - Renderer.NativeView.BringSubviewToFront(nativeControl); -#endif - nativeControl.Layer.ZPosition = i * 1000; - } + OrderElement(child, i); } } } @@ -165,17 +152,25 @@ void EnsureChildrenOrder() var child = ElementController.LogicalChildren[z] as VisualElement; if (child == null) continue; - var childRenderer = Platform.GetRenderer(child); + OrderElement(child, z); + } + } - if (childRenderer == null) - continue; + void OrderElement(VisualElement element, int order) + { + if (CompressedLayout.GetIsHeadless(element)) + return; - var nativeControl = childRenderer.NativeView; + var childRenderer = Platform.GetRenderer(element); + + if (childRenderer == null) + return; + + var nativeControl = childRenderer.NativeView; #if __MOBILE__ - Renderer.NativeView.BringSubviewToFront(nativeControl); + Renderer.NativeView.BringSubviewToFront(nativeControl); #endif - nativeControl.Layer.ZPosition = z * 1000; - } + nativeControl.Layer.ZPosition = order * 1000; } void OnChildAdded(object sender, ElementEventArgs e) From 3c8d1cc0018b062705599af067ac5cf433a5f4a5 Mon Sep 17 00:00:00 2001 From: Shanmukha Ranganath Date: Tue, 29 Oct 2019 20:32:58 +0530 Subject: [PATCH 3/6] Re-order on runtime child addition and Tests --- .../Issue8129.cs | 164 ++++-------------- .../VisualElementPackager.cs | 6 + 2 files changed, 44 insertions(+), 126 deletions(-) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs index 03ae0e6ad6c..72acedf371e 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs @@ -19,158 +19,70 @@ namespace Xamarin.Forms.Controls.Issues [Issue(IssueTracker.Github, 8129, "[Bug] Adding children to iOS VisualElementPackager has O(N^2) performance and thrashes the native layer", PlatformAffected.iOS)] public class Issue8129 : TestContentPage { - RefreshView _refreshView; - Command _refreshCommand; - public Issue8129() { } protected override void Init() { - Title = "Refresh View with too many elements (2000) Tests"; - var scrollViewContent = new StackLayout(); - - Enumerable - .Range(0, 2000) - .Select(_ => new Label() { HeightRequest = 200, Text = $"Pull me down to refresh me - {_}" }) - .ForEach(x => scrollViewContent.Children.Add(x)); - - - bool canExecute = true; - _refreshCommand = new Command(async (parameter) => - { - if (!_refreshView.IsRefreshing) - { - throw new Exception("IsRefreshing should be true when command executes"); - } + Title = "Page with too many elements (2000) Tests"; - if (parameter != null && !(bool)parameter) - { - throw new Exception("Refresh command incorrectly firing with disabled parameter"); - } + var grid = new Grid(); + var stackLayout = new StackLayout(); - await Task.Delay(2000); - _refreshView.IsRefreshing = false; - }, (object parameter) => + var addChildrenCommand = new Command((parameter) => { - return parameter != null && canExecute && (bool)parameter; + Enumerable + .Range(0, 2000) + .Select(_ => new Label() { HeightRequest = 200, Text = $"I am Label #{_}" }) + .ForEach(x => stackLayout.Children.Add(x)); }); - _refreshView = new RefreshView() + var addChildrenButton = new Button { - Content = new ScrollView() - { - HeightRequest = 2000, - BackgroundColor = Color.Green, - Content = scrollViewContent, - AutomationId = "LayoutContainer" - }, - Command = _refreshCommand, - CommandParameter = true + Text = "Add 2000 Labels", + Command = addChildrenCommand }; - var isRefreshingLabel = new Label(); - - var label = new Label { BindingContext = _refreshView }; - isRefreshingLabel.SetBinding(Label.TextProperty, new Binding("IsRefreshing", stringFormat: "IsRefreshing: {0}", source: _refreshView)); - - var commandEnabledLabel = new Label { BindingContext = _refreshView }; - commandEnabledLabel.SetBinding(Label.TextProperty, new Binding("IsEnabled", stringFormat: "IsEnabled: {0}", source: _refreshView)); - - Content = new StackLayout() + var addZChildrenButton = new Button { - Children = + Text = "Add BoxView on Top", + Command = new Command((parameter) => { - isRefreshingLabel, - commandEnabledLabel, - new Button() - { - Text = "Toggle Refresh", - Command = new Command(() => - { - _refreshView.IsRefreshing = !_refreshView.IsRefreshing; - }) - }, - new Button() + grid.AddChild(new BoxView { - Text = "Toggle Can Execute", - Command = new Command(() => - { - canExecute = !canExecute; - _refreshCommand.ChangeCanExecute(); - }), - AutomationId = "ToggleCanExecute" - }, - new Button() - { - Text = "Toggle Can Execute Parameter", - Command = new Command(() => - { - _refreshView.CommandParameter = !((bool)_refreshView.CommandParameter); - _refreshCommand.ChangeCanExecute(); - }), - AutomationId = "ToggleCanExecuteParameter" - }, - new Button() - { - Text = "Toggle Command Being Set", - Command = new Command(() => - { - if(_refreshView.Command != null) - _refreshView.Command = null; - else - _refreshView.Command = _refreshCommand; - }), - AutomationId = "ToggleCommandBeingSet" - }, - _refreshView - } + HeightRequest = 300, + WidthRequest = 300, + BackgroundColor = Color.Green + }, 0, 0, 1, 3); + }) }; + grid.AddChild(addChildrenButton, 0, 0); + grid.AddChild(addZChildrenButton, 0, 1); + grid.AddChild(stackLayout, 0, 2); + + Content = grid; } #if UITEST [Test] - public void IsRefreshingAndCommandTest() + public void AddTooManyContentsTest() { - RunningApp.Tap(q => q.Button("Toggle Refresh")); - RunningApp.WaitForElement(q => q.Marked("IsRefreshing: True")); - RunningApp.Screenshot("Refreshing"); - RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); - RunningApp.Screenshot("Refreshed"); + RunningApp.WaitForElement(q => q.Button("Add 2000 Labels")); + RunningApp.Screenshot("Before adding 2000 Labels"); + RunningApp.Tap(q => q.Button("Add 2000 Labels")); + RunningApp.WaitForElement(q => q.Marked("I am Label #1")); + RunningApp.Screenshot("After adding 2000 Labels"); } [Test] - public void IsRefreshingAndCommandTest_SwipeDown() + public void ZIndexAfterAddingContentsTest() { - RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); - - TriggerRefresh(); - RunningApp.WaitForElement(q => q.Marked("IsRefreshing: True")); - RunningApp.Screenshot("Refreshing"); - RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); - RunningApp.Screenshot("Refreshed"); - } - - [Test] - public void RefreshDisablesWithCommand() - { - RunningApp.WaitForElement("IsRefreshing: False"); - RunningApp.Tap("ToggleCanExecute"); - RunningApp.WaitForElement("IsEnabled: False"); - TriggerRefresh(); - - var results = RunningApp.Query("IsRefreshing: True"); - Assert.AreEqual(0, results.Length); - results = RunningApp.Query("IsRefreshing: True"); - Assert.AreEqual(0, results.Length); - } - - void TriggerRefresh() - { - var container = RunningApp.WaitForElement("LayoutContainer")[0]; - RunningApp.Pan(new Drag(container.Rect, Drag.Direction.TopToBottom, Drag.DragLength.Medium)); - + RunningApp.WaitForElement(q => q.Button("Add BoxView on Top")); + RunningApp.Screenshot("Before adding BoxView on Top"); + RunningApp.Tap(q => q.Button("Add BoxView on Top")); + RunningApp.WaitForNoElement(q => q.Button("Add BoxView on Top")); + RunningApp.Screenshot("After adding BoxView on Top"); } #endif } -} \ No newline at end of file +} diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 6ed04bd48ac..66ad57296bf 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using Xamarin.Forms.Internals; #if __MOBILE__ @@ -158,6 +159,7 @@ void EnsureChildrenOrder() void OrderElement(VisualElement element, int order) { + Performance.Start(out string reference); if (CompressedLayout.GetIsHeadless(element)) return; @@ -171,13 +173,17 @@ void OrderElement(VisualElement element, int order) Renderer.NativeView.BringSubviewToFront(nativeControl); #endif nativeControl.Layer.ZPosition = order * 1000; + Performance.Stop(reference); } void OnChildAdded(object sender, ElementEventArgs e) { var view = e.Element as VisualElement; if (view != null) + { OnChildAdded(view); + OrderElement(view, ElementController.LogicalChildren.IndexOf(view)); + } } void OnChildRemoved(object sender, ElementEventArgs e) From dbb71a40d61e547a64ffe5bfd9c321ffdbe4c53e Mon Sep 17 00:00:00 2001 From: Shanmukha Ranganath Date: Sat, 2 Nov 2019 01:28:07 +0530 Subject: [PATCH 4/6] Runtime addition ensures Z Index --- .../Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs | 7 ------- Xamarin.Forms.Platform.iOS/VisualElementPackager.cs | 10 +++++++--- 2 files changed, 7 insertions(+), 10 deletions(-) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs index 72acedf371e..cafd47bf2d6 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue8129.cs @@ -12,17 +12,10 @@ namespace Xamarin.Forms.Controls.Issues { -#if UITEST - [Category(UITestCategories.RefreshView)] -#endif [Preserve(AllMembers = true)] [Issue(IssueTracker.Github, 8129, "[Bug] Adding children to iOS VisualElementPackager has O(N^2) performance and thrashes the native layer", PlatformAffected.iOS)] public class Issue8129 : TestContentPage { - public Issue8129() - { - } - protected override void Init() { Title = "Page with too many elements (2000) Tests"; diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 66ad57296bf..9d17cdab531 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -143,12 +143,16 @@ protected virtual void OnChildRemoved(VisualElement view) viewRenderer.Dispose(); } - void EnsureChildrenOrder() + void EnsureChildrenOrder(VisualElement element = null) { if (ElementController.LogicalChildren.Count == 0) return; - for (var z = 0; z < ElementController.LogicalChildren.Count; z++) + var effectedChildIndex = 0; + if (element != null) + effectedChildIndex = ElementController.LogicalChildren.IndexOf(element); + + for (var z = effectedChildIndex; z < ElementController.LogicalChildren.Count; z++) { var child = ElementController.LogicalChildren[z] as VisualElement; if (child == null) @@ -182,7 +186,7 @@ void OnChildAdded(object sender, ElementEventArgs e) if (view != null) { OnChildAdded(view); - OrderElement(view, ElementController.LogicalChildren.IndexOf(view)); + EnsureChildrenOrder(view); } } From 4e1c18aa121b62ac6e6f48a26a7fced3021447fa Mon Sep 17 00:00:00 2001 From: Gerald Versluis Date: Mon, 30 Aug 2021 14:18:32 +0200 Subject: [PATCH 5/6] Stop performance measurement on return --- Xamarin.Forms.Platform.iOS/VisualElementPackager.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 9d17cdab531..78e4d4e0c17 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -170,7 +170,11 @@ void OrderElement(VisualElement element, int order) var childRenderer = Platform.GetRenderer(element); if (childRenderer == null) + { + Performance.Stop(reference); + return; + } var nativeControl = childRenderer.NativeView; #if __MOBILE__ From d1c95f6778e2e3cb4914df1dbdefddb59361f07c Mon Sep 17 00:00:00 2001 From: Gerald Versluis Date: Mon, 30 Aug 2021 14:24:49 +0200 Subject: [PATCH 6/6] Update VisualElementPackager.cs --- Xamarin.Forms.Platform.iOS/VisualElementPackager.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs index 78e4d4e0c17..fee37eec634 100644 --- a/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs +++ b/Xamarin.Forms.Platform.iOS/VisualElementPackager.cs @@ -165,7 +165,11 @@ void OrderElement(VisualElement element, int order) { Performance.Start(out string reference); if (CompressedLayout.GetIsHeadless(element)) + { + Performance.Stop(reference); + return; + } var childRenderer = Platform.GetRenderer(element);