From: Shane Neuville Date: Thu, 17 Oct 2019 06:44:25 +0000 (-0600) Subject: Make the Refresh View Respect Command CanExecute and add Refreshing event (#7866) X-Git-Tag: accepted/tizen/5.5/unified/20200421.150457~140^2~5 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=f9114b1306f2896cce07d358725f63ce6ab8cac5;p=platform%2Fcore%2Fcsapi%2Fxsf.git Make the Refresh View Respect Command CanExecute and add Refreshing event (#7866) * Make Refresh View respect Command CanExecute * - force isrefreshing false when isenabled fasle - Add IsRefreshing Event * - shift command execution up to xplat code * - fix ui test * - cleanup * - add additional check * Update Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml.cs * - fix flag --- diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml new file mode 100644 index 0000000..21d7e43 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml @@ -0,0 +1,35 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml.cs new file mode 100644 index 0000000..2330659 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Issue7803.xaml.cs @@ -0,0 +1,170 @@ +using System.Collections.ObjectModel; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using Xamarin.Forms.Xaml; +using System.Collections.Generic; +using System.Threading.Tasks; +using System.ComponentModel; +using System; + +#if UITEST +using Xamarin.UITest; +using Xamarin.UITest.Queries; +using NUnit.Framework; +using Xamarin.Forms.Core.UITests; +using System.Linq; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ +#if UITEST + [NUnit.Framework.Category(UITestCategories.CollectionView)] +#endif +#if APP + [XamlCompilation(XamlCompilationOptions.Compile)] +#endif + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Github, 7803, "[Bug] CarouselView/RefreshView pull to refresh command firing twice on a single pull", PlatformAffected.All)] + public partial class Issue7803 : TestContentPage + { +#if APP + public Issue7803() + { + + InitializeComponent(); + + BindingContext = new ViewModel7803(); + } +#endif + + protected override void Init() + { + + } + +#if UITEST + [Test] + public void DelayedIsRefreshingAndCommandTest_SwipeDown() + { + var collectionView = RunningApp.WaitForElement(q => q.Marked("CollectionView7803"))[0]; + + RunningApp.Pan(new Drag(collectionView.Rect, Drag.Direction.TopToBottom, Drag.DragLength.Medium)); + + RunningApp.WaitForElement(q => q.Marked("Count: 20")); + RunningApp.WaitForNoElement(q => q.Marked("Count: 30")); + + AppResult[] lastCellResults = null; + + RunningApp.QueryUntilPresent(() => + { + RunningApp.DragCoordinates(collectionView.Rect.CenterX, collectionView.Rect.Y + collectionView.Rect.Height - 50, collectionView.Rect.CenterX, collectionView.Rect.Y + 5); + + lastCellResults = RunningApp.Query("19"); + + return lastCellResults; + }, 10, 1); + + Assert.IsTrue(lastCellResults?.Any() ?? false); + } +#endif + } + + [Preserve(AllMembers = true)] + public class ViewModel7803 : INotifyPropertyChanged + { + public ObservableCollection Items { get; set; } = new ObservableCollection(); + + private bool _isRefreshing; + + public bool IsRefreshing + { + get + { + return _isRefreshing; + } + set + { + _isRefreshing = value; + + OnPropertyChanged("IsRefreshing"); + } + } + + private string _text; + + public string Text + { + get + { + return _text; + } + set + { + _text = value; + + OnPropertyChanged("Text"); + } + } + + public Command RefreshCommand { get; set; } + + public ViewModel7803() + { + PopulateItems(); + + RefreshCommand = new Command(async () => + { + IsRefreshing = true; + + await Task.Delay(2000); + PopulateItems(); + + IsRefreshing = false; + }); + } + + void PopulateItems() + { + var count = Items.Count; + + for (var i = count; i < count + 10; i++) + Items.Add(new Model7803() { Position = i }); + + Text = "Count: " + Items.Count; + } + + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string name) + { + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name)); + } + } + + [Preserve(AllMembers = true)] + public class Model7803 : INotifyPropertyChanged + { + private int _position; + + public int Position + { + get + { + return _position; + } + set + { + _position = value; + + OnPropertyChanged("Position"); + } + } + + public event PropertyChangedEventHandler PropertyChanged; + + protected void OnPropertyChanged(string name) + { + PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name)); + } + } +} diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/RefreshViewTests.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/RefreshViewTests.cs index 789d361..557d453 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/RefreshViewTests.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/RefreshViewTests.cs @@ -2,6 +2,7 @@ using System.Threading.Tasks; using Xamarin.Forms.CustomAttributes; using Xamarin.Forms.Internals; +using System; #if UITEST using Xamarin.Forms.Core.UITests; @@ -19,6 +20,8 @@ namespace Xamarin.Forms.Controls.Issues public class RefreshViewTests : TestContentPage { RefreshView _refreshView; + Command _refreshCommand; + public RefreshViewTests() { } @@ -26,14 +29,34 @@ namespace Xamarin.Forms.Controls.Issues protected override void Init() { Title = "Refresh View Tests"; - var scrollViewContent = - new StackLayout() - { - }; + var scrollViewContent = new StackLayout(); - Enumerable.Range(0, 10).Select(_ => new Label() { HeightRequest = 200, Text = "Pull me down to refresh me" }) + Enumerable + .Range(0, 10) + .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() @@ -43,11 +66,8 @@ namespace Xamarin.Forms.Controls.Issues Content = scrollViewContent, AutomationId = "LayoutContainer" }, - Command = new Command(async () => - { - await Task.Delay(2000); - _refreshView.IsRefreshing = false; - }) + Command = _refreshCommand, + CommandParameter = true }; var isRefreshingLabel = new Label(); @@ -55,11 +75,15 @@ namespace Xamarin.Forms.Controls.Issues 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", @@ -68,6 +92,38 @@ namespace Xamarin.Forms.Controls.Issues _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 } }; @@ -88,15 +144,33 @@ namespace Xamarin.Forms.Controls.Issues { RunningApp.WaitForElement(q => q.Marked("IsRefreshing: False")); - var container = RunningApp.WaitForElement("LayoutContainer")[0]; - - RunningApp.Pan(new Drag(container.Rect, Drag.Direction.TopToBottom, Drag.DragLength.Medium)); - + 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 9de27c1..2676835 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 @@ -84,6 +84,9 @@ Code + + Issue7803.xaml + @@ -1567,4 +1570,10 @@ MSBuild:Compile + + + Designer + MSBuild:UpdateDesignTimeXaml + + \ No newline at end of file diff --git a/Xamarin.Forms.Core.UnitTests/RefreshViewTests.cs b/Xamarin.Forms.Core.UnitTests/RefreshViewTests.cs new file mode 100644 index 0000000..a3b3e59 --- /dev/null +++ b/Xamarin.Forms.Core.UnitTests/RefreshViewTests.cs @@ -0,0 +1,157 @@ +using System; +using System.Collections; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using System.Windows.Input; +using NUnit.Framework; +using Xamarin.Forms.Internals; + +namespace Xamarin.Forms.Core.UnitTests +{ + [TestFixture] + public class RefreshViewTests : BaseTestFixture + { + [TearDown] + public override void TearDown() + { + base.TearDown (); + Device.PlatformServices = null; + Device.Info = null; + } + + [SetUp] + public override void Setup () + { + base.Setup (); + Device.PlatformServices = new MockPlatformServices (); + Device.Info = new TestDeviceInfo (); + } + + [Test] + public void StartsEnabled() + { + RefreshView refreshView = new RefreshView(); + Assert.IsTrue(refreshView.IsEnabled); + } + + [Test] + public void CanExecuteDisablesRefreshView() + { + RefreshView refreshView = new RefreshView(); + refreshView.Command = new Command(() => { }, () => false); + Assert.IsFalse(refreshView.IsEnabled); + } + + [Test] + public void CanExecuteEnablesRefreshView() + { + RefreshView refreshView = new RefreshView(); + refreshView.Command = new Command(() => { }, () => true); + Assert.IsTrue(refreshView.IsEnabled); + } + + [Test] + public void CanExecuteChangesEnabled() + { + RefreshView refreshView = new RefreshView(); + + bool canExecute = true; + var command = new Command(() => { }, () => canExecute); + refreshView.Command = command; + + canExecute = false; + command.ChangeCanExecute(); + Assert.IsFalse(refreshView.IsEnabled); + + + canExecute = true; + command.ChangeCanExecute(); + Assert.IsTrue(refreshView.IsEnabled); + } + + [Test] + public void CommandPropertyChangesEnabled() + { + RefreshView refreshView = new RefreshView(); + + bool canExecute = true; + var command = new Command((p) => { }, (p) => p != null && (bool)p); + refreshView.CommandParameter = true; + refreshView.Command = command; + + Assert.IsTrue(refreshView.IsEnabled); + refreshView.CommandParameter = false; + Assert.IsFalse(refreshView.IsEnabled); + refreshView.CommandParameter = true; + Assert.IsTrue(refreshView.IsEnabled); + } + + [Test] + public void RemovedCommandEnablesRefreshView() + { + RefreshView refreshView = new RefreshView(); + + bool canExecute = true; + var command = new Command(() => { }, () => false); + refreshView.Command = command; + Assert.IsFalse(refreshView.IsEnabled); + refreshView.Command = null; + Assert.IsTrue(refreshView.IsEnabled); + refreshView.Command = command; + Assert.IsFalse(refreshView.IsEnabled); + } + + [Test] + public void IsRefreshingStaysFalseWithDisabledCommand() + { + RefreshView refreshView = new RefreshView(); + + bool canExecute = true; + refreshView.Command = new Command(() => { }, () => false); + refreshView.IsRefreshing = true; + Assert.IsFalse(refreshView.IsRefreshing); + } + + [Test] + public void IsRefreshingSettableToTrue() + { + RefreshView refreshView = new RefreshView(); + Assert.IsFalse(refreshView.IsRefreshing); + + refreshView.IsRefreshing = true; + Assert.IsTrue(refreshView.IsRefreshing); + } + + [Test] + public void IsRefreshingStaysFalseWithDisabledRefreshView() + { + RefreshView refreshView = new RefreshView(); + refreshView.IsEnabled = false; + refreshView.IsRefreshing = true; + Assert.IsFalse(refreshView.IsRefreshing); + } + + [Test] + public void IsRefreshingTogglesFalseWhenIsEnabledSetToFalse() + { + RefreshView refreshView = new RefreshView(); + refreshView.IsRefreshing = true; + refreshView.IsEnabled = false; + Assert.IsFalse(refreshView.IsRefreshing); + } + + [Test] + public void IsRefreshingEventFires() + { + RefreshView refreshView = new RefreshView(); + bool eventFired = false; + refreshView.Refreshing += (_, __) => eventFired = true; + Assert.IsFalse(eventFired); + refreshView.IsRefreshing = true; + Assert.IsTrue(eventFired); + } + } +} diff --git a/Xamarin.Forms.Core.UnitTests/Xamarin.Forms.Core.UnitTests.csproj b/Xamarin.Forms.Core.UnitTests/Xamarin.Forms.Core.UnitTests.csproj index ff544f6..b2438b2 100644 --- a/Xamarin.Forms.Core.UnitTests/Xamarin.Forms.Core.UnitTests.csproj +++ b/Xamarin.Forms.Core.UnitTests/Xamarin.Forms.Core.UnitTests.csproj @@ -76,6 +76,7 @@ + diff --git a/Xamarin.Forms.Core/RefreshView.cs b/Xamarin.Forms.Core/RefreshView.cs index 1216cdc..0550c81 100644 --- a/Xamarin.Forms.Core/RefreshView.cs +++ b/Xamarin.Forms.Core/RefreshView.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.CompilerServices; using System.Windows.Input; using Xamarin.Forms.Platform; @@ -9,6 +10,7 @@ namespace Xamarin.Forms public class RefreshView : ContentView, IElementConfiguration { readonly Lazy> _platformConfigurationRegistry; + public event EventHandler Refreshing; public RefreshView() { @@ -20,7 +22,41 @@ namespace Xamarin.Forms } public static readonly BindableProperty IsRefreshingProperty = - BindableProperty.Create(nameof(IsRefreshing), typeof(bool), typeof(RefreshView), false, BindingMode.TwoWay); + BindableProperty.Create(nameof(IsRefreshing), typeof(bool), typeof(RefreshView), false, BindingMode.TwoWay, coerceValue: OnIsRefreshingPropertyCoerced, propertyChanged: OnIsRefreshingPropertyChanged); + + static void OnIsRefreshingPropertyChanged(BindableObject bindable, object oldValue, object newValue) + { + bool value = (bool)newValue; + + if (!value) + return; + + var refreshView = ((RefreshView)bindable); + refreshView.Refreshing?.Invoke(bindable, EventArgs.Empty); + if (refreshView.Command != null) + refreshView.Command.Execute(refreshView.CommandParameter); + } + + static object OnIsRefreshingPropertyCoerced(BindableObject bindable, object value) + { + RefreshView view = (RefreshView)bindable; + bool newValue = (bool)value; + + // IsRefreshing can always be toggled to false + if (!newValue) + return value; + + if (!view.IsEnabled) + return false; + + if (view.Command == null) + return value; + + if (!view.Command.CanExecute(view.CommandParameter)) + return false; + + return value; + } public bool IsRefreshing { @@ -29,7 +65,19 @@ namespace Xamarin.Forms } public static readonly BindableProperty CommandProperty = - BindableProperty.Create(nameof(Command), typeof(ICommand), typeof(RefreshView)); + BindableProperty.Create(nameof(Command), typeof(ICommand), typeof(RefreshView), propertyChanged: OnCommandChanged); + + static void OnCommandChanged(BindableObject bindable, object oldValue, object newValue) + { + RefreshView refreshView = (RefreshView)bindable; + if (oldValue is ICommand oldCommand) + oldCommand.CanExecuteChanged -= refreshView.RefreshCommandCanExecuteChanged; + + if (newValue is ICommand newCommand) + newCommand.CanExecuteChanged += refreshView.RefreshCommandCanExecuteChanged; + + refreshView.RefreshCommandCanExecuteChanged(bindable, EventArgs.Empty); + } public ICommand Command { @@ -42,7 +90,7 @@ namespace Xamarin.Forms typeof(object), typeof(RefreshView), null, - propertyChanged: (bindable, oldvalue, newvalue) => ((RefreshView)bindable).RefreshCommandCanExecuteChanged(bindable, EventArgs.Empty)); + propertyChanged: (bindable, oldvalue, newvalue) => ((RefreshView)(bindable)).RefreshCommandCanExecuteChanged(((RefreshView)(bindable)).Command, EventArgs.Empty)); public object CommandParameter { @@ -53,7 +101,9 @@ namespace Xamarin.Forms void RefreshCommandCanExecuteChanged(object sender, EventArgs eventArgs) { if (Command != null) - IsEnabled = Command.CanExecute(CommandParameter); + SetValueCore(IsEnabledProperty, Command.CanExecute(CommandParameter)); + else + SetValueCore(IsEnabledProperty, true); } public static readonly BindableProperty RefreshColorProperty = @@ -77,5 +127,13 @@ namespace Xamarin.Forms { return _platformConfigurationRegistry.Value.On(); } + + protected override void OnPropertyChanged([CallerMemberName] string propertyName = null) + { + base.OnPropertyChanged(propertyName); + if (IsEnabledProperty.PropertyName == propertyName) + if (!IsEnabled && IsRefreshing) + IsRefreshing = false; + } } } \ No newline at end of file diff --git a/Xamarin.Forms.Platform.Android/Renderers/RefreshViewRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/RefreshViewRenderer.cs index c88bb4d..88c7af9 100644 --- a/Xamarin.Forms.Platform.Android/Renderers/RefreshViewRenderer.cs +++ b/Xamarin.Forms.Platform.Android/Renderers/RefreshViewRenderer.cs @@ -52,12 +52,9 @@ namespace Xamarin.Forms.Platform.Android _refreshing = value; if (RefreshView != null && RefreshView.IsRefreshing != _refreshing) - RefreshView.IsRefreshing = _refreshing; + RefreshView.SetValueFromRenderer(RefreshView.IsRefreshingProperty, _refreshing); base.Refreshing = _refreshing; - - if (base.Refreshing && Element is RefreshView refreshView && refreshView.Command != null && refreshView.Command.CanExecute(refreshView?.CommandParameter)) - refreshView.Command.Execute(refreshView?.CommandParameter); } } diff --git a/Xamarin.Forms.Platform.UAP/RefreshViewRenderer.cs b/Xamarin.Forms.Platform.UAP/RefreshViewRenderer.cs index 621809a..343dbcb 100644 --- a/Xamarin.Forms.Platform.UAP/RefreshViewRenderer.cs +++ b/Xamarin.Forms.Platform.UAP/RefreshViewRenderer.cs @@ -186,10 +186,6 @@ namespace Xamarin.Forms.Platform.UWP CompleteRefresh(); _refreshCompletionDeferral = args.GetDeferral(); Element.SetValueFromRenderer(RefreshView.IsRefreshingProperty, true); - if (Element?.Command?.CanExecute(Element?.CommandParameter) ?? false) - { - Element.Command.Execute(Element?.CommandParameter); - } } } } \ No newline at end of file diff --git a/Xamarin.Forms.Platform.iOS/Renderers/RefreshViewRenderer.cs b/Xamarin.Forms.Platform.iOS/Renderers/RefreshViewRenderer.cs index 6ffda38..b5c6489 100644 --- a/Xamarin.Forms.Platform.iOS/Renderers/RefreshViewRenderer.cs +++ b/Xamarin.Forms.Platform.iOS/Renderers/RefreshViewRenderer.cs @@ -23,19 +23,18 @@ namespace Xamarin.Forms.Platform.iOS _isRefreshing = value; if (Element != null && Element.IsRefreshing != _isRefreshing) - Element.IsRefreshing = _isRefreshing; + Element.SetValueFromRenderer(RefreshView.IsRefreshingProperty, _isRefreshing); - if (_isRefreshing) + + if (_isRefreshing != _refreshControl.Refreshing) { - _refreshControl.BeginRefreshing(); + if (_isRefreshing) + _refreshControl.BeginRefreshing(); + else + _refreshControl.EndRefreshing(); - if (Element is RefreshView refreshView && refreshView.Command != null && refreshView.Command.CanExecute(refreshView?.CommandParameter)) - refreshView.Command.Execute(refreshView?.CommandParameter); + TryOffsetRefresh(this, IsRefreshing); } - else - _refreshControl.EndRefreshing(); - - TryOffsetRefresh(this, IsRefreshing); } }