[Android] Span line height fix #20352

kubaflo · 2024-02-04T19:17:43Z

Description of Change

The fm.Top value was constantly increasing so that the height of each line grew progressively. Instead, the line height should be adjusted based on the initial top value of font metrics

Issues Fixed

Fixes #19592

Before	After

ghost · 2024-02-04T19:17:53Z

Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

jsuarezruiz · 2024-02-05T07:16:03Z

/azp run

azure-pipelines · 2024-02-05T07:16:24Z

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz · 2024-02-05T09:36:41Z

src/Controls/tests/UITests/Tests/Issues/Issue19592.cs

+			// The line height should be the same for each line
+			// of the paragraph, 1.5 and 2.5 respectively,
+			// as opposed to progressively growing
+			App.Screenshot();


Could we use here App.VerifyScreenshot? This will generate a snapshot to compare it with a reference one.

jsuarezruiz

The test NativeFormattedStringContainsSpan seems to be failing with the changes
Java.Lang.NullPointerException : Attempt to read from field 'float android.util.DisplayMetrics.scaledDensity' on a null object reference

kubaflo · 2024-02-05T19:08:24Z

@jsuarezruiz I have fixed the falling test

PureWeen · 2024-02-21T21:26:55Z

/azp run

azure-pipelines · 2024-02-21T21:27:16Z

Azure Pipelines successfully started running 3 pipeline(s).

jonathanpeppers · 2024-03-04T15:20:04Z

src/Controls/src/Core/Platform/Android/Extensions/FormattedStringExtensions.cs

@@ -64,6 +64,12 @@ public static SpannableString ToSpannableString(this Label label)

 			var builder = new StringBuilder();

