fix(Textbox): minWidth of text, measure lines before wrapping#8994
fix(Textbox): minWidth of text, measure lines before wrapping#8994ShaMan123 wants to merge 28 commits into
Conversation
|
Build Stats
|
ShaMan123
left a comment
ShaMan123
left a comment
There was a problem hiding this comment.
stable and clean
ShaMan123
left a comment
ShaMan123
left a comment
There was a problem hiding this comment.
DONE DONE DONE!
| if (i === 0 || lineWidthAfter - additionalSpace > maxWidth) { | ||
| // push a new line, we spread to protect `data` from mutation | ||
| lines.push([...graphemes]); | ||
| lineWidth = width; |
There was a problem hiding this comment.
| lineWidth = width; | |
| lineWidth = infixWidth + width; |
If someone decides to override measureLineForWrapping and return an infix we should respect it.
something like:
some words some words some words some words
some words some words ....
a line with an infix at the start
There was a problem hiding this comment.
and then we should push the infix if it exists
There was a problem hiding this comment.
what do you mean? an infix in this case is just something between parts of text. and is either nothing or a space right now.
measureLines shouldn't change the lines imho, if someone does that is going on its own road with no guarantee of support
There was a problem hiding this comment.
Yes, let's abandon this idea.
measureLines shouldn't change the lines imho, if someone does that is going on its own road with no guarantee of support
I don't understand this
|
I now understand what I dislike about text logic. |
In an ideal world measurements would also be cached, though that mean adding cache invalidation and a lot more work |
|
Not really, |
|
I guess the real question then is how much of a performance hit measuring actually causes. Even if it's small these methods get called many times especially when there's lots of text |
|
horrible English, sorry: I don't think so. I was expecting |
Because charbounds has been added years after __lineWidth and __lineHeight already existed. Many PRs aren't merged because in order:
|
|
I understand. |
|
@ShaMan123 going back to the focus of merging this PR, there are some red flags for me. If the changes are required because you are splitting the class method not in half, but in many parts and taking some of them in a different order from the original, then we are not simply extracting some code in a method. |
yes Text is hard, is fragile and the main focus after 6.0 should be RTL and vertical text ( that you started long ago ), and proper emoji support. |
|
After extracting measuring the rest seems really simple/natural but I get your point. Please suggest a way forward for this PR. |
ShaMan123
changed the title
|
I have found and fixed a bug so now I have a good excuse to merge this |
ShaMan123
left a comment
ShaMan123
left a comment
There was a problem hiding this comment.
moved the measuring logic and managed to fix a bug due to it
| if (minWidth + reservedSpace > this.dynamicMinWidth) { | ||
| this.dynamicMinWidth = minWidth - additionalSpace + reservedSpace; | ||
| } |
There was a problem hiding this comment.
this part I don't understand.
When I do I can make this and maxWidth a bit more tidy and add some comments
There was a problem hiding this comment.
if i remember correctly maxWidth is the largest space we have at our disposal.
The larges between what we want the texbox to be, or the size of the smallest word.
| return value.split(this._wordJoiners); | ||
| } | ||
|
|
||
| measureLinesForWrapping(lines: string[]) { |
There was a problem hiding this comment.
I don't like the naming
| infix: string | null; | ||
| infixWidth: number; | ||
| }[], | ||
| minWidth: 0, |
There was a problem hiding this comment.
not sure this is useful
| var line2 = textbox._wrapText([''], 100, 0); | ||
| assert.deepEqual(line2, [[]], 'wrapping with splitByGrapheme'); | ||
| }); | ||
| QUnit.test('wrapping respects max line width', function (assert) { |
|
If you don't like reduce and prefer for loops I will do that. |
| _wrapText(lines: string[], desiredWidth: number, reservedSpace = 0) { | ||
| const wrapped: string[][] = []; | ||
| this.isWrapping = true; | ||
| const { lines: data, minWidth } = this.measureLinesForWrapping(lines); |
There was a problem hiding this comment.
We should cache this and recalc only when text/styles change.
This will make resizing a lot faster
|
So i put the master code side to side with this PR an i can't understand wich part you extracted. It wrap a single line at time. the pr change more between last 2 days, and now is completely changing the process of text wrapping in order to early catch the minimum word width to avoid extra breaking in lines, since a long word on the last line could make you wrapping in the early lines not optimal. I wouldn't call it even a bug, but i understand we want to fix it. That is a bug and can be fixed, and that is the only part i understand of this PR. I can propose, let's see where the bugfix bring us, let's check again the need of dividing the measurement with the word measurement exposed when the bugfix is merged |
|
let me know i i understood the largest word width / min width bug |
|
You understand the width breaking line issue correctly |
|
Not sure I agree with closing |
3 participants
Motivation
#8990
Wrapping line logic is unreadable and very hard to trace back.
It is needed (by me at least) for overriding so it must be clear and scoped.
Description
minWidthshould be shared between lines.In case there is a text line that is long after a short one, the short line does not know it has more available size to grow to.
This is visible with the resizing control, see attached videos.
I suggest you read this PR commit by commit since they are self explanatory IMO, or not, you can simply read as is.
I consolidated all measuring logic (moved infix measuring to the measuring loop) and extracted it to
measureLinesForWrapping, now running before wrapping and not as part of wrapping.Besides that I cleaned up logic to be readable and to use as much consts and as less switches as possible.
Changes
No changes to functionality.
Exposed
measureLinesForWrappingBREAKING:
_wrapLinesignatureGist
In Action
Master
Parcel.Sandbox.-.Google.Chrome.2023-06-08.12-31-11_Trim.mp4
Fixed
Parcel.Sandbox.-.Google.Chrome.2023-06-08.12-36-28_Trim.mp4