Repository navigation
[rfw] Fix fontWeight not applied in release mode - #11581
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated fontWeight decoder to handle FontWeight values as strings or integers, ensuring stability in release mode. It updates strutStyle and textStyle to utilize this new decoder and includes comprehensive tests. Feedback identifies several instances where absolute keys are used for property lookups—specifically textDirection in the sweep shader, and textBaseline and overflow in textStyle—instead of relative keys, which prevents correct resolution when these structures are nested.
d9c573b to
a8bd4b5
Compare
|
Thanks for the contribution! In the future, please do not delete the checklist that is in the PR template; it is there for a reason. This PR is missing required elements described in the checklist, which need to be addressed before it moves forward with review. I am marking the PR as a Draft. Please re-add and complete the checklist, updating the PR as appropriate, and when that’s complete please feel free to mark the PR as ready for review. |
|
Thanks for the feedback! I've re added the PR template checklist and completed all applicable items. I have marked PR as Ready for review. Also this is my first time contributing to open source, so I appreciate the guidance! |
There was a problem hiding this comment.
Code Review
This pull request introduces a dedicated fontWeight decoder to resolve issues with FontWeight stability in release mode, updating TextStyle and StrutStyle to utilize it. It also fixes a key resolution bug in the sweep shader decoder and includes comprehensive unit tests for the new decoding logic. Feedback suggests simplifying the numeric FontWeight mapping by leveraging the FontWeight.values list for improved conciseness and robustness.
|
It looks like you've run the autoformatter on some files that don't use it (look for the |
524ee17 to
1944db2
Compare
|
Apologies for that, the auto-formatter was mistakenly run on the hand-formatted files. I've reverted all unrelated formatting changes; the PR now only touches argument_decoders.dart (new fontWeight decoder + two call site updates), argument_decoders_test.dart (regression test), CHANGELOG.md, and pubspec.yaml. |
f919678 to
359bc88
Compare
Please don't just repost AI-generated comments verbatim; reviewers don't need to have their review feedback repeated back to them. |
869467b to
a41ff8d
Compare
Sorry about that It's my first pr not much aware of rules. |
| return gradient(source, key)!.createShader( | ||
| rect(source, [...key, 'rect']) ?? Rect.zero, | ||
| textDirection: enumValue<TextDirection>(TextDirection.values, source, ['textDirection']) ?? TextDirection.ltr, | ||
| textDirection: enumValue<TextDirection>(TextDirection.values, source, [...key, 'textDirection']) ?? TextDirection.ltr, |
There was a problem hiding this comment.
This path-scoping fix (and the two similar ones below at textBaseline L1416 and overflow L1430) is a real bug fix but is unrelated to the fontWeight AOT issue this PR targets, and it isn't mentioned in the description or CHANGELOG. It changes behavior for any caller that previously relied on the (broken) root-level lookup, so consider splitting it into a separate PR with its own CHANGELOG entry.
There was a problem hiding this comment.
I added those fixes in response to gemini comments but you're right they're not in scope of this issue. So As of now I have removed those path scoping fixes from this pr.
2210bcc to
4bd046a
Compare
| return null; | ||
| } | ||
|
|
||
| /// Returns a [FontWeight] from the specified string or integer. |
There was a problem hiding this comment.
Bonus points for explaining the condition when the function returns null.
|
What's left to land this? |
4bd046a to
1107151
Compare
1107151 to
4612ab1
Compare
|
Hey @iam-abhijha it looks like this fell through the review cracks. Sorry about that. Would you like to still land this change? If so, can you rebase with the tip of tree? |
FontWeight is not a real Dart enum; it is a hand-crafted class with
static const members. The previous implementation decoded fontWeight via
enumValue(), which matched strings against FontWeight.value.toString().
In AOT/release builds this is not guaranteed to be stable, so matches
silently failed and fontWeight defaulted to null (rendered as w400).
Add a dedicated ArgumentDecoders.fontWeight() method that maps strings
("w100"-"w900", "normal", "bold") and integers (100-900) to the correct
FontWeight constant using an explicit switch, without relying on
toString(). Update textStyle and strutStyle to use it.
Fixes flutter/flutter#180223
4612ab1 to
7401b19
Compare
Piinks
added
CICD
5 participants
Fixes the
fontWeightproperty of RFWTextStyle(andStrutStyle) not being applied in release mode forTextwidgets.Fixes flutter/flutter#180223
Root cause
ArgumentDecoders.textStyle(andstrutStyle) decodedfontWeightusingenumValue(FontWeight.values, ...), which matched the input string againstFontWeight.value.toString().split('.').last.FontWeightis not a real Dartenum; it is a hand-crafted class withstatic constmembers. In AOT/release builds,toString()for such classes is not guaranteed to return the same human-readable name (tree-shaking can strip debug labels), so all matches silently fail andnullis returned, causing Flutter to fall back to the default weight (w400).Fix
Added a new
ArgumentDecoders.fontWeightstatic method that maps strings ("w100"–"w900","normal","bold") and integers (100–900) to the correctFontWeightconstant using an explicitswitch, without relying ontoString(). Updated bothtextStyleandstrutStyleto use this new decoder.Pre-Review Checklist
[shared_preferences]///).