Skip to content
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

Remove MeasureInvalidated bubble-up mechanism / clean-up legacy layouts / fix iOS CV items resize #25664

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Nov 4, 2024

Description of Change

This PR includes #25465 and improves it more in order to have iOS CV1 and CV2 handlers rely on native invalidation mechanism rather than subscribing to MeasureInvalidated.

I'd prefer to simply merge this one and just close #25465.

What's included

  • Removal for MeasureInvalidated bubble-up mechanism (basically "revert"s Make MeasureInvalidated event work correctly #23052 and goes back to 8.0.82) behavior
  • Removal of Compatibility.Layout code useless and harming code (causing additional invalidations)
  • Changed the SetNeedsLayout mechanism which now relies on a more-measurable while-loop
    • The loop stops on UIScrollViews
    • Introduced IMauiPlatformView interface on iOS with two methods
      • InvalidateAncestorsMeasuresWhenMovedToWindow() to schedule propagating invalidation when view is attached to Window
      • InvalidateMeasure(bool isPropagating) to normally simply call SetNeedsLayout on the view
        • Thanks to this interface, we can customize the behavior
  • Refactored iOS CollectionViewHandler1 to use the new platform-level invalidation mechanism
  • Refactored iOS CollectionViewHandler2 to use the new platform-level invalidation mechanism
    • Before this change, CV2 was not reacting to measure changes from items

Using a initial version of the #25671 UI test I've been able to measure the benefits of this PR over main branch using CV1, and it is impressive:

  • main: Measure passes: 877, Arrange passes: 541
  • PR: Measure passes: 409, Arrange passes: 395

CV2 was not handling measure changes so we don't have data to compare with main.
On the other side we can compare CV1 and CV2 with the new fix and the latest version of #25671

  • Measure passes: 489 CV2 / 525 with CV1
  • Arrange passes: 359 CV2 / 308 with CV1

As we can see, CV2 is doing better with measure passes which is very likely the most expensive operation, so we should be good.

Issues Fixed

Fixes #25650
Fixes #24996
Fixes #25391
Fixes #25264

Videos of AllTheLists

Captured using a modified version of https://github.com/davidortinau/AllTheLists.git compiled in Release mode with a constant number of lessons generated (high number).

challenging.patch

Using CV1

It has some usual hiccups but nothing weird.

PR-25664-CV1-AllTheLists.mp4

Using CV2

Resizing of item works, but it has some hiccups.
Notice how the scrollbar acts weird in comparison to CV1.

PR-25664-CV2-AllTheLists.mp4

@albyrock87 albyrock87 requested a review from a team as a code owner November 4, 2024 11:49
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 4, 2024
@albyrock87 albyrock87 force-pushed the legacy-layout-cleanup branch 3 times, most recently from a465e64 to 5bef429 Compare November 4, 2024 14:27
@PureWeen
Copy link
Member

PureWeen commented Nov 4, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@albyrock87 albyrock87 force-pushed the legacy-layout-cleanup branch 3 times, most recently from 917a208 to 15d5364 Compare November 5, 2024 12:23
@albyrock87 albyrock87 changed the title Remove MeasureInvalidated bubble-up mechanism and clean-up legacy layouts. Remove MeasureInvalidated bubble-up mechanism / clean-up legacy layouts / fix iOS CV items resize Nov 5, 2024
}
if (trigger == InvalidationTrigger.HorizontalOptionsChanged || trigger == InvalidationTrigger.VerticalOptionsChanged)
{
ComputeConstraintForView(view);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been moved to VisualElement.InvalidateMeasureInternal()

@rmarinho
Copy link
Member

rmarinho commented Nov 5, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community ✨ Community Contribution
Projects
None yet
3 participants