-
Notifications
You must be signed in to change notification settings - Fork 2k
Reduce the number of calls to invalidate measure #21801
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 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 |
|---|---|---|
|
|
@@ -62,7 +62,8 @@ public partial class Label : View, IFontElement, ITextElement, ITextAlignmentEle | |
| formattedString.Parent = null; | ||
| label.RemoveSpans(formattedString.Spans); | ||
| } | ||
| }, propertyChanged: (bindable, oldvalue, newvalue) => | ||
| }, | ||
| propertyChanged: (bindable, oldvalue, newvalue) => | ||
| { | ||
| var label = ((Label)bindable); | ||
|
|
||
|
|
@@ -76,7 +77,8 @@ public partial class Label : View, IFontElement, ITextElement, ITextAlignmentEle | |
| label.SetupSpans(formattedString.Spans); | ||
| } | ||
|
|
||
| label.InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| label.InvalidateMeasureIfLabelSizeable(); | ||
|
|
||
| if (newvalue != null) | ||
| label.Text = null; | ||
| }); | ||
|
|
@@ -94,26 +96,21 @@ public virtual string UpdateFormsText(string source, TextTransform textTransform | |
|
|
||
| /// <summary>Bindable property for <see cref="LineBreakMode"/>.</summary> | ||
| public static readonly BindableProperty LineBreakModeProperty = BindableProperty.Create(nameof(LineBreakMode), typeof(LineBreakMode), typeof(Label), LineBreakMode.WordWrap, | ||
| propertyChanged: (bindable, oldvalue, newvalue) => ((Label)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged)); | ||
| propertyChanged: (bindable, oldvalue, newvalue) => ((Label)bindable).InvalidateMeasureIfLabelSizeable()); | ||
|
|
||
| /// <summary>Bindable property for <see cref="LineHeight"/>.</summary> | ||
| public static readonly BindableProperty LineHeightProperty = LineHeightElement.LineHeightProperty; | ||
|
|
||
| /// <summary>Bindable property for <see cref="MaxLines"/>.</summary> | ||
| public static readonly BindableProperty MaxLinesProperty = BindableProperty.Create(nameof(MaxLines), typeof(int), typeof(Label), -1, propertyChanged: (bindable, oldvalue, newvalue) => | ||
| { | ||
| if (bindable != null) | ||
| { | ||
| ((Label)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| } | ||
| }); | ||
| public static readonly BindableProperty MaxLinesProperty = BindableProperty.Create(nameof(MaxLines), typeof(int), typeof(Label), -1, | ||
| propertyChanged: (bindable, oldvalue, newvalue) => ((Label)bindable).InvalidateMeasureIfLabelSizeable()); | ||
|
|
||
| /// <summary>Bindable property for <see cref="Padding"/>.</summary> | ||
| public static readonly BindableProperty PaddingProperty = PaddingElement.PaddingProperty; | ||
|
|
||
| /// <summary>Bindable property for <see cref="TextType"/>.</summary> | ||
| public static readonly BindableProperty TextTypeProperty = BindableProperty.Create(nameof(TextType), typeof(TextType), typeof(Label), TextType.Text, | ||
| propertyChanged: (bindable, oldvalue, newvalue) => ((Label)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged)); | ||
| propertyChanged: (bindable, oldvalue, newvalue) => ((Label)bindable).InvalidateMeasureIfLabelSizeable()); | ||
|
|
||
| readonly Lazy<PlatformConfigurationRegistry<Label>> _platformConfigurationRegistry; | ||
|
|
||
|
|
@@ -260,24 +257,22 @@ void IFontElement.OnFontAutoScalingEnabledChanged(bool oldValue, bool newValue) | |
| void HandleFontChanged() | ||
| { | ||
| Handler?.UpdateValue(nameof(ITextStyle.Font)); | ||
| InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| InvalidateMeasureIfLabelSizeable(); | ||
| } | ||
|
|
||
| void ILineHeightElement.OnLineHeightChanged(double oldValue, double newValue) => | ||
| InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
|
|
||
| void OnFormattedTextChanging(object sender, PropertyChangingEventArgs e) | ||
| { | ||
| OnPropertyChanging(nameof(FormattedText)); | ||
| } | ||
| InvalidateMeasureIfLabelSizeable(); | ||
|
|
||
| void ITextElement.OnTextTransformChanged(TextTransform oldValue, TextTransform newValue) => | ||
| InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| InvalidateMeasureIfLabelSizeable(); | ||
|
|
||
| void OnFormattedTextChanging(object sender, PropertyChangingEventArgs e) => | ||
| OnPropertyChanging(nameof(FormattedText)); | ||
|
|
||
| void OnFormattedTextChanged(object sender, PropertyChangedEventArgs e) | ||
| { | ||
| OnPropertyChanged(nameof(FormattedText)); | ||
| InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| InvalidateMeasureIfLabelSizeable(); | ||
| } | ||
|
|
||
| void SetupSpans(IEnumerable spans) | ||
|
|
@@ -357,18 +352,19 @@ void Span_GestureRecognizer_CollectionChanged(object sender, NotifyCollectionCha | |
|
|
||
| void ITextAlignmentElement.OnHorizontalTextAlignmentPropertyChanged(TextAlignment oldValue, TextAlignment newValue) | ||
| { | ||
| // This is a no-op since the horizontal text alignment does not affect bounds or | ||
| // any other property that would require a measure invalidation. | ||
| } | ||
|
|
||
| static void OnTextPropertyChanged(BindableObject bindable, object oldvalue, object newvalue) | ||
| { | ||
| var label = (Label)bindable; | ||
| LineBreakMode breakMode = label.LineBreakMode; | ||
| bool isVerticallyFixed = (label.Constraint & LayoutConstraint.VerticallyFixed) != 0; | ||
| bool isSingleLine = !(breakMode == LineBreakMode.CharacterWrap || breakMode == LineBreakMode.WordWrap); | ||
| if (!isVerticallyFixed || !isSingleLine) | ||
| ((Label)bindable).InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
|
Comment on lines
-365
to
-369
Member
Author
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. This is a "major" change where I think the calls will prevent a valid measure from taking place. Assume you have a label: var label = new Label {
HeightRequest = 100,
MinimumHeightRequest = 10,
LineBreakMode = LineBreakMode.NoWrap,
}This condition will evaluate as follows: bool isVerticallyFixed = true; // the constraint evals to fixed as there is a height constraint
bool isSingleLine = true; // the break mode is NoWrap and thus is NOT NOT wrapping
if (!isVerticallyFixed || !isSingleLine) // becomes if (!true || !true) or falseHowever, if you look at the label definition, it is not a wrapping label and it has a fixed height. However, it does NOT have a fixed with. This means updating the text WILL require a measure since the width has changed. But here we do not call it. |
||
|
|
||
| if (TextChangedShouldInvalidateMeasure(label)) | ||
| label.InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
|
|
||
| if (newvalue != null) | ||
| ((Label)bindable).FormattedText = null; | ||
| label.FormattedText = null; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
|
|
@@ -379,12 +375,12 @@ public IPlatformElementConfiguration<T, Label> On<T>() where T : IConfigPlatform | |
|
|
||
| void ITextElement.OnTextColorPropertyChanged(Color oldValue, Color newValue) | ||
| { | ||
| // This is a no-op since the text color does not affect bounds or | ||
| // any other property that would require a measure invalidation. | ||
| } | ||
|
|
||
| void ITextElement.OnCharacterSpacingPropertyChanged(double oldValue, double newValue) | ||
| { | ||
| InvalidateMeasure(); | ||
| } | ||
| void ITextElement.OnCharacterSpacingPropertyChanged(double oldValue, double newValue) => | ||
| InvalidateMeasureIfLabelSizeable(); | ||
|
|
||
| internal bool HasFormattedTextSpans | ||
| => (FormattedText?.Spans?.Count ?? 0) > 0; | ||
|
|
@@ -411,16 +407,75 @@ public override IList<GestureElement> GetChildElements(Point point) | |
| return spans; | ||
| } | ||
|
|
||
| Thickness IPaddingElement.PaddingDefaultValueCreator() | ||
| { | ||
| return default(Thickness); | ||
| } | ||
| Thickness IPaddingElement.PaddingDefaultValueCreator() => default; | ||
|
|
||
| void IPaddingElement.OnPaddingPropertyChanged(Thickness oldValue, Thickness newValue) => | ||
| InvalidateMeasureIfLabelSizeable(); | ||
|
|
||
| Font ITextStyle.Font => this.ToFont(); | ||
|
|
||
| void IPaddingElement.OnPaddingPropertyChanged(Thickness oldValue, Thickness newValue) | ||
| /// <summary> | ||
| /// This method prevents unnecessary measure invalidations when the label is not | ||
| /// sizeable. If the label has a fixed width and height, then no matter what the | ||
| /// text is, the label will never change size. | ||
| /// </summary> | ||
| void InvalidateMeasureIfLabelSizeable() | ||
| { | ||
| if (!IsLabelSizeable(this)) | ||
| return; | ||
|
|
||
| InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| } | ||
|
|
||
| Font ITextStyle.Font => this.ToFont(); | ||
| /// <summary> | ||
| /// Determines if the label can grow in any direction based on the constraints. If the | ||
| /// label cannot grow in any direction, then we usually don't need to do anything. | ||
| /// </summary> | ||
| internal static bool IsLabelSizeable(Label label) | ||
| { | ||
| // Determine in which direction the label can grow/shrink. | ||
| var constraint = label.Constraint; | ||
| var isVerticallySizeable = (constraint & LayoutConstraint.VerticallyFixed) == 0; | ||
| var isHorizontallySizeable = (constraint & LayoutConstraint.HorizontallyFixed) == 0; | ||
| var isSizeable = isVerticallySizeable || isHorizontallySizeable; | ||
|
|
||
| // If the label cannot grow in any direction, then we usually don't need to do anything. | ||
| if (!isSizeable) | ||
| return false; | ||
|
|
||
| // The label may grow/shrink based on the constraints, so we may need to invalidate. | ||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Determines if the text has changed in a way that would require a measure invalidation. | ||
| /// Unlike FormattedText changes, Text changes may not always require invalidation because | ||
| /// the text size and spacing is all uniform. Formatted text may have a case where even | ||
| /// though the label is a single line, the font size of a span may cause the label to grow | ||
| /// vertically. | ||
| /// </summary> | ||
| internal static bool TextChangedShouldInvalidateMeasure(Label label) | ||
| { | ||
| // If the label cannot grow in any direction, then we don't need to invalidate. | ||
| var isSizeable = IsLabelSizeable(label); | ||
| if (!isSizeable) | ||
| return false; | ||
|
|
||
| // Determine if the label can grow vertically (wrapping means it may grow vertically). | ||
| var constraint = label.Constraint; | ||
| var breakMode = label.LineBreakMode; | ||
| var isHorizontallySizeable = (constraint & LayoutConstraint.HorizontallyFixed) == 0; | ||
| var isMultiline = breakMode == LineBreakMode.CharacterWrap || breakMode == LineBreakMode.WordWrap; | ||
| var isSingleLine = !isMultiline; | ||
|
|
||
| // If the label cannot grow horizontally and is only single line, | ||
| // then we don't need to invalidate since the only direction it can grow in | ||
| // is vertically but it never will. | ||
| if (!isHorizontallySizeable && isSingleLine) | ||
| return false; | ||
|
Comment on lines
+474
to
+475
Member
Author
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. This condition is the new condition. Besides the check to make sure that if the width AND height are fixed, this check makes sure to only avoid a layout if the WIDTH is constrained. Previously the height was the constraint that was used however it was not correct. And even if it was correct in some cases, we cannot assume vertical and horizontal are mutually exclusive. |
||
|
|
||
| // The label may grow/shrink based on the constraints, so we need to invalidate. | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1628,11 +1628,11 @@ static void OnRequestChanged(BindableObject bindable, object oldvalue, object ne | |
| { | ||
| var constraint = LayoutConstraint.None; | ||
| var element = (VisualElement)bindable; | ||
| if (element.WidthRequest >= 0 && element.MinimumWidthRequest >= 0) | ||
| if (element.WidthRequest >= 0) | ||
| { | ||
| constraint |= LayoutConstraint.HorizontallyFixed; | ||
| } | ||
| if (element.HeightRequest >= 0 && element.MinimumHeightRequest >= 0) | ||
| if (element.HeightRequest >= 0) | ||
|
Comment on lines
-1631
to
+1635
Member
Author
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. This is very interesting, and I am not sure I agree with the logic before. Why is the fixed status determined by the minimum bounds? If I have a view that has a height of say 100, why is it dependent on the min height? This is an AND operation, so BOTH need to be set, but setting height is all that is needed. If I set a Height of 100 and a MinHeight of 0, 10, 100, 200 it makes no difference.
Member
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. This check was added before Xamarin.Forms was even public. I couldn't find a historical rationalization for why this was here. |
||
| { | ||
| constraint |= LayoutConstraint.VerticallyFixed; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| #if IOS | ||
| using NUnit.Framework; | ||
| using UITest.Appium; | ||
| using UITest.Core; | ||
|
|
||
| namespace Microsoft.Maui.TestCases.Tests.Issues | ||
| { | ||
| internal class ReduceInvalidateMeasure : _IssuesUITest | ||
| { | ||
| public ReduceInvalidateMeasure(TestDevice device) | ||
| : base(device) | ||
| { } | ||
|
|
||
| public override string Issue => "https://github.com/dotnet/maui/pull/21801"; | ||
|
|
||
| [Test] | ||
| public void ReduceInvalidateMeasuresUpdatingLabel() | ||
| { | ||
| App.WaitForElement("UpdateTextLabel"); | ||
|
|
||
| const int repeats = 2; | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateTextButton"); | ||
| } | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateSizeButton"); | ||
| } | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateFontSizeButton"); | ||
| } | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateLineBreakModeButton"); | ||
| } | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateLineHeightButton"); | ||
| } | ||
|
|
||
| for (int i = 0; i < repeats; i++) | ||
| { | ||
| App.Tap("UpdateVisibilityButton"); | ||
| } | ||
|
|
||
| VerifyScreenshot(); | ||
| } | ||
| } | ||
| } | ||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| <?xml version="1.0" encoding="utf-8" ?> | ||
| <ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui" | ||
| xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml" | ||
| x:Class="Maui.Controls.Sample.Issues.ReduceInvalidateMeasure"> | ||
| <ScrollView> | ||
| <VerticalStackLayout | ||
| Padding="12"> | ||
| <Label | ||
| x:Name="UpdateTextLabel" | ||
| AutomationId="UpdateTextLabel" | ||
| BackgroundColor="Red" | ||
| Text="Lorem ipsum dolor sit amet"/> | ||
| <Button | ||
| x:Name="UpdateTextButton" | ||
| AutomationId="UpdateTextButton" | ||
| Text="Update Text" | ||
| Clicked="OnUpdateTextButtonClicked"/> | ||
| <Label | ||
| x:Name="UpdateSizeLabel" | ||
| AutomationId="UpdateSizeLabel" | ||
| BackgroundColor="Blue" | ||
| Text="Lorem ipsum dolor" | ||
| HorizontalOptions="Start" | ||
| WidthRequest="200"/> | ||
| <Button | ||
| x:Name="UpdateSizeButton" | ||
| AutomationId="UpdateSizeButton" | ||
| Text="Update Label Size" | ||
| Clicked="OnUpdateSizeButtonClicked"/> | ||
| <Label | ||
| x:Name="UpdateFontSizeLabel" | ||
| AutomationId="UpdateFontSizeLabel" | ||
| TextColor="Green" | ||
| FontSize="16" | ||
| Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit"/> | ||
| <Button | ||
| x:Name="UpdateFontSizeButton" | ||
| AutomationId="UpdateFontSizeButton" | ||
| Text="Update Label FontSize" | ||
| Clicked="OnUpdateFontSizeButtonClicked"/> | ||
| <Label | ||
| x:Name="UpdateLineBreakModeLabel" | ||
| AutomationId="UpdateLineBreakModeLabel" | ||
| Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit." | ||
| LineBreakMode="WordWrap"/> | ||
| <Button | ||
| x:Name="UpdateLineBreakModeButton" | ||
| AutomationId="UpdateLineBreakModeButton" | ||
| Text="Update Label LineBreakMode" | ||
| Clicked="OnUpdateLineBreakModeButtonClicked"/> | ||
| <Label | ||
| x:Name="UpdateLineHeightLabel" | ||
| AutomationId="UpdateLineHeightLabel" | ||
| LineHeight="1" | ||
| Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit. Lorem ipsum dolor sit amet, consectetur adipiscing elit."/> | ||
| <Button | ||
| x:Name="UpdateLineHeightButton" | ||
| AutomationId="UpdateLineHeightButton" | ||
| Text="Update Label LineHeight" | ||
| Clicked="OnUpdateLineHeightButtonClicked"/> | ||
| <Label | ||
| x:Name="UpdateVisibilityLabel" | ||
| AutomationId="UpdateVisibilityLabel" | ||
| FontSize="Medium" | ||
| TextColor="Orange" | ||
| Text="Lorem ipsum dolor sit amet, consectetur adipiscing elit." | ||
| LineBreakMode="WordWrap"/> | ||
| <Button | ||
| x:Name="UpdateVisibilityButton" | ||
| AutomationId="UpdateVisibilityButton" | ||
| Text="Update Label Visibility" | ||
| Clicked="OnUpdateVisibilityButtonClicked"/> | ||
| </VerticalStackLayout> | ||
| </ScrollView> | ||
| </ContentPage> |
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.
This is the case for all calls to invalidate, first check the conditions.