Fix infinite loop on drawing of large labels

[wimrijnders]

Fix for #3171 and #3185.

• Fixed the infinite loop which can occur in specific case when drawing node labels.
• Labels with one huge word longer than width constraint are now broken into multiple lines.

This can lock up Network due to the infinite loop, so I feel that the PRIORITY label is in order here. You can't have that happening in cases that users would regard as 'normal usage'.

The actual fix for the issue (see above) is the code in function splitStringIntoLines().

---

Changes:

• Added new local class LabelAccumulator to Label.js. This moves away recurring actions within the main loops of the label handling.
• Moved code specific to splitting up label text to new method Label._processLabelText()
• Added unit tests for this method
• Added unit tests for proper handling of html and markdown tags
• Added unit tests for checking proper handling of widthConstraint.maximum.
• Added commenting, notes and TODO's where appropriate.

[wimrijnders]

Some extra info

I'm afraid that this fix is going to be pretty tough to review, if you're relying on diffing.
The code in question has changed significantly; it was pretty convoluted, and I found myself cleaning it up and refactoring just to understand it. It was not my intention at all to make such a drastic change, but I believe it is for the better.

The goal of this refactoring is to make the code for the label splitting clearer.
This does not mean it has become simpler, but the code has been rearranged to make it more understandable.

I have added unit tests to be sure that all is working as expected. This also tests the markup and tags for when using multi-fonts. The unit tests may be expanded in the future.

[jmvenancio]

I started testing and found a problem already :( @wimrijnders

https://i.gyazo.com/6e9c441b4045c43a780ab2861a836458.mp4

[jmvenancio]

It only happens after the zoom exceed 1.2

[wimrijnders]

@jmvenancio What exactly is the problem? What I see is that the particular subject of this fix works as expected.

Perhaps you mean the 'flash' that happens when the node height is larger than the view height? And the whole view turns the color of the node? I would call this a separate issue, and to be honest not a very critical one. But perhaps you differ in opinion of this.

[jmvenancio]

@wimrijnders you're right. It is a different issue. Although it still does not work as expected, because on that particular case the width constraint is broken. And of course that is not a critical issue, I will continue testing but I'll mention this problem on the review.

Thank you for all your work!!

[jmvenancio]

There is a new issue that was generated with this fix. I could not find any infinite loops and the text is correctly disposed inside the nodes. Although I think this can be accepted!
https://i.gyazo.com/6e9c441b4045c43a780ab2861a836458.mp4

Just found another problem, I'm not sure if this is ready to be merged like this.

[wimrijnders]

you're right. It is a different issue.

@jmvenancio If it really bothers you, please make a new issue out of it.

As for the second issue, you mean the overflow in the big rectangle on the left, right?
It looks like there's a max height constraint on that rectangle. I only handled the width in this fix, and my thought is that then the overflow is a separate issue. If it's not due to a max height constraint, please show me example code that produced that.

I have to say you're very much stress-testing this fix. I would also say that that is a good thing.

[jmvenancio]

I am not really sure it is only about the height constraint. Notice on the left green box, the "a a a" goes out of the rectangle, on the other hand on the right green box the "bolas bolas" does not go out and it has a superior height. And I set the maximum height to the height of the small rectangles so, maybe two issues should be created.

In my specific case this fix isn´t enough since I really want all the nodes to have the same size. Also I think that the maximum height constraint should always be respected, much like width constraint is with this fix... And maybe this is another issue...

[wimrijnders]

@jmvenancio Could you give me the code that made the previous screen dump? I'd like to examine that in more detail.

[yotamberk]

@wimrijnders what's the status for this PR? Can it be reviewed?

[wimrijnders]

@yotamberk Yes, it can definitely be reviewed. The core issue here is the fix on infinite loop. The discussion here is about an edge case with respect to layout.

@jmvenancio Could you open a new issue, please, for the overflowing of the big labels? I think it is worthwhile to pursue this, but not in the current PR. Thanks.

[Maniae]

Maintainers Note: This comment and resulting discussion moved to separate issue #3464.

I experienced crashes thrown from LabelAccumulator:

Uncaught TypeError: Cannot read property 'blocks' of undefined
at LabelAccumulator.finalize (Label.js:117)
at Label._processLabelText (Label.js:1200)
at Label._processLabel (Label.js:1212)
at Label.getTextSize (Label.js:656)
at Box.getDimensionsFromLabel (NodeBase.js:280)
at Box.resize (Box.js:29)
at Box._updateBoundingBox (NodeBase.js:236)
at Box.updateBoundingBox (Box.js:69)
at Box.draw (Box.js:55)
at Node.draw (Node.js:504)

This is happening every time a label, or a new line of the label, starts with a blank character followed by a word too large for the label width.
Also thrown when using two LF characters in a label. ("\n\n" throws the error, but "\r\n\r\n" doesn't)