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 blurry FontImageSource icons in iOS Tab Bar and buttons #16864

Merged
merged 19 commits into from Aug 30, 2023

Conversation

japarson
Copy link
Contributor

@japarson japarson commented Aug 18, 2023

Description of Change

We are not properly scaling the iOS icons:

var scale = destinationContext.Window?.Screen?.Scale ?? 1;

The issue here is that on the first render, the Window is null, so we use a scale of 1 instead of e.g., 3. We can instead get the Window and scale through MauiContext. In the tab bar, we weren't using a scale at all.

image

Issues Fixed

Fixes #6043
Fixes #14728

@japarson japarson requested a review from a team as a code owner August 18, 2023 19:50
@japarson
Copy link
Contributor Author

@jknaudt21 Can you point out any documentation that needs to be added?

@Eilon Eilon added the area/image 🖼️ Image loading, sources, caching label Aug 18, 2023
@samhouts samhouts added this to the .NET 8 GA milestone Aug 18, 2023
@jknaudt21
Copy link
Contributor

@jknaudt21 Can you point out any documentation that needs to be added?

Not really. A lot of Maui.Platform doesn't get displayed in the docs (don't know why), so there's nothing here that stands out.

That being said, you could always take the opportunity to document UpdateSourceAsync 😋

jknaudt21
jknaudt21 previously approved these changes Aug 18, 2023
Copy link
Contributor

@jknaudt21 jknaudt21 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Aside from seeing if Windows should also receive the same fix (you already mentioned this in the comments), my only recommendation would be to add a test to check that that image's scale matches that of the IMauiContext. Not sure if that's even possible, but might we worth a try.

@mattleibow
Copy link
Member

I think I also had a start with this, but no idea what state it is in: https://github.com/dotnet/maui/pull/14807/files

One difference I see is that I used an optional window method, so I am wondering if there are situations where there is no window yet.

Also, does this respond to density/scaling changes? Like on macOS you update the display scale?

@PureWeen
Copy link
Member

One difference I see is that I used an optional window method, so I am wondering if there are situations where there is no window yet.

I don't think so, the MauiContext you use for this stuff only comes into existence if there's a window.

We could still keep the code in place here

https://github.com/dotnet/maui/pull/16864/files#diff-a209ad18263c1c3effbd89b25fff7dbe248270890bb057f2564180cb0b7550b5L32

That defaults to one if no windows is set though.

Maybe it can log a warnings?

Also, does this respond to density/scaling changes? Like on macOS you update the display scale?

Not really sure how to achieve this right now. Image does this because we inherit from UIImageView but we can't inherit from UIButton. Perhaps we just need to wrap all our UIButtons so we can get life cycle events from them.

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

Looks OK generally, maybe some thoughts:

  • we should probably avoid API shanges
  • Maybe use the ubiquitous GetDisplayDensity() on all platform windows to get the density as it is a uniform API and supports windows' unique features.

src/Core/src/ImageSources/ImageSourceExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt Outdated Show resolved Hide resolved
src/Core/src/Platform/Android/ImageSourcePartExtensions.cs Outdated Show resolved Hide resolved
src/Core/src/Platform/Windows/ImageSourcePartExtensions.cs Outdated Show resolved Hide resolved
@jfversluis
Copy link
Member

@japarson you're still OK processing the feedback and taking this home? If not, please let us know so we can look at it! Thanks!

@japarson
Copy link
Contributor Author

@japarson you're still OK processing the feedback and taking this home? If not, please let us know so we can look at it! Thanks!

Yep, working on it. Just had to wait for a couple PRs from Matt and Shane.

Copy link
Member

@mattleibow mattleibow left a comment

Choose a reason for hiding this comment

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

looks good.

@PureWeen PureWeen enabled auto-merge (squash) August 29, 2023 22:44
@PureWeen PureWeen merged commit 43f4174 into main Aug 30, 2023
38 checks passed
@PureWeen PureWeen deleted the dev/japarson/6043 branch August 30, 2023 03:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/image 🖼️ Image loading, sources, caching platform/iOS 🍎
Projects
None yet
7 participants