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

Fix Android arrange coordinate conversion #27179

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

Conversation

albyrock87
Copy link
Contributor

@albyrock87 albyrock87 commented Jan 16, 2025

Description of Change

Android emulator (for instance) has 2.625 display density multiplier.

Let's say you're arranging a label to this frame:

X = 112.00000000000001dp
Y = 318.66666666666669dp
Width = 187.42857142857142dp
Height = 22.095238095238095dp

If we convert this to pixels (Ceiling is how MAUI rounds it) we get

X = 294.00000000000006 => 295px
Y = 836.5 => 837px
Width = 491.99999999999994 => 492px
Height = 58px

Now here's the deal: the output rectangle must have width = 492px.

Problem is: when arranging, MAUI is converting left, right, top, bottom coordinates separately, where right = X + Width (same for bottom).

So basically we get

X + Width = (187.42857142857142 + 112.00000000000001) * 2.625

which is

299.42857142857144 * 2.625 = 786

Now if we do 786px - 295px we get 491px which is less than 492px expected platform width.
This is the reason the Label is being cut-off.

The reason MAUI is Ceiling when converting to pixel is exactly to avoid cutting content, but that's a valid reasoning only when talking about width or height.
The coordinate system should instead follow the Rounding principle to accomodate the content to the nearest pixel: in the example above, it makes no sense to convert 294.00000000000006 to 295.

This PR applies this reasoning to the arrange system.

Issues Fixed

Fixes #17884

@albyrock87 albyrock87 requested a review from a team as a code owner January 16, 2025 13:43
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Jan 16, 2025
@veikkoeeva
Copy link

@albyrock87 Just a random person here, but that looks like a solid explanation to add also in the code so it's easily discoverable also later to whoever reads the code.

Copy link
Contributor

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

@albyrock87
Copy link
Contributor Author

I've added the comments in the code.

That said, I'm afraid this PR will generate UITests failures everywhere and it'll take forever to capture all the screenshots.

@albyrock87
Copy link
Contributor Author

albyrock87 commented Jan 16, 2025

We have another option to not break all the UI tests: keep the Ceiling rounding for coordinates too, and change this behavior only with .NET10.

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

In theory it should generated the needed screenshots and we can just replace, or we can ask for help to finish that tedious work of updating the screenshots.

@PureWeen
Copy link
Member

I've added the comments in the code.

That said, I'm afraid this PR will generate UITests failures everywhere and it'll take forever to capture all the screenshots.

once we see the screenshots that'll give us an idea of the magnitude of change. There's definitely a lot :-)
We'll see how subtle/large it is once those are uploaded

@PureWeen
Copy link
Member

PureWeen commented Jan 17, 2025

I think this change makes sense...

I'm a little curious about the initial conversion from px to dp and if we should be rounding that

Like, if a DP value is 294.0000000000001 it seems like we should just convert that to 294 when doing the PX to DP conversion

I'm also wondering if we should midpoint round the width and height also. It seems like that's how Android suggests always doing the rounding https://developer.android.com/training/multiscreen/screendensities#dips-pels

@albyrock87
Copy link
Contributor Author

albyrock87 commented Jan 17, 2025

Like, if a DP value is 294.0000000000001 it seems like we should just convert that to 294 when doing the PX to DP conversion

I'm afraid that value comes from the centering algorithm (in this specific example) and not from PX to DP conversion.

Obviously the centering algorithm uses DP coming from a measure pass which is in PX, but that's a plain px / density without any kind of rounding and that seems correct to me.

I also wonder why we're going through float while converting DP double to PX int.

@jsuarezruiz jsuarezruiz added area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter platform/android 🤖 labels Jan 17, 2025
var top = Context.ToPixels(frame.Top);
var bottom = Context.ToPixels(frame.Bottom);
var right = Context.ToPixels(frame.Right);
var (left, top, right, bottom) = context.ToPixels(frame);
Copy link
Contributor

Choose a reason for hiding this comment

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

Slightly impacts tests with Labels and custom FontFamily (around 1-2% of changes detected when checking the image).
image

Example:
image

Failing tests:

  • FontFamily
  • FontFamilyLoadsDynamically
  • VerifyTextDecorationAppliedProperly
  • Bugzilla53834Test
  • ChildFlexLayoutContentShouldAppear
  • SystemFontsShouldRenderCorrectly
  • LabelHyperlinkUnderlineColor

Could you review and update the failing ones? Let me know if can help with something.

{
EnsureMetrics(self);

return (float)Math.Round(dp * s_displayDensity, MidpointRounding.AwayFromZero);
Copy link
Contributor

Choose a reason for hiding this comment

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

Apart from the UITest, could be nice to include some device tests checking conversions.

@PureWeen
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

/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
area-layout StackLayout, GridLayout, ContentView, AbsoluteLayout, FlexLayout, ContentPresenter community ✨ Community Contribution platform/android 🤖
Projects
Status: Changes Requested
Development

Successfully merging this pull request may close these issues.

[Android] Entire words omitted & letters truncated from Label display
8 participants