feat(Text): condensed styles structure

[melchiar]

This condenses text styles into a better optimized structure at serialization, and expands the structure again at initialization. The new structure uses ranges rather than having a style object for each character, resulting in significantly smaller style strings when the same style is used for a range of characters.

Here's a comparison for a textbox with 110 characters and 2 unique styles.

Current styles structure:

'{"0":{"0":{"fill":"red"},"1":{"fill":"red"},"2":{"fill":"red"},"3":{"fill":"red"},"4":{"fill":"red"},"5":{"fill":"red"},"6":{"fill":"red"},"7":{"fill":"red"},"8":{"fill":"red"},"9":{"fill":"red"},"10":{"fill":"red"},"11":{"fill":"red"},"12":{"fill":"red"},"13":{"fill":"red"},"14":{"fill":"red"},"15":{"fill":"red"},"16":{"fill":"red"},"17":{"fill":"red"},"18":{"fill":"red"},"19":{"fill":"red"},"20":{"fill":"red"},"21":{"fill":"red"},"22":{"fill":"red"},"23":{"fill":"red"},"24":{"fill":"red"},"25":{"fill":"red"},"26":{"fill":"red"},"27":{"fill":"red"},"28":{"fill":"red"},"29":{"fill":"red"},"30":{"fill":"red"},"31":{"fill":"red"},"32":{"fill":"red"},"33":{"fill":"red"},"34":{"fill":"red"},"35":{"fill":"red"},"36":{"fill":"red"},"37":{"fill":"red"},"38":{"fill":"red"},"39":{"fill":"red"},"40":{"fill":"red"},"41":{"fill":"red"},"42":{"fill":"red"},"43":{"fill":"red"},"44":{"fill":"red"},"45":{"fill":"red"},"46":{"fill":"red"},"47":{"fill":"red"},"48":{"fill":"red"},"49":{"fill":"red"},"50":{"fill":"red"},"51":{"fill":"red"},"52":{"fill":"red"},"54":{"fill":"blue"},"55":{"fill":"blue"},"56":{"fill":"blue"},"57":{"fill":"blue"},"58":{"fill":"blue"},"59":{"fill":"blue"},"60":{"fill":"blue"},"61":{"fill":"blue"},"62":{"fill":"blue"},"63":{"fill":"blue"},"64":{"fill":"blue"},"65":{"fill":"blue"},"66":{"fill":"blue"},"67":{"fill":"blue"},"68":{"fill":"blue"},"69":{"fill":"blue"},"70":{"fill":"blue"},"71":{"fill":"blue"},"72":{"fill":"blue"},"73":{"fill":"blue"},"74":{"fill":"blue"},"75":{"fill":"blue"},"76":{"fill":"blue"},"77":{"fill":"blue"},"78":{"fill":"blue"},"79":{"fill":"blue"},"80":{"fill":"blue"},"81":{"fill":"blue"},"82":{"fill":"blue"},"83":{"fill":"blue"},"84":{"fill":"blue"},"85":{"fill":"blue"},"86":{"fill":"blue"},"87":{"fill":"blue"},"88":{"fill":"blue"},"89":{"fill":"blue"},"90":{"fill":"blue"},"91":{"fill":"blue"},"92":{"fill":"blue"},"93":{"fill":"blue"},"94":{"fill":"blue"},"95":{"fill":"blue"},"96":{"fill":"blue"},"97":{"fill":"blue"},"98":{"fill":"blue"},"99":{"fill":"blue"},"100":{"fill":"blue"},"101":{"fill":"blue"},"102":{"fill":"blue"},"103":{"fill":"blue"},"104":{"fill":"blue"},"105":{"fill":"blue"},"106":{"fill":"blue"},"107":{"fill":"blue"},"108":{"fill":"blue"},"109":{"fill":"blue"}}}'

Condensed styles structure

'[{"start":0,"end":53,"style":{"fill":"red"}},{"start":54,"end":110,"style":{"fill":"blue"}}]'

[melchiar]

@asturur, @ShaMan123 I still need to fix the tests, but if you could take a look a look and let me know your thoughts on this approach that'd be appreciated.

[ShaMan123]

@melchiar why not migrate all logic to your new approach?
It seems better

[melchiar]

@melchiar why not migrate all logic to your new approach? It seems better

Ultimately it came down to complexity and performance.

The current structure, while more verbose, is fast since it uses line numbers as keys so the style for each line can be accessed directly. The new structure uses an array so finding the style for a specific character means iterating through the style groups looking for that index. Since styles are in ranges, changing the style for a subset of characters means going through and modifying any overlapping groups. Every text change made would also require going through every style collection in the array to update the index numbers.

By using this structure only for serialization we avoid these issues, and also avoid a major breaking change since this approach is backwards compatible.

Thoughts?

[ShaMan123]

Sounds solid to me. Winning on all fronts. perf in runtime and storage size after serializing.

[ShaMan123]

What do think about renaming? So it's clearer what the methods do.
Something like getExportedStyles/stylesToObject/serializeStyles/inflateStyles and their counterparts..

[melchiar]

What do think about renaming? So it's clearer what the methods do. Something like getExportedStyles/stylesToObject/serializeStyles/inflateStyles and their counterparts..

Yeah, I had trouble with the names. I initially went with compressStyles/decompressStyles but that's not really accurate since there is no compression. I think getCondensedStyles is pretty clear, but there isn't really a good antonym for condense so I just went with getExpandedStyles. Not perfect I agree. The methods only return the new styles though and don't actually assign them so I do think they should be named "get...something" (this was intentional so that condensed styles can easily be obtained without modifying the object).

[ShaMan123]

