feat(Text): condensed styles structure v6

[melchiar]

Styles structure rewrite for v6 (ported from #7842)

[ShaMan123]

Looks good

[ShaMan123]

Why are unit tests failing?

[github-actions]

Code Coverage Summary

> [email protected] coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.26 |    76.24 |   85.78 |   82.98 |                                               
 fabric.js |   83.26 |    76.24 |   85.78 |   82.98 | ...,30922,30996,31007-31072,31195,31294,31530 
-----------|---------|----------|---------|---------|-----------------------------------------------

[melchiar]

Why are unit tests failing?

Just fixed the unit/textbox.js test. Don't know why the firefox visual test is failing but it seems unrelated to this pr.

[ShaMan123]

It does that all the time. The unit tests were what I intended to ask about.

[melchiar]

@asturur are we okay to merge this?

[ShaMan123]

conflicts @melchiar

conflicts

[ShaMan123]

simple conflicts as I see it. All should be resolved to the master tag

[github-actions]

Code Coverage Summary

> [email protected] coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.24 |    76.19 |    85.7 |   82.96 |                                               
 fabric.js |   83.24 |    76.19 |    85.7 |   82.96 | ...,31048,31122,31133-31198,31321,31420,31656 
-----------|---------|----------|---------|---------|-----------------------------------------------

[melchiar]

simple conflicts as I see it. All should be resolved to the master tag

should be good now

[ShaMan123]

I think we should use Object.assign in all fromObject methods so we don't mutate the arg

[melchiar]

I think we should use Object.assign in all fromObject methods so we don't mutate the arg

Object.assign is already used in the stylesFromArray method though (line 1300 in src/util/misc.js). Where exactly are you seeing a risk of mutation?

[github-actions]

Code Coverage Summary

> [email protected] coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.25 |    76.22 |    85.7 |   82.97 |                                               
 fabric.js |   83.25 |    76.22 |    85.7 |   82.97 | ...,31048,31122,31133-31198,31321,31420,31656 
-----------|---------|----------|---------|---------|-----------------------------------------------

[github-actions]

Code Coverage Summary

> [email protected] coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.26 |    76.22 |    85.7 |   82.97 |                                               
 fabric.js |   83.26 |    76.22 |    85.7 |   82.97 | ...,31052,31126,31137-31202,31325,31424,31660 
-----------|---------|----------|---------|---------|-----------------------------------------------

[ShaMan123]

@melchiar I do not understand the tests... Something seems off

[asturur]

i think we need to solve the mistery of line 193 and 217 in unit/text.js
those assignments shouldn't be there.
Weren't needed before, shouldn't be needed now

[github-actions]

Code Coverage Summary

> [email protected] coverage:report
> nyc report --reporter=lcov --reporter=text

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.25 |    76.19 |    85.7 |   82.96 |                                               
 fabric.js |   83.25 |    76.19 |    85.7 |   82.96 | ...,31052,31126,31137-31202,31325,31424,31660 
-----------|---------|----------|---------|---------|-----------------------------------------------

[melchiar]

i think we need to solve the mistery of line 193 and 217 in unit/text.js those assignments shouldn't be there. Weren't needed before, shouldn't be needed now

Should be good now. Turns out that a unit test issue I had tried to "fix" was actually related to the mutation bug that shachar noticed.

[ShaMan123]

Looks good.
I would like to ask a final question from a previous discussion but I think it's important.

let data// has styles array set
fabric.Text.fromObject(data) // respects styles
new fabric.Text(data) // doesn't respect styles

Am I right? I don't like that. I think it should respect both use cases
It's a design issue that lacks in fabric

[asturur]

I don't think that is a real issue, exactly as you can't instantiate an obejct with a serialized pattern, gradient or element.
The serailization has a shape and the object options/data a different one.
FromObject is there to fill the gap.

You don't exepct an js object to be initialized with its JSON.stringify form without passing into JSON.parse, and this is exactly the same

[ShaMan123]

Well I still dislike it.
Fabric has too many unintuitive functions you can call only in a certain way that beg to be called in other contexts.
It makes for bad DX and a feel that the repo is broken. Though it is not.
Bad PR for fabric IMO - to many dos and don'ts and cases and allowed combinations and disallowed combinations. e.g. you can't set path on path directly but if you do you must call _setPositionDimesions. Now styles is part of this notorious gang.

[melchiar]

I don't really have a strong opinion on it either way.

I agree we should be trying to make the dev experience as intuitive as possible and not force a heavy reliance on docs to show the correct way to call each method. I also feel though that methods like new fabric.Text are better suited to creating basic objects that inherit an app's default properties, while fromObject is the better and more logical method to use when deserializing previously created objects and already have complete properties. If we're going to try to make both approaches equally flexible then we would also need to fix the other things lacking in the new fabric. approach as Andrea mentioned.

[asturur]

@ShaMan123 is an issue with serialization, toObject and toJSON are called when you have to convert objects to strings to store them somewhere.
Those are strings now and not functional pieces of code.
Is the same for gradients, patterns, and rects.
You can't add to canvas a rect that has been serialized right? and that feels normal.

If you call toObject on something, to use it again you have to call fromObject.

[ShaMan123]

I understand your point but I still think it's too fragile.
But let's move forward

[asturur]

To move forward please dismiss your request for changes that have been addressed.
If you don't want to expliitly approve that is fine, but at least remove the change requested flag