Tight Paragraph Width #8530

justinmc · 2019-04-10T19:54:03Z

Paragraph currently sets its width to be the maximum space allowed for it when it wraps beyond one line. This creates extra empty space when you want to draw something with a tight container:

This PR will enable the drawing of a paragraph with a width only as wide as it needs to be to contain wrapping text tightly.

Solution

I propose we add an enum to Text to allow the user to choose how width will be calculated.

TextWidthType.tight will render like a chat bubble:

Text(overflowingString, widthType: TextWidthType.tight),

TextWidthType.full, the default, will render like it always has:

Text(overflowingString),

Concerns

The naming was hastily chosen. I worry that users will expect that TextWidthType.full will cause the Text to render full width for a single line of text. Following existing behavior, it will not.

justinmc · 2019-04-10T20:02:17Z

Current behavior:

PR behavior:

justinmc · 2019-04-12T19:44:00Z

Note to self: There is a very similar enum BoxWidthStyle, any possible reuse?

justinmc · 2019-04-15T18:28:49Z

@GaryQian Can you give me a review on my approach here? I'll talk to Hans when he's around as well.
This is the paragraph width thing I was asking you for advice on the other day. Framework PR is here: flutter/flutter#30988

I put the enum inside of the engine, so users will have to import it from dart:ui. Were you also suggesting duplicating the enum in both the framework and the engine?

GaryQian · 2019-04-16T23:28:21Z

lib/ui/text.dart

-  if (fontFamily != null) {
+  if (widthType != null) {
    result[0] |= 1 << 6;
+    result[6] = widthType.index;


Since this is a binary value, it is possible to compress this into much smaller than the 32 bit int you are using now, but for now, this is OK. The benefit of what you have is that it is consistent and more readable. A more compressed version would be harder to read and maintain.

It looks like I'm going to remove widthType, so this won't be a problem anymore.

GaryQian · 2019-04-16T23:32:12Z

lib/ui/text.dart

    FontWeight fontWeight,
    FontStyle fontStyle,
    StrutStyle strutStyle,
+    TextWidthType widthType,


So since dart:ui is a more base level library, we may want to consider handling the switching between full and tight completely in the framework as an abstraction over dart:ui. At the dart:ui level, I think it makes more sense for both widths to be available in the getter for the width. Either make the width getter property take the enum or make a separate getter for the tight width.

The idea of innately possessing a tight or full width is more of a framework-level issue, and dart:ui (and below) shouldn't really have the paragraph preemptively pick its width as it would now with a setting in ParagraphStyle.

I think it makes sense for the setting to be in ParagraphStyle, but have the framework consult it or otherwise extract the widthType from the ParagraphStyle to choose with dart:ui method to call.

This would mean it isnt necessary to pass the widthType all the way down into Libtxt in the initial passing of ParagraphStyle into native.

GaryQian · 2019-04-16T23:35:27Z

third_party/txt/src/txt/paragraph.cc

              return a.code_units.start < b.code_units.start;
            });
+
+  if (paragraph_style_.width_type == TextWidthType::tight) {


(see above comments for context) LibTxt should be able to indiscriminately provide the metrics for both width types. By making the paragraph "choose" which width to provide, we are artificially hiding a potentially useful metric from the users of this quite low level API.

This hiding makes more sense if done completely in the framework level.

GaryQian · 2019-04-16T23:38:52Z

The implementation of the libtxt->dart:ui layers seems to be good though!

justinmc · 2019-04-17T17:18:16Z

@GaryQian Ready for re-review. Now I'm setting a textWidth property and reading that from the framework, so the engine doesn't care about TextWidthType anymore.

GaryQian · 2019-04-17T17:35:47Z

Implementation looks good. Do you think you can add a simple test to txt/tests/paragraph_unittests.cc?

GaryQian · 2019-04-17T17:39:40Z

lib/ui/text.dart

  double get height native 'Paragraph_height';

+  /// The distance from the left edge of the leftmost character to the right
+  /// edge of the rightmost character in the paragraph.


glyph may be a more precise word for this concept

GaryQian · 2019-04-17T17:40:23Z

third_party/txt/src/txt/paragraph.h

  double GetMaxWidth() const;

+  // Returns the tight width found in Layout(), which is defined as the
+  // horizontal distance from the left edge of the leftmost character to the


glyph (same as above)

GaryQian · 2019-04-17T22:34:58Z

third_party/txt/tests/paragraph_unittests.cc

+  // no line perfectly spans the width of the full line, so tight_width_ is less
+  // than width_.
+  ASSERT_DOUBLE_EQ(paragraph->width_, available_width);
+  ASSERT_TRUE(paragraph->tight_width_ < paragraph->width_);


You should be able to compare against either hardcoded values or the right edge of the right-most getRectsForRange box.

justinmc · 2019-04-17T23:01:51Z

@GaryQian I used getRectsForRange for the single line case because that's always just the distance between the left edge of the first box and the right edge of the last box. In the multiline case though, I would have to loop through all boxes and find the min and max values, which is just duplicating what the actual code does, so I hardcoded that one. Let me know if you've got a better idea for that.

GaryQian · 2019-04-17T23:05:41Z

Sounds good!

GaryQian

LGTM!

flutter/engine@06fea14...4b9966f git log 06fea14..4b9966f --no-merges --oneline 4b9966f Add an adjustment to the line width check in LineBreaker::addWordBreak (flutter/engine#8623) 60bb866 Roll src/third_party/skia a94670bd08cd..2c2240f66805 (2 commits) (flutter/engine#8632) a144f17 Tight Paragraph Width (flutter/engine#8530) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff (cbracken@google.com), and stop the roller if necessary.

FlashTang · 2025-02-19T15:05:53Z

Thank you so much , exactly what I need. I almost gave up on flutter before seeing this.
but what about Textfield ?

justinmc added the Work in progress (WIP) Not ready (yet) for review! label Apr 10, 2019

justinmc self-assigned this Apr 10, 2019

googlebot added the cla: yes label Apr 10, 2019

Tightly set paragraph width

cd7eb21

justinmc force-pushed the text-wrap-width branch from ddfb0d7 to 54e347c Compare April 12, 2019 00:33

MVP for a widthType flag that allows tight paragraph width

5de3316

justinmc force-pushed the text-wrap-width branch from 54e347c to 5de3316 Compare April 12, 2019 22:38

Documentation for TextWidthType enum

2d0ef41

justinmc mentioned this pull request Apr 12, 2019

Tight Paragraph Width flutter/flutter#30988

Merged

Fix code style problem

e4d7bd0

justinmc changed the title ~~WIP Tight Paragraph Width~~ Tight Paragraph Width Apr 15, 2019

justinmc removed the Work in progress (WIP) Not ready (yet) for review! label Apr 15, 2019

justinmc requested review from GaryQian and HansMuller April 15, 2019 18:28

GaryQian reviewed Apr 16, 2019

View reviewed changes

Remove widthType enum stuff in engine to prepare for new approach

1941f16

justinmc force-pushed the text-wrap-width branch from a17a9b7 to 1941f16 Compare April 17, 2019 16:21

Set tightWidth so framework can read it

b9403e5

GaryQian reviewed Apr 17, 2019

View reviewed changes

justinmc added 2 commits April 17, 2019 14:57

character => glyph

d173aa4

Assertion for tight_width_ in unit tests

8f68d3b

justinmc force-pushed the text-wrap-width branch from 666e555 to 8f68d3b Compare April 17, 2019 22:33

GaryQian reviewed Apr 17, 2019

View reviewed changes

More specific tight_width_ tests

78f8b6a

GaryQian approved these changes Apr 17, 2019

View reviewed changes

Fix code style problems

b9ae99d

justinmc merged commit a144f17 into flutter:master Apr 18, 2019

justinmc deleted the text-wrap-width branch April 18, 2019 16:08

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Apr 18, 2019

a144f17 Tight Paragraph Width (flutter/engine#8530)

227e241

engine-flutter-autoroll mentioned this pull request Apr 18, 2019

Roll engine 06fea14e5800..4b9966f5cb41 (3 commits) flutter/flutter#31271

Merged

justinmc mentioned this pull request Apr 23, 2019

Rename tightWidth to longestLine #8706

Merged

Tight Paragraph Width #8530

Tight Paragraph Width #8530

Uh oh!

Conversation

justinmc commented Apr 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Solution

Concerns

Uh oh!

justinmc commented Apr 10, 2019

Uh oh!

justinmc commented Apr 12, 2019

Uh oh!

justinmc commented Apr 15, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GaryQian commented Apr 16, 2019

Uh oh!

justinmc commented Apr 17, 2019

Uh oh!

GaryQian commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justinmc commented Apr 17, 2019

Uh oh!

GaryQian commented Apr 17, 2019

Uh oh!

GaryQian left a comment

Choose a reason for hiding this comment

Uh oh!

FlashTang commented Feb 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

justinmc commented Apr 10, 2019 •

edited

Loading

GaryQian Apr 16, 2019 •

edited

Loading