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.

Sign up for GitHub

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

Jump to bottom

Fix(fabric.Text): Avoid reiterating measurements when width is 0 and measure also empty lines for consistency. #7497

Merged
merged 2 commits into from
Nov 25, 2021
Merged

Fix(fabric.Text): Avoid reiterating measurements when width is 0 and measure also empty lines for consistency. #7497

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcc6e40
Fix what appear to be bugs in some of the text cache checks
avrami-ly Nov 18, 2021
ac14883
Remove check for line === '', if this check passes, _charBounds can b…
avrami-ly Nov 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 3 additions & 10 deletions src/shapes/text.class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1343,19 +1343,12 @@
* @return {Number} Line width
*/
getLineWidth: function(lineIndex) {
if (this.__lineWidths[lineIndex]) {
if (this.__lineWidths[lineIndex] !== undefined) {
avra-m3 marked this conversation as resolved.
Show resolved Hide resolved
return this.__lineWidths[lineIndex];
}

var width, line = this._textLines[lineIndex], lineInfo;

if (line === '') {
width = 0;
}
else {
lineInfo = this.measureLine(lineIndex);
width = lineInfo.width;
}
var lineInfo = this.measureLine(lineIndex);
Copy link
Member

Choose a reason for hiding this comment

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

does measureLine have the same check inside?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not as far as I have seen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When I stepped through line by line, I got as far as this._measureChar with an empty string.

Copy link
Member

Choose a reason for hiding this comment

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

So why we want to skip the shortcut and go far down with the empty line?

Copy link
Member

Choose a reason for hiding this comment

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

I m happy about the fix, i want to understand why we also remove the shortcut. Is not of immediate understanding to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Originally I fixed the shortcut, it caused an error when a line consisted solely of a blank space because _charBounds[x] is undefined.

My understanding is that this._measureLine sets _charBounds for each line so we shouldn't skip it otherwise other parts of the code will fail :(.

We could restore the check but currently it's the equivalent of if(false) because [''] will never equal '',

Fixing it to act as a shortcut is probably possible, I'd need to investigate further and may have consequences when it comes to how different platforms measure blank newlines?.

Copy link
Member

Choose a reason for hiding this comment

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

no is ok as it is.
The fact that skipping measurements was creating error elsewhere is good to me.

var width = lineInfo.width;
this.__lineWidths[lineIndex] = width;
return width;
},
Expand Down