+			var fontMetrics = context?.Resources?.DisplayMetrics != null ? new Paint()
+			{
+				TextSize = TypedValue.ApplyDimension(ComplexUnitType.Sp, (float)defaultFontSize, context.Resources.DisplayMetrics)


This line looks like it would throw if context.Resources returned null.

#I'm not sure why it would throw🤔
The only place that fontMetrics is here

maui/src/Controls/src/Core/Platform/Android/Extensions/FormattedStringExtensions.cs

Line 110 in 24d4049

if (span.LineHeight >= 0 && fontMetrics is not null)

and there's a null check

jonathanpeppers · 2024-03-04T15:22:28Z

src/Controls/src/Core/Platform/Android/Extensions/FormattedStringExtensions.cs

+			var fontMetrics = context?.Resources?.DisplayMetrics != null ? new Paint()
+			{
+				TextSize = TypedValue.ApplyDimension(ComplexUnitType.Sp, (float)defaultFontSize, context.Resources.DisplayMetrics)
+			}.GetFontMetrics()
+			: null;


Could we write this code in Java, it looks like it interops between C# and Java a lot. I've seen some past issue complaining about Android FormattedString performance like:

CollectionView+FormattedString Poor Performance #14222

So we'd make a version of this method that returns "Font metrics" instead of "Window metrics":

maui/src/Core/AndroidNative/maui/src/main/java/com/microsoft/maui/PlatformInterop.java

Lines 581 to 592 in ad28b65

/**

* Computes the current WindowMetrics' bounds

* @param activity

* @return Rect value of the bounds

*/

@NonNull

public static Rect getCurrentWindowMetrics(Activity activity) {

return WindowMetricsCalculator.Companion

.getOrCreate()

.computeCurrentWindowMetrics(activity)

.getBounds();

}

Yes, that would be beneficial, but I'm not sure how to write this code in JAVA and how to use it in MAUI

If you add a method in this file, it will magically appear on the PlatformInterop static class, you can call from C# like:

//signature public static Rect GetCurrentWindowMetrics(Activity activity); //usage var rect = PlatformInterop.GetCurrentWindowMetrics(myActivity);

Thank you! I pushed a new commit with this functionality

I made one small change, but it seems like this should be ready if we fix up the screenshot.

jonathanpeppers

This looks good to me if the new test & screenshot work. 👍

PureWeen · 2024-03-19T19:01:10Z

/rebase

* call `context.getResources().getDisplayMetrics()` once * build `maui.aar`

jsuarezruiz · 2024-03-21T10:39:06Z

/azp run MAUI-UITests-public

azure-pipelines · 2024-03-21T10:39:19Z

Azure Pipelines successfully started running 1 pipeline(s).

Conflicts: src/Core/src/maui.aar

jonathanpeppers

Fixed the merge conflict.

PureWeen · 2024-03-29T13:59:45Z

Not sure why UI Tests aren't triggering
Manual run here
https://dev.azure.com/xamarin/public/_build/results?buildId=112277&view=results

Context: https://github.com/Redth/MauiCollectionViewGallery Fixes: dotnet#14222 This will conflict with: * dotnet#20352 But I will rework this after it is merged. Profiling the above sample while scrolling on a Pixel 5, a lot of time is spent in `FormattedStringExtensions.RecalculateSpanPositions()`: (20%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.UpdateDrawState(Android.Text.TextPaint) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.Apply(Android.Text.TextPaint) (6.3%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedTex The key contributors are `FontSpan` and `LineHeightSpan` which: * Implement `MetricAffectingSpan`, an abstract class that allows to change the metrics of the text. * Implement `UpdateDrawState()` and `Apply()` methods that are called during draw. Causing frequent Java -> C# interop. To fix this, let's move the following types from C# to Java: * `FontSpan` -> `PlatformFontSpan` * `LetterSpacingSpan` -> `PlatformFontSpan` (use different ctor) * `LineHeightSpan` -> `PlatformLineHeightSpan` `PlatformLineHeightSpan` is similar, as it is an implementation of the `LineHeightSpan` interface. With these changes, I see a nice improvement while scrolling: (5.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (4.0%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedText `RecalculateSpanPositions` overall, went from 20% to 5.5%! Comparing the new types, the times spent calling the constructors are also improved: Before: (1.1%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan..ctor(Microsoft.Maui.Font,M (1.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.LetterSpacingSpan..ctor(double) After: (1.0%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(Android.Content.Context,Android.Graphics.Typeface,bool,single) (0.82%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(single) This should be reasonable for .NET 8servicing, as there should be no behavior changes and no API changes. In a future PR, it looks like `FormattedStringExtensions` could be improved further, but this is a decent starting point that makes it a lot better.

PureWeen · 2024-04-01T16:17:35Z

/azp run

azure-pipelines · 2024-04-01T16:17:56Z

Azure Pipelines successfully started running 3 pipeline(s).

Context: https://github.com/Redth/MauiCollectionViewGallery Fixes: dotnet#14222 This will conflict with: * dotnet#20352 But I will rework this after it is merged. Profiling the above sample while scrolling on a Pixel 5, a lot of time is spent in `FormattedStringExtensions.RecalculateSpanPositions()`: (20%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.UpdateDrawState(Android.Text.TextPaint) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.Apply(Android.Text.TextPaint) (6.3%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedTex The key contributors are `FontSpan` and `LineHeightSpan` which: * Implement `MetricAffectingSpan`, an abstract class that allows to change the metrics of the text. * Implement `UpdateDrawState()` and `Apply()` methods that are called during draw. Causing frequent Java -> C# interop. To fix this, let's move the following types from C# to Java: * `FontSpan` -> `PlatformFontSpan` * `LetterSpacingSpan` -> `PlatformFontSpan` (use different ctor) * `LineHeightSpan` -> `PlatformLineHeightSpan` `PlatformLineHeightSpan` is similar, as it is an implementation of the `LineHeightSpan` interface. With these changes, I see a nice improvement while scrolling: (5.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (4.0%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedText `RecalculateSpanPositions` overall, went from 20% to 5.5%! Comparing the new types, the times spent calling the constructors are also improved: Before: (1.1%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan..ctor(Microsoft.Maui.Font,M (1.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.LetterSpacingSpan..ctor(double) After: (1.0%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(Android.Content.Context,Android.Graphics.Typeface,bool,single) (0.82%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(single) This should be reasonable for .NET 8servicing, as there should be no behavior changes and no API changes. In a future PR, it looks like `FormattedStringExtensions` could be improved further, but this is a decent starting point that makes it a lot better.

Context: https://github.com/Redth/MauiCollectionViewGallery Fixes: #14222 This will conflict with: * #20352 But I will rework this after it is merged. Profiling the above sample while scrolling on a Pixel 5, a lot of time is spent in `FormattedStringExtensions.RecalculateSpanPositions()`: (20%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.UpdateDrawState(Android.Text.TextPaint) (11%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan.Apply(Android.Text.TextPaint) (6.3%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedTex The key contributors are `FontSpan` and `LineHeightSpan` which: * Implement `MetricAffectingSpan`, an abstract class that allows to change the metrics of the text. * Implement `UpdateDrawState()` and `Apply()` methods that are called during draw. Causing frequent Java -> C# interop. To fix this, let's move the following types from C# to Java: * `FontSpan` -> `PlatformFontSpan` * `LetterSpacingSpan` -> `PlatformFontSpan` (use different ctor) * `LineHeightSpan` -> `PlatformLineHeightSpan` `PlatformLineHeightSpan` is similar, as it is an implementation of the `LineHeightSpan` interface. With these changes, I see a nice improvement while scrolling: (5.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.RecalculateSpanPositions(Android.Widget.TextView,Microsoft.Maui.Controls.Label,Android.Text.SpannableString,Microsoft.Maui.SizeRequest) (4.0%) MauiCollectionViewGallery!PoolMathApp.Helpers.FormattedTextExtensions.ToFormattedString(PoolMathApp.Models.FormattedText `RecalculateSpanPositions` overall, went from 20% to 5.5%! Comparing the new types, the times spent calling the constructors are also improved: Before: (1.1%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.FontSpan..ctor(Microsoft.Maui.Font,M (1.5%) Microsoft.Maui.Controls!Microsoft.Maui.Controls.Platform.FormattedStringExtensions.LetterSpacingSpan..ctor(double) After: (1.0%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(Android.Content.Context,Android.Graphics.Typeface,bool,single) (0.82%) Microsoft.Maui!Microsoft.Maui.PlatformFontSpan..ctor(single) This should be reasonable for .NET 8servicing, as there should be no behavior changes and no API changes. In a future PR, it looks like `FormattedStringExtensions` could be improved further, but this is a decent starting point that makes it a lot better.

kubaflo requested a review from a team as a code owner February 4, 2024 19:17

kubaflo requested review from PureWeen and jsuarezruiz February 4, 2024 19:17

ghost added the community ✨ Community Contribution label Feb 4, 2024

jsuarezruiz added platform/android area-controls-label Label, Span labels Feb 5, 2024

ghost added the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label Feb 5, 2024

jsuarezruiz reviewed Feb 5, 2024

View reviewed changes

jonathanpeppers reviewed Mar 4, 2024

View reviewed changes

kubaflo force-pushed the span-line-height-android-fix branch from 778d523 to 12d9c14 Compare March 5, 2024 15:47

jonathanpeppers previously approved these changes Mar 5, 2024

View reviewed changes

kubaflo and others added 7 commits March 19, 2024 19:01

[Android] Fixed Wrong Span LineHeight (dotnet#19592)

8409bfc

Added a UI Test (dotnet#19592)

cfc0f67

Added a null checking (dotnet#19592)

c511456

Updated the UiTest (dotnet#19592)

eea4912

Added snapshot

1786480

Added Java native code

32912cb

Improve Java code

ce1b722

* call `context.getResources().getDisplayMetrics()` once * build `maui.aar`

github-actions bot force-pushed the span-line-height-android-fix branch from f32a663 to ce1b722 Compare March 19, 2024 19:02

jsuarezruiz previously approved these changes Mar 27, 2024

View reviewed changes

Merge branch 'main' into span-line-height-android-fix

18fdf8f

Conflicts: src/Core/src/maui.aar

jonathanpeppers dismissed stale reviews from jsuarezruiz and themself via 18fdf8f March 27, 2024 13:49

jonathanpeppers approved these changes Mar 27, 2024

View reviewed changes

PureWeen mentioned this pull request Mar 28, 2024

fix issue in orientation of images #21458

Merged

PureWeen approved these changes Mar 28, 2024

View reviewed changes

PureWeen enabled auto-merge (squash) March 28, 2024 23:39

jonathanpeppers mentioned this pull request Mar 29, 2024

[android] improve FormattedString performance #21530

Closed

PureWeen merged commit 8359359 into dotnet:main Apr 1, 2024

jonathanpeppers mentioned this pull request Apr 8, 2024

[android] improve FormattedString performance #21712

Merged

MartyIX mentioned this pull request Apr 9, 2024

[WinUI] LineHeight behavior is different from HTML&CSS behavior #21705

Open

github-actions bot locked and limited conversation to collaborators May 2, 2024

Eilon removed the legacy-area-controls Label, Button, CheckBox, Slider, Stepper, Switch, Picker, Entry, Editor label May 10, 2024

samhouts added fixed-in-8.0.20 fixed-in-9.0.0-preview.4.10690 labels Aug 2, 2024

	/**
	* Computes the current WindowMetrics' bounds
	* @param activity
	* @return Rect value of the bounds
	*/
	@NonNull
	public static Rect getCurrentWindowMetrics(Activity activity) {
	return WindowMetricsCalculator.Companion
	.getOrCreate()
	.computeCurrentWindowMetrics(activity)
	.getBounds();
	}

[Android] Span line height fix #20352

[Android] Span line height fix #20352

Uh oh!

Conversation

kubaflo commented Feb 4, 2024

Description of Change

Issues Fixed

Uh oh!

ghost commented Feb 4, 2024

Uh oh!

jsuarezruiz commented Feb 5, 2024

Uh oh!

azure-pipelines bot commented Feb 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsuarezruiz left a comment

Choose a reason for hiding this comment

Uh oh!

kubaflo commented Feb 5, 2024

Uh oh!

PureWeen commented Feb 21, 2024

Uh oh!

azure-pipelines bot commented Feb 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers Mar 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

PureWeen commented Mar 19, 2024

Uh oh!

jsuarezruiz commented Mar 21, 2024

Uh oh!

azure-pipelines bot commented Mar 21, 2024

Uh oh!

jonathanpeppers left a comment

Choose a reason for hiding this comment

Uh oh!

PureWeen commented Mar 29, 2024

Uh oh!

PureWeen commented Apr 1, 2024

Uh oh!

azure-pipelines bot commented Apr 1, 2024

Uh oh!

Uh oh!

jonathanpeppers Mar 5, 2024 •

edited

Loading