My point is that the purpose of getting a condensed style is unclear and that's why I think it should be renamed so that a dev seeing it for the first time will have a feeling when to call it (preferably without reading the JSDOC description)

[melchiar]

Fair point. Do you think getStylesObject/getStylesArray would work?

[ShaMan123]

How about toStyleObject/fromStyleObject?

[melchiar]

or maybe stylesToArray/stylesFromArray?

[ShaMan123]

yeah, those are fine

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.35 |    77.18 |   86.28 |   83.06 |                                               
 fabric.js |   83.35 |    77.18 |   86.28 |   83.06 | ...,30303,30428,30508-30573,30696,30795,31012 
-----------|---------|----------|---------|---------|-----------------------------------------------

[ShaMan123]

Seems good apart from a few minor changes (see comments) and a test that needs to be added

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.4 |    77.31 |   86.27 |   83.11 |                                               
 fabric.js |    83.4 |    77.31 |   86.27 |   83.11 | ...,30300,30425,30505-30570,30693,30792,31009 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.4 |    77.33 |   86.27 |   83.11 |                                               
 fabric.js |    83.4 |    77.33 |   86.27 |   83.11 | ...,30300,30425,30505-30570,30693,30792,31009 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |    83.4 |    77.31 |   86.27 |   83.11 |                                               
 fabric.js |    83.4 |    77.31 |   86.27 |   83.11 | ...,30300,30425,30505-30570,30693,30792,31009 
-----------|---------|----------|---------|---------|-----------------------------------------------

[melchiar]

Isn't there another property that can be used? And if not maybe use _reNewline? Maybe make the data available at _splitTextIntoLines?

Splitting by _reNewline would make sense since it seems linebreaks can sometimes appear as \r\n. Some more refactoring would be order though since _reNewline is a property of fabric.Text, while we've moved the new methods over to utils. We could just use the lines prop of the _splitTextIntoLines method in fabric.Text (the _splitTextIntoLines of fabric.Textbox wouldn't work here since it separates lines with autowrapping), though calling _splitTextIntoLines would slow performance since it does a whole bunch of unnecessary stuff compared to just using the split() approach.

I'd be inclined to make _reNewline a property of the fabric namespace and then just call split(fabric._reNewLine) in _stylesToArray. This seems to make the most sense from a performance standpoint.

[ShaMan123]

We could just use the lines prop of the _splitTextIntoLines method in fabric.Text (the _splitTextIntoLines of fabric.Textbox wouldn't work here since it separates lines with autowrapping), though calling _splitTextIntoLines would slow performance since it does a whole bunch of unnecessary stuff compared to just using the split() approach.

I don't understand why it would slow down things.
It's called anyways initialize > initDimensions > _splitText > _splitTextIntoLines both in Text and Textbox.

fabric.js/src/shapes/text.class.js

Line 433 in 887a745

this._splitText();

fabric.js/src/shapes/textbox.class.js

Line 97 in 887a745

this._styleMap = this._generateStyleMap(this._splitText());

[ShaMan123]

and maybe _generateStyleMap is now redundant

[melchiar]

I don't understand why it would slow down things. It's called anyways initialize > initDimensions > _splitText > _splitTextIntoLines both in Text and Textbox.

We could move _splitText() from initDimensions() to initialize(), then pass the textLines property as an argument to the stylesToArray method. Doing that though would defeat the purpose of making stylesToArray a util since you'd still need a text instance to call _splitText() on to send to the util.

Also, the textbox class still presents problems since the _splitTextIntoLines method of textbox performs wrapping.

[asturur]

regargin the splitting text in the serialization, yes apart customization we need to check how we split lines in general.
If i remember correctly textbox splits differently but then has a map to go back to normal textlines in itext mode.
That would mean that how itext breaks line is always good, maybe we can just call that method instead of using the split.

Also maybe is possible to change the runtime format in something that is a single long array instead of an array of array.
That would make inserting and deleting easier, and we can track which is the start index of each line easily,

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.42 |    77.35 |   86.27 |   83.14 |                                               
 fabric.js |   83.42 |    77.35 |   86.27 |   83.14 | ...,30302,30427,30507-30572,30695,30794,31011 
-----------|---------|----------|---------|---------|-----------------------------------------------

[melchiar]

regargin the splitting text in the serialization, yes apart customization we need to check how we split lines in general. If i remember correctly textbox splits differently but then has a map to go back to normal textlines in itext mode. That would mean that how itext breaks line is always good, maybe we can just call that method instead of using the split.

@asturur The would work, though I've currently got the stylesFromArray/stylesToArray methods as utils rather than attached to an instance. Would you suggest we attach those methods back to the text class so that the _splitText method can be called on the instance?

[ShaMan123]

If it's meant to be used in fromObject it can't be attached to instance.

[melchiar]

@asturur Just wanted to follow up on this. As @ShaMan123 has pointed out, the new methods can't be called from fromObject if they're attached to an instance. Doesn't keeping to the current method make the most sense?

[ShaMan123]

I agree with @asturur. However because text splitting occurs after initialization I agree with you as well.

[melchiar]

@asturur do you have a minute to take a look at this?

[github-actions]

Code Coverage Summary

> fabric@5.2.1 coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.42 |    77.36 |   86.27 |   83.14 |                                               
 fabric.js |   83.42 |    77.36 |   86.27 |   83.14 | ...,30303,30428,30508-30573,30696,30795,31012 
-----------|---------|----------|---------|---------|-----------------------------------------------

[asturur]

I think this is fine, and also that we should advertise this change as much as possible and see if a boolean for compatibility is necessary for someone that may have been invested in the style analisys in the serialized objects.

Also as soon as we add the object spread operator let's remove that full clone so that we can clone the actual objects we make copy of in the toArray util.

all changes addressed