-
Notifications
You must be signed in to change notification settings - Fork 2k
[iOS/Android] Fix SwipeItem.IsVisible not refreshing native swipe items when binding changes #35217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
cb915d0
783bf94
90e1a7d
082e5e2
8e55425
6953ef0
2ecdc6a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,7 +14,7 @@ public partial class SwipeItem : MenuItem, Controls.ISwipeItem, Maui.ISwipeItemM | |
| public static readonly BindableProperty BackgroundColorProperty = BindableProperty.Create(nameof(BackgroundColor), typeof(Color), typeof(SwipeItem), null); | ||
|
|
||
| /// <summary>Bindable property for <see cref="IsVisible"/>.</summary> | ||
| public static readonly BindableProperty IsVisibleProperty = BindableProperty.Create(nameof(IsVisible), typeof(bool), typeof(SwipeItem), true); | ||
| public static readonly BindableProperty IsVisibleProperty = BindableProperty.Create(nameof(IsVisible), typeof(bool), typeof(SwipeItem), true, propertyChanged: OnIsVisibleChanged); | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the background color of the swipe item. This is a bindable property. | ||
|
|
@@ -40,6 +40,12 @@ public bool IsVisible | |
|
|
||
| Visibility ISwipeItemMenuItem.Visibility => this.IsVisible ? Visibility.Visible : Visibility.Collapsed; | ||
|
|
||
| static void OnIsVisibleChanged(BindableObject bindable, object oldValue, object newValue) | ||
| { | ||
| var swipeItem = (SwipeItem)bindable; | ||
| swipeItem.Handler?.UpdateValue(nameof(ISwipeItemMenuItem.Visibility)); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Defensive cast — |
||
| } | ||
|
|
||
| void Maui.ISwipeItem.OnInvoked() | ||
| { | ||
| if (Command != null && Command.CanExecute(CommandParameter)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,108 @@ | ||
| using System.ComponentModel; | ||
| using System.Runtime.CompilerServices; | ||
|
|
||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 34832, "SwipeItem.IsVisible doesn't properly refresh native swipe items when binding value changes dynamically", PlatformAffected.Android | PlatformAffected.iOS)] | ||
| public class Issue34832 : ContentPage | ||
| { | ||
| readonly Issue34832ViewModel _viewModel = new() { IsDeleteVisible = false }; | ||
| SwipeView _swipeView; | ||
|
|
||
| public Issue34832() | ||
| { | ||
| BindingContext = _viewModel; | ||
|
|
||
| SwipeItem deleteSwipeItem = new SwipeItem | ||
| { | ||
| Text = "Delete", | ||
| BackgroundColor = Colors.Green, | ||
| AutomationId = "DeleteSwipeItem" | ||
| }; | ||
| deleteSwipeItem.SetBinding(SwipeItem.IsVisibleProperty, new Binding(nameof(Issue34832ViewModel.IsDeleteVisible))); | ||
|
|
||
| SwipeItem archiveSwipeItem = new SwipeItem | ||
| { | ||
| Text = "Archive", | ||
| BackgroundColor = Colors.Blue, | ||
| AutomationId = "ArchiveSwipeItem" | ||
| }; | ||
|
|
||
| _swipeView = new SwipeView | ||
| { | ||
| AutomationId = "TestSwipeView", | ||
| HeightRequest = 60, | ||
| LeftItems = new SwipeItems { deleteSwipeItem, archiveSwipeItem }, | ||
| Content = new Grid | ||
| { | ||
| BackgroundColor = Colors.LightGray, | ||
| Children = | ||
| { | ||
| new Label | ||
| { | ||
| Text = "Swipe left to reveal items", | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| VerticalOptions = LayoutOptions.Center | ||
| } | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| Button toggleButton = new Button | ||
| { | ||
| Text = "Toggle Delete Visibility", | ||
| AutomationId = "ToggleVisibilityButton" | ||
| }; | ||
| toggleButton.Clicked += (s, e) => _viewModel.IsDeleteVisible = !_viewModel.IsDeleteVisible; | ||
|
|
||
| Button openSwipeButton = new Button | ||
| { | ||
| Text = "Open Swipe", | ||
| AutomationId = "OpenSwipeButton" | ||
| }; | ||
| openSwipeButton.Clicked += (s, e) => _swipeView?.Open(OpenSwipeItem.LeftItems); | ||
|
|
||
| Button resetButton = new Button | ||
| { | ||
| Text = "Reset", | ||
| AutomationId = "ResetButton" | ||
| }; | ||
| resetButton.Clicked += (s, e) => _viewModel.IsDeleteVisible = false; | ||
|
|
||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = new Thickness(20), | ||
| Spacing = 20, | ||
| Children = | ||
| { | ||
| _swipeView, | ||
| toggleButton, | ||
| openSwipeButton, | ||
| resetButton, | ||
| } | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| public class Issue34832ViewModel : INotifyPropertyChanged | ||
| { | ||
| bool _isDeleteVisible; | ||
|
|
||
| public bool IsDeleteVisible | ||
| { | ||
| get => _isDeleteVisible; | ||
| set | ||
| { | ||
| if (_isDeleteVisible != value) | ||
| { | ||
| _isDeleteVisible = value; | ||
| OnPropertyChanged(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public event PropertyChangedEventHandler PropertyChanged; | ||
|
|
||
| protected void OnPropertyChanged([CallerMemberName] string name = null) => | ||
| PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(name)); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| #if TEST_FAILS_ON_WINDOWS // Issue Link - https://github.com/dotnet/maui/issues/35216 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Missing Windows coverage justification — entire file is wrapped in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Missing Windows coverage justification — entire file is wrapped in |
||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue34832 : _IssuesUITest | ||
| { | ||
| public override string Issue => "SwipeItem.IsVisible doesn't properly refresh native swipe items when binding value changes dynamically"; | ||
|
|
||
| public Issue34832(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| [Test] | ||
| [Order(1)] | ||
| [Category(UITestCategories.SwipeView)] | ||
| public void SwipeItemInitiallyHiddenBecomesVisibleAfterBindingChanges() | ||
| { | ||
| Exception? exception = null; | ||
| App.WaitForElement("OpenSwipeButton"); | ||
| App.Tap("OpenSwipeButton"); | ||
|
|
||
| VerifyScreenshotOrSetException(ref exception, "SwipeOpen_InitiallyHidden"); | ||
|
|
||
| App.Tap("ToggleVisibilityButton"); | ||
|
|
||
| VerifyScreenshotOrSetException(ref exception, "SwipeOpen_BecomeVisible"); | ||
|
|
||
| App.Tap("TestSwipeView"); | ||
| App.Tap("ResetButton"); | ||
|
|
||
| if (exception is not null) | ||
| { | ||
| throw exception; | ||
| } | ||
| } | ||
|
|
||
| [Test] | ||
| [Order(2)] | ||
| [Category(UITestCategories.SwipeView)] | ||
| public void SwipeItemBecomesHiddenAfterBindingChanges() | ||
| { | ||
| Exception? exception = null; | ||
| App.WaitForElement("ToggleVisibilityButton"); | ||
| App.Tap("ToggleVisibilityButton"); | ||
| App.Tap("OpenSwipeButton"); | ||
|
|
||
| VerifyScreenshotOrSetException(ref exception, "SwipeOpen_DeleteVisible"); | ||
|
|
||
| App.Tap("ToggleVisibilityButton"); | ||
|
|
||
| VerifyScreenshotOrSetException(ref exception, "SwipeOpen_DeleteHidden"); | ||
|
|
||
| App.Tap("TestSwipeView"); | ||
| App.Tap("ResetButton"); | ||
|
|
||
| if (exception is not null) | ||
| { | ||
| throw exception; | ||
| } | ||
| } | ||
| } | ||
| #endif | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -299,7 +299,7 @@ void UpdateSwipeItems() | |
| double swipeItemsWidth; | ||
|
|
||
| if (_swipeDirection == SwipeDirection.Left || _swipeDirection == SwipeDirection.Right) | ||
| swipeItemsWidth = items.Count * SwipeViewExtensions.SwipeItemWidth; | ||
| swipeItemsWidth = items.Count(GetIsVisible) * SwipeViewExtensions.SwipeItemWidth; | ||
| else | ||
| swipeItemsWidth = _contentView.Frame.Width; | ||
|
SyedAbdulAzeemSF4852 marked this conversation as resolved.
|
||
|
|
||
|
|
@@ -312,6 +312,7 @@ void UpdateSwipeItems() | |
| foreach (var item in items) | ||
| { | ||
| UIView swipeItem = item.ToPlatform(Element.Handler.MauiContext); | ||
| swipeItem.Hidden = !GetIsVisible(item); | ||
|
SyedAbdulAzeemSF4852 marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Inconsistency with MapVisibility —
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Inconsistency with MapVisibility — |
||
| _actionView.AddSubview(swipeItem); | ||
| _swipeItems.Add(item, swipeItem); | ||
| } | ||
|
|
@@ -342,6 +343,11 @@ void LayoutSwipeItems(List<UIView> childs) | |
|
|
||
| foreach (var child in childs) | ||
| { | ||
| if (i >= items.Count) | ||
| { | ||
| break; | ||
| } | ||
|
|
||
| if (!child.Hidden) | ||
| { | ||
| var item = items[i]; | ||
|
|
@@ -371,10 +377,10 @@ void LayoutSwipeItems(List<UIView> childs) | |
| UpdateSwipeItemInsets(button); | ||
| } | ||
|
|
||
| i++; | ||
| previousWidth += swipeItemWidth; | ||
| } | ||
|
|
||
| i++; | ||
| _swipeItemsRect.Add(child.Frame); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Stale frame entries in
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [minor] Stale frame entries in |
||
| } | ||
| } | ||
|
|
@@ -626,12 +632,12 @@ void SetFrame() | |
| { | ||
| case SwipeDirection.Left: | ||
| _contentView.Frame = new CGRect(_originalBounds.X + offset, _originalBounds.Y, _originalBounds.Width, _originalBounds.Height); | ||
| actionSize = Element.RightItems.Count * SwipeViewExtensions.SwipeItemWidth; | ||
| actionSize = Element.RightItems.Count(GetIsVisible) * SwipeViewExtensions.SwipeItemWidth; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Performance — LINQ on drag hot path —
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [moderate] Performance — LINQ on drag hot path — |
||
| _actionView.Frame = new CGRect(actionSize + offset, actionBounds.Y, actionBounds.Width, actionBounds.Height); | ||
| break; | ||
| case SwipeDirection.Right: | ||
| _contentView.Frame = new CGRect(_originalBounds.X + offset, _originalBounds.Y, _originalBounds.Width, _originalBounds.Height); | ||
| actionSize = Element.LeftItems.Count * SwipeViewExtensions.SwipeItemWidth; | ||
| actionSize = Element.LeftItems.Count(GetIsVisible) * SwipeViewExtensions.SwipeItemWidth; | ||
| _actionView.Frame = new CGRect(-actionSize + offset, actionBounds.Y, actionBounds.Width, actionBounds.Height); | ||
| break; | ||
| case SwipeDirection.Up: | ||
|
|
@@ -841,12 +847,12 @@ void SwipeToThreshold(bool animated = true) | |
| { | ||
| case SwipeDirection.Left: | ||
| _contentView.Frame = new CGRect(_originalBounds.X - swipeThreshold, _originalBounds.Y, _originalBounds.Width, _originalBounds.Height); | ||
| actionSize = Element.RightItems.Count * SwipeViewExtensions.SwipeItemWidth; | ||
| actionSize = Element.RightItems.Count(GetIsVisible) * SwipeViewExtensions.SwipeItemWidth; | ||
| _actionView.Frame = new CGRect(actionSize - swipeThreshold, actionBounds.Y, actionBounds.Width, actionBounds.Height); | ||
| break; | ||
| case SwipeDirection.Right: | ||
| _contentView.Frame = new CGRect(_originalBounds.X + swipeThreshold, _originalBounds.Y, _originalBounds.Width, _originalBounds.Height); | ||
| actionSize = Element.LeftItems.Count * SwipeViewExtensions.SwipeItemWidth; | ||
| actionSize = Element.LeftItems.Count(GetIsVisible) * SwipeViewExtensions.SwipeItemWidth; | ||
| _actionView.Frame = new CGRect(-actionSize + swipeThreshold, actionBounds.Y, actionBounds.Width, actionBounds.Height); | ||
| break; | ||
| case SwipeDirection.Up: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[minor] Defensive cast —
var swipeItem = (SwipeItem)bindable;will throw ifIsVisiblePropertyis ever attached to a non-SwipeItem target (e.g. via styles applying to a derived type that re-uses the BP). The conventional MAUI pattern isif (bindable is SwipeItem swipeItem) swipeItem.Handler?.UpdateValue(...). Low risk becauseBindableProperty.Createties the property totypeof(SwipeItem), but the safer pattern is preferred inpropertyChangedcallbacks throughout the codebase.