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

Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 31a7f4d..e7eb1c8 (7 commits) #26332

Merged
merged 39 commits into from Feb 4, 2019

Conversation

GaryQian
Copy link
Contributor

@GaryQian GaryQian commented Jan 10, 2019

This is the framework-half of the work to implement strut. See #26085.

The engine half can be found in flutter/engine#7414

This PR adds class StrutStyle, and introduces it as a new parameter to Text, RichText, TextPainter, and RenderParagraph. StrutStyle defines the properties necessary to fully define a strut.

git log --oneline --no-merges 31a7f4d..e7eb1c8
e7eb1c8 Update the verify_exported script to include the symbols for ICU data (#7647)
b032bbd Roll src/third_party/skia 70ebd9ca0616..9c8ad0316147 (9 commits) (#7646)
2c610bf Fix dynamic array -> vector (#7645)
09a3735 Respect default goma path on Windows (#7643)
050dcaa Embed ICU data inside libflutter.so on Android (#7588)
c92df42 Strut implementation (#7414)
15f2b92 Roll src/third_party/skia 673a048b209c..70ebd9ca0616 (8 commits) (#7641)

@zoechi zoechi added framework flutter/packages/flutter repository. See also f: labels. a: typography Text rendering, possibly libtxt a: text input Entering text in a text field or keyboard related problems labels Jan 10, 2019
@GaryQian
Copy link
Contributor Author

cc @Hixie This includes the API changes we talked about.
cc @jason-simmons This is the other half of the engine strut PR

@GaryQian
Copy link
Contributor Author

Some points to note:

The strut default values are defined in the engine instead of the framework. This is so that the minimal amount of information is necessary to pass between dart and native. The framework is capable of passing nothing to native to indicate usage of default properties.

The way the data is serialized allows for all non-strut uses to pass zero bytes to native.This adds minimal overhead to use cases without strut (the vast majority of text). When a strut is used, the minimal data passed is one byte with each additional property adding only its own size to the serialization. For example, a StrutStyle(fontSize: 15) passes 5 bytes (one for mask, 4 for the float).

@GaryQian GaryQian changed the title Add Strut and StrutStyle to Text, RichText, TextPainter, and RenderParagraph Add StrutStyle to Text, RichText, TextPainter, and RenderParagraph Jan 16, 2019
Copy link
Contributor

@HansMuller HansMuller left a comment

Choose a reason for hiding this comment

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

Everything looks pretty good, most of the feedback is small potatoes. I think there could be a little more clarity about exactly how StrutStyle maps to line heights and baselines, and some guidance about when would an app need to specify strut styles.

packages/flutter/lib/src/painting/strut_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/painting/strut_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/painting/strut_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/painting/strut_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/painting/strut_style.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/painting/text_style.dart Outdated Show resolved Hide resolved
@Hixie
Copy link
Contributor

Hixie commented Jan 29, 2019

LGTM with minor comments, but the bots aren't happy.

@GaryQian
Copy link
Contributor Author

Need to land the engine change, roll the engine manually, update goldens, and update assets-for-api-docs before everything can go green. It is working locally.

GaryQian added a commit to flutter/engine that referenced this pull request Jan 30, 2019
@GaryQian GaryQian changed the title Add StrutStyle to Text, RichText, TextPainter, and RenderParagraph Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui Jan 30, 2019
@GaryQian GaryQian changed the title Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine (9 commits) Jan 30, 2019
@GaryQian GaryQian changed the title Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine (9 commits) Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 01b4f4..e7eb1c8 (9 commits) Jan 30, 2019
GaryQian added a commit to flutter/goldens that referenced this pull request Jan 30, 2019
@GaryQian GaryQian changed the title Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 01b4f4..e7eb1c8 (9 commits) Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 31a7f4d..e7eb1c8 (9 commits) Jan 30, 2019
@GaryQian GaryQian changed the title Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 31a7f4d..e7eb1c8 (9 commits) Integrate Strut: Add StrutStyle, expose Strut API, wire up strut with dart:ui, Roll engine 31a7f4d..e7eb1c8 (7 commits) Jan 30, 2019
@GaryQian
Copy link
Contributor Author

GaryQian commented Feb 4, 2019

For reference: @dnfield brought up compatibility with Flutter SVG plugin when it used lineHeight in a previous version.

@GaryQian GaryQian merged commit c6cc3cd into flutter:master Feb 4, 2019
@Hixie
Copy link
Contributor

Hixie commented Feb 6, 2019

This seems to have regressed flutter_gallery_ios32__transition_perf 99th_percentile_frame_build_time_millis pretty badly (up to 30%-100%).

@Hixie
Copy link
Contributor

Hixie commented Feb 6, 2019

I suspect that that was caused by the machine in devicelab being bad. The regression is relative to an improvement that coincides with when I was in devicelab rebooting machines, and the PR where it improved is otherwise innocuous.

kangwang1988 pushed a commit to XianyuTech/flutter that referenced this pull request Feb 12, 2019
… dart:ui, Roll engine 31a7f4d..e7eb1c8 (7 commits) (flutter#26332)

Includes a breaking change to dart:ui ParagraphStyle where lineHeight is renamed to height for consistency with TextStyle.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a: text input Entering text in a text field or keyboard related problems a: typography Text rendering, possibly libtxt c: API break Backwards-incompatible API changes framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants