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

Don't wrap long words #14

Closed
MarcelGarus opened this issue Apr 12, 2019 · 12 comments
Closed

Don't wrap long words #14

MarcelGarus opened this issue Apr 12, 2019 · 12 comments
Labels
enhancement New feature or request

Comments

@MarcelGarus
Copy link

MarcelGarus commented Apr 12, 2019

Problem
When trying to display text that contains long words using the AutoSizeText widget, it can get wrapped if the available space is small. Perhaps there are some places where this is the intended effect, but in most cases, it's not. Design-wise, this is very dangerous because often, apps display data that's unknown at compile-time, so most developers probably aren't even aware of this behavior.
That's why I propose to present an option to the developer (perhaps a wrapWords in the constructor) that lets the user opt-in to word-wrapping, making not wrapping words the default.

The technical side
So how is it possible to not wrap words?
Let's suppose you calculated a seemingly fitting text size. The following is one possible way of making sure the text doesn't wrap:

  1. Break the text into separate lines by replacing every section of whitespaces with newlines (\n).
  2. Create a TextPainter for the text, setting the maxLines property to the number of words.
  3. Measure the text again with the incoming width.
  4. If the text didExceedMaxLines, a word got wrapped! Otherwise, we're good.
  5. If a word got wrapped, try the same for a smaller text.

Possible concerns
The only possible contra argument that comes to my mind is that the widget is less performing. But as the widget isn't heavily optimized for performance yet (i.e. linear search for the text size is used instead of binary search), and the decision making for the text size doesn't happen often, I'm sure it's a trade-off worth making.

Implementation
So, how do you feel about this proposal?
If you do agree that this would be a nice feature to have, would you like to implement it or should I do so and then, file a pull request? (Could take some time, I'm currently quite busy.)

@simc
Copy link
Owner

simc commented Apr 12, 2019

I really like your proposal. Also the implementation idea sounds good. The only thing I'm not sure about is if it should be the default behavior or not.

My thinking about this:
Pros:

  • intended behavior in most cases

Cons:

  • can produce unexpected results in corner cases
  • can have a performance impact
  • different behavior than Text widget

Maybe we should also add a second parameter to configure wrapping very long words despite wrapWords = false (e.g. maxWordLengthWithoutWrapping)

I've been thinking about improving performance of AutoSizeText for a while now and there might be an even better solution than binary search but I haven't tested it yet.

@simc simc added the enhancement New feature or request label Apr 12, 2019
@MarcelGarus
Copy link
Author

Oh, of course, you're right, I didn't consider that the AutoSizeText should be a drop-in replacement for the original Text widget. Hmm, in that case opting in to avoid wrapping words seems like a good idea, so I agree with you on that.

I'm not sure about wrapping very long words, because of how arbitrary the definition would need to be. Like, do you determine the length using the number of letters or the actual, rendered length? And for a given length, when exactly is the word considered very long and when not? There's a lot of questions like those and a deterministic widget behavior should be our first priority.

That's why I believe a simple wrapWords constructor parameter with a default value of true is the best option here.

About the performance impact: Let's assume text layout has a cost of O(1) (which it does not, but let's ignore that for a moment). Given an array of n possible font sizes, the current linear search implementation takes O(n) to search, while a binary search solution would yield O(log n) performance. Checking for the wrapping of words doesn't change the performance of the widget on an order of magnitude in either of those cases.
Of course, the layout time would double in the worst case, but given how fast the Flutter framework itself is and how aggressively it caches, I'd argue it isn't noticeable in any real-world scenario.

@simc
Copy link
Owner

simc commented Apr 14, 2019

Okay, so I'll start implementing this feature. Let's see how it works in real world scenarios and if we need additional options to refine it.
I agree that the performance won't be a problem in the vast majority of cases. For very long texts where it might hit performance, AutoSizeText is probably not the best option anyway.

@simc simc added the wip This issue is currently being implemented label Apr 14, 2019
@MarcelGarus
Copy link
Author

Okay great, looking forward to it. 👍

@kevin-haynie
Copy link

I have a related problem with your package. Single long words get wrapped even after the largest font size is determined. I believe your check for max font size needs to consider if any of the words are so long that they don't fit on a single line....

@simc
Copy link
Owner

simc commented May 14, 2019

I still work on this feature. There should be a test version soon...

@kevin-haynie
Copy link

kevin-haynie commented May 14, 2019

One tweak that I made that you might want to include is to ensure that maxLines <= num words:

bool checkTextFits(TextSpan text, Locale locale, double scale, int maxLines, double maxWidth, double maxHeight) {
  // knh 5/8/19
  maxLines = maxLines.clamp(1, text.text.split(RegExp('\\s+')).length);

  var tp = TextPainter(
    text: text,
    textAlign: TextAlign.left,
    textDirection: TextDirection.ltr,
    textScaleFactor: scale ?? 1,
    ellipsis: '.',
    maxLines: maxLines,
    locale: locale,
  );

  tp.layout(maxWidth: maxWidth);

  return !(tp.didExceedMaxLines || tp.height > maxHeight || tp.width > maxWidth);
}

@simc
Copy link
Owner

simc commented May 14, 2019

@kevin-haynie Thanks, I'll include that in the new version...

@simc
Copy link
Owner

simc commented May 15, 2019

I published a new dev version on pub (https://pub.dev/packages/auto_size_text/versions/2.0.0-dev).

You can disable word wrapping with wrapWords: false.

Please open another issue if there are any problems with the new version.

@simc simc closed this as completed May 15, 2019
@simc simc removed the wip This issue is currently being implemented label May 15, 2019
@kras95
Copy link

kras95 commented Mar 11, 2020

I think that wrapWords should be false by default, because it gave me pain trying to figure out why my words were jumping to new rows for a while.

@MarcelGarus
Copy link
Author

That is totally true and was indeed the intended functionality to be implemented:

Oh, of course, you're right, I didn't consider that the AutoSizeText should be a drop-in replacement for the original Text widget. Hmm, in that case opting in to avoid wrapping words seems like a good idea, so I agree with you on that.

@jbryanh
Copy link

jbryanh commented Oct 28, 2021

I'll create an issue for this as well. I'm not sure I understand what the normal Text Widget line breaking behavior is, but I haven't seen it break a word like "word" into a "wor" on one line and then the "d" below alone....which is what the default behavior for AutoSizeText does unless you over-ride the default wrapWords.

I feel like human behavior is to create rules for the exceptions or "on principle" even when it goes against prevailing or normal expected behavior. I'd propose defaulting wrapWords to false because I estimate that 99% of use cases prefer it.

I'd add that 50% of plugin users probably abandon the plugin mid-understanding without fully investigating the strange behavior out of the box.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants