Reduce the number of calls to invalidate measure #21801
Conversation
There was a problem hiding this comment.
This is a "small and simple" change, but as we all know this has HUGE knock-on effects. iOS is smarter with layouts, but I worry about the other platforms. I will need to add a large bunch of UI tests and things like that.
However, this will at least get the ball rolling.
| label.InvalidateMeasureInternal(InvalidationTrigger.MeasureChanged); | ||
| label.InvalidateMeasureIfLabelSizeable(); |
There was a problem hiding this comment.
This is the case for all calls to invalidate, first check the conditions.
| 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); |
There was a problem hiding this comment.
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 (!isHorizontallySizeable && isSingleLine) | ||
| return false; |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This check was added before Xamarin.Forms was even public. I couldn't find a historical rationalization for why this was here.
| public override void InvalidateIntrinsicContentSize() | ||
| { | ||
| base.InvalidateIntrinsicContentSize(); | ||
|
|
||
| if (Frame.Width == 0 && Frame.Height == 0) | ||
| { | ||
| // The Label hasn't actually been laid out on screen yet; no reason to request a layout | ||
| return; | ||
| } | ||
|
|
||
| if (!Frame.Size.IsCloseTo(AddInsets(IntrinsicContentSize), (nfloat)0.001)) | ||
| { | ||
| // The text or its attributes have changed enough that the size no longer matches the set Frame. It's possible | ||
| // that the Label needs to be laid out again at a different size, so we request that the parent do so. | ||
| Superview?.SetNeedsLayout(); | ||
| } | ||
| } |
There was a problem hiding this comment.
I have not seen any real cases where this call should/does/needs to invalidate the layout. We are already calling invalidate in the Label class when we change the properties (Text, Font, CharSpacing) so this is redundant.
Also, comparing Frame to IntrinsicContentSize is ALWAYS incorrect. Assume you have a label with text "Hello" and the width of that text is say 100, then you set the label width of 200. The Frame now has a width of 200, but IntrinsicContentSize has a width of 100... This means it will ALWAYS trigger a layout.
jsuarezruiz
added
area-layout
|
Hi @mattleibow , |
|
@mattleibow Let's chat about this, to prepare a good list of UITests covering all the possible impacted scenarios. |
Eilon
added
t/perf
|
/rebase |
github-actions
bot
force-pushed
the
dev/reduce-invalidates
branch
from
7af3b6d
to
d96ee42
Compare
|
/rebase |
github-actions
bot
force-pushed
the
dev/reduce-invalidates
branch
from
d96ee42
to
c7bc495
Compare
|
/rebase |
github-actions
bot
force-pushed
the
dev/reduce-invalidates
branch
from
c7bc495
to
7df8d96
Compare
PureWeen
force-pushed
the
dev/reduce-invalidates
branch
from
7df8d96
to
2a182f6
Compare
Description of Change
While investigating layout calls with Label, we noticed a few extra calls to SetNeedsLayout than is particularly necessary. On iOS at least, multiple calls to SetNeedsLayout does not actually do anything as it just "sets a flag" for the next layout pass.
But. Sometimes we do not need to invalidate the layout. For example: