-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[google_fonts] Update the font generator to exclude variable font entries when a static entry of the same weight and style exists #10739
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
base: main
Are you sure you want to change the base?
Conversation
Updated the font generator to exclude variable font entries when a static entry of the same weight and style exists, keeping variable fonts only if no static equivalent is present. Added new fields to the Font proto for postScriptName and isVf, and removed related static font entries from generated Dart parts. This prepares for future explicit variable font support and improves font selection consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request updates the font generator to prevent duplicate font entries by excluding variable fonts when a static equivalent with the same weight and style already exists. This is achieved by adding isVf and postScriptName fields to the Font protobuf message, regenerating the necessary Dart files, and implementing filtering logic in the generator. My review focuses on the new filtering logic, for which I've suggested a more efficient and readable implementation that avoids a nested loop.
|
@stuartmorgan-g I added |
Since the files are generated by the package itself, we could change them to not use the |
For sure, can do that. But what of the question about failing tests? |
It looks to me like it's failing earlier than trying to run them; it's not able to even compile the file. It looks like using path-based includes that reach outside of |
|
Stepping back slightly, can you write a test that tests that the generated files have the property we want, rather than directly testing the implementation of the generator? |
We can't test the API directly since Dart always returns the last value if a map contains duplicates. So that leaves clunky string matching or using the ast package to check for duplicates. Which is preferable do you think, or is there another way? |
|
Is the core thing we want to test just "there aren't duplicate entries with the same key", or is it "we removed the right duplicate"? If it's the former, the best option is probably to make the filename change; we don't need to test things that the analyzer will catch. If it's the latter, could we pick some specific known values to test in the output (to see that it's the static version), with a comment explaining how to pick a new test value if the test starts failing after running the generator due to font changes? |
Renamed generated files from .g.dart to .dart for google_fonts_all_parts and all font part files. Updated generator, template, and main library to use the new file extensions, and added some missing public API documentation.
Introduced functions to count static and variable font variants before and after deduplication. The generator now prints detailed statistics about font families and their variants, aiding in validation and debugging.
Former is covered now by file rename. For the latter, I suppose I'm fairly confident in the correctness right now. I also added some debugging to the generator to clarify what's happening |
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // ignore_for_file: specify_nonobvious_property_types, specify_nonobvious_local_variable_types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ignoring these because all types are the same function type with a long list of arguments (e.g. https://pub.dev/documentation/google_fonts/latest/google_fonts/GoogleFonts/aBeeZee-constant.html), which makes for quite ugly code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
I'm pretty sure the reason I renamed the files as part of the import was that I started to fix analyzer issues, ran into this, realized it would be awful, and then was going to add ignores but realized that since it was generated code we could just treat it like we do other generated code. Given that we now have a reason not to do that, the ignores strike the right balance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, that makes sense!
#10713 accidentally introduced duplicate variable fonts for 400 weight fonts, an issue only discovered through https://pub.dev/packages/google_fonts/score, because this repo's analysis options exclude generated files. Using 7.0.0 would cause the variable font variant to be used for 400 weight, a non-issue, apart from the typically larger font size.
With this PR:
Fixes flutter/flutter#180634
Related to flutter/flutter#174575 and flutter/flutter#174567
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or I have commented below to indicate which version change exemption this PR falls under1.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or I have commented below to indicate which CHANGELOG exemption this PR falls under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3