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

Get the correct iOS system font for each weight #48937

Merged
merged 17 commits into from
Jan 4, 2024

Conversation

MitchellGoodwin
Copy link
Contributor

@MitchellGoodwin MitchellGoodwin commented Dec 12, 2023

See flutter/issue/139689 and flutter/issue/132475

Registers all of the possible font weight values for "CupertinoSystemDisplay". The registering of "CupertinoSystemText" was removed, as the fallback will correctly show the right fonts, and this logic does not work for the smaller font due to Apple APIs removing the font weight.

The font weights for 800 and 900 had to be added outside of the loop as they were coming back with values different from what was being put in.

Before:
Screenshot 2023-12-06 at 1 17 11 PM

After:
Screenshot 2024-01-03 at 3 15 33 PM

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@MitchellGoodwin MitchellGoodwin changed the title [WIP] Get the correct system font for each weight Get the correct iOS system font for each weight Jan 3, 2024
@MitchellGoodwin MitchellGoodwin marked this pull request as ready for review January 3, 2024 23:27
third_party/txt/src/txt/platform_mac.mm Show resolved Hide resolved
ASSERT_EQ(dynamic_font_manager.font_provider()
.MatchFamily("CupertinoSystemDisplay")
->count(),
9);
Copy link
Member

Choose a reason for hiding this comment

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

When I run this locally I'm seeing a size of 10 instead of 9
(8 values from 0 through 700, plus the extra 780 and 810 weights)

@MitchellGoodwin MitchellGoodwin added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 4, 2024
@auto-submit auto-submit bot merged commit 0bbb4d6 into flutter:main Jan 4, 2024
27 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 4, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 4, 2024
…140990)

flutter/engine@b2a9ce8...0bbb4d6

2024-01-04 58190796+MitchellGoodwin@users.noreply.github.com Get the correct iOS system font for each weight (flutter/engine#48937)
2024-01-04 43054281+camsim99@users.noreply.github.com [Android] Re-land "Re-land  'Add target to have linux_android_emulator_tests run on AVDs with Android 33 & 34'" (flutter/engine#49101)
2024-01-04 godofredoc@google.com Use Mac M1s or x86 wherever possible. (flutter/engine#49540)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

return CTFontCreateCopyWithAttributes(ct_font, size, nullptr, var_font_desc);
}

Copy link
Member

Choose a reason for hiding this comment

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

I was profiling allocations - and this seems to be leaking the CFNumbers, CFMutableDictionary and CTFontDescriptor. I don't see CFRelease calls anywhere? I don't think there's a ScopedCFTypeRef in fml so this will need to be released manually.

Copy link
Member

Choose a reason for hiding this comment

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

@knopp could you file an issue for that so it doesn't get lost?

Copy link
Member

Choose a reason for hiding this comment

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

I think this has been already fixed when clang tidy started complaining about missing CFRelease.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know which release will contain the memory leak fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants