-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS/MacCatalyst] Fix IndicatorView not updating when IndicatorSize is changed to default value #35215
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
[iOS/MacCatalyst] Fix IndicatorView not updating when IndicatorSize is changed to default value #35215
Changes from all commits
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 |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| namespace Maui.Controls.Sample.Issues; | ||
|
|
||
| [Issue(IssueTracker.Github, 35214, "Dynamically changing IndicatorView IndicatorSize to default value does not work", PlatformAffected.iOS | PlatformAffected.macOS)] | ||
| public class Issue35214 : ContentPage | ||
| { | ||
| public Issue35214() | ||
| { | ||
| var carouselItems = new List<string> { "Item 0", "Item 1", "Item 2" }; | ||
|
|
||
| var indicatorView = new IndicatorView | ||
| { | ||
| AutomationId = "TestIndicatorView", | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| IndicatorColor = Colors.Orange, | ||
| SelectedIndicatorColor = Colors.Blue, | ||
| IndicatorSize = 20, | ||
| }; | ||
|
|
||
| var carouselView = new CarouselView | ||
| { | ||
| ItemsSource = carouselItems, | ||
| HeightRequest = 300, | ||
| HorizontalOptions = LayoutOptions.Fill, | ||
| IndicatorView = indicatorView, | ||
| ItemTemplate = new DataTemplate(() => | ||
| { | ||
| var label = new Label | ||
| { | ||
| VerticalOptions = LayoutOptions.Center, | ||
| HorizontalOptions = LayoutOptions.Center, | ||
| FontSize = 18, | ||
| }; | ||
| label.SetBinding(Label.TextProperty, "."); | ||
| return label; | ||
| }), | ||
| }; | ||
|
|
||
| var setDefaultSizeButton = new Button | ||
| { | ||
| AutomationId = "SetDefaultSizeButton", | ||
| Text = "Set IndicatorSize = 6 (Default)", | ||
| Margin = new Thickness(0, 20, 0, 0) | ||
| }; | ||
|
|
||
| setDefaultSizeButton.Clicked += (s, e) => | ||
| { | ||
| indicatorView.IndicatorSize = 6; | ||
| }; | ||
|
|
||
| Content = new VerticalStackLayout | ||
| { | ||
| Padding = 20, | ||
| Spacing = 10, | ||
| Children = | ||
| { | ||
| carouselView, | ||
| indicatorView, | ||
| setDefaultSizeButton, | ||
| } | ||
| }; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues; | ||
|
|
||
| public class Issue35214 : _IssuesUITest | ||
| { | ||
| public Issue35214(TestDevice device) : base(device) | ||
| { | ||
| } | ||
|
|
||
| public override string Issue => "Dynamically changing IndicatorView IndicatorSize to default value does not work"; | ||
|
|
||
| [Test, Order(1)] | ||
| [Category(UITestCategories.IndicatorView)] | ||
| public void VerifyIndicatorSizeBeforeReset() | ||
| { | ||
| App.WaitForElement("TestIndicatorView"); | ||
| VerifyScreenshot("IndicatorSizeBeforeReset", retryTimeout: TimeSpan.FromSeconds(2)); | ||
| } | ||
|
|
||
| [Test, Order(2)] | ||
| [Category(UITestCategories.IndicatorView)] | ||
| public void VerifyIndicatorSizeAfterReset() | ||
| { | ||
| App.WaitForElement("SetDefaultSizeButton"); | ||
| App.Tap("SetDefaultSizeButton"); | ||
|
Shalini-Ashokan marked this conversation as resolved.
|
||
| VerifyScreenshot("IndicatorSizeAfterReset", retryTimeout: TimeSpan.FromSeconds(2)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,7 +13,7 @@ public class MauiPageControl : UIPageControl, IUIViewLifeCycleEvents | |
|
|
||
| WeakReference<IIndicatorView>? _indicatorView; | ||
| bool _updatingPosition; | ||
| double _lastAppliedIndicatorSize = -1; | ||
| double _lastAppliedIndicatorSize = DefaultIndicatorSize; | ||
|
Shalini-Ashokan 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. [minor] Regression Prevention - The original _lastAppliedIndicatorSize = -1 sentinel (introduced by PR #31463 to guarantee the scale transform fires on first layout pass alongside shadows) was changed to DefaultIndicatorSize. Added comment explaining the choice and explicitly verifying PR #31463 compatibility: Math.Abs(anyNonDefaultSize - 6) > 0 so first-pass scaling still fires correctly. |
||
|
|
||
| public MauiPageControl() | ||
| { | ||
|
|
@@ -65,7 +65,7 @@ public override void LayoutSubviews() | |
|
|
||
| public void UpdateIndicatorSize() | ||
| { | ||
| if (IndicatorSize == 0 || IndicatorSize == DefaultIndicatorSize) | ||
| if (IndicatorSize == 0 || IndicatorSize == _lastAppliedIndicatorSize) | ||
|
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] Logic and Correctness - The IndicatorSize == 0 early-return guard silently leaves a stale scale transform when IndicatorSize is set to 0 after a non-default value. Concrete scenario: user sets IndicatorSize = 20 (scale 20/6 applied, _lastAppliedIndicatorSize = 20), then sets IndicatorSize = 0 - the old guard fires, the foreach is skipped, indicators remain visually stuck at 20/6 scale. Fixed by treating IndicatorSize == 0 as DefaultIndicatorSize via targetSize = IndicatorSize == 0 ? DefaultIndicatorSize : IndicatorSize, so Math.Abs(6 - 20) = 14 >= tolerance correctly triggers an identity-scale reset. Also removed the redundant exact-equality check since the tolerance check already subsumes it. |
||
| return; | ||
|
|
||
| if (Math.Abs(IndicatorSize - _lastAppliedIndicatorSize) < IndicatorSizeTolerance) | ||
|
|
||
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] Regression Prevention / Test Coverage - VerifyIndicatorSizeAfterReset is [Order(2)] and implicitly depends on Order(1) having run first to capture the before-state baseline and leave the page in its initial state (IndicatorSize = 20). Added comment documenting this dependency and noting the after-state assertion remains valid if run in isolation, even though the before-state screenshot would be missing.