feat(Group): 2nd Patch of New Group! 🎉 #7859

ShaMan123 · 2022-04-04T08:56:11Z

This PR makes nested selection work out of the box.

Group's selection observers are now almost completely redundant (d8f8251) if I am not mistaken. The only use I can think of is when active object is not active selection. Consider using canvas.getActiveObjects().includes(obj) instead.

Logic has been adapted and now active selection almost completely supports selecting nesting objects (multiple selection is handled by #7882 ) I believe the only thing from perfect impl is containsPoint

To Do - done on #7900

canEnter - feat(Group): 2nd Patch of New Group! 🎉 #7859 (comment)
- naming
- throwing or what? feat(Group): 2nd Patch of New Group! 🎉 #7859 (comment) Errors will raise in current code after warning is logged. Meaning it shouldn't warn, severity is error.
Group renderBackground - migrate to feat(Group): 2nd Patch of New Group! 🎉 #7859 (comment)
collection add/insertAt/remove return values
minor dead code in text.class feat(Group): 2nd Patch of New Group! 🎉 #7859 (comment) - this can be removed only if group set coords on it's object. The question is what would be less damaging to perf... leaving as is or setting coords on all nested objects? Or a third option would be for group to check if object is on screen before it renders it
containsPoint should be calculated relative to canvas coordinate plane
remove object monitors?

@asturur

agree on isInFrontOf
verify edge case with this._objectsToRender invalidation
find a different solution for _getTransformedDimensions, @asturur improve my understanding of the group layout needs for it

Changed Files

src/brushes/spray_brush.class.js - adapt to new group
src/canvas.class.js - return top most target including sub targets in case target is interactive
src/mixins/animation.mixin.js - fix animations to be relative to canvas
src/mixins/canvas_events.mixin.js - port missing logic from v6!
src/mixins/canvas_grouping.mixin.js - port missing logic from v6! accounting for nested objects
- make sure not to create a circular call when adding an object to active selection
- create active selection using isInFrontOf
src/mixins/eraser_brush.mixin.js - adapt to new group (follow up will be in a separate PR)
src/mixins/itext_behavior.mixin.js - add/remove handlers from canvas events (not group)
src/mixins/itext_click_behavior.mixin.js - enable text editing under group
src/mixins/object_ancestry.mixin.js - port logic from v6!
src/mixins/object_geometry.mixin.js - port logic from v6!
- account for nested objects (getCoords+ _getTransformedDimensions)
- expose methods on object that interact with canvas coordinate plane
src/mixins/object_stacking.mixin.js - exposed isInFrontOf to determine which object is in front of the other in the entire tree
src/shapes/active_selection.class.js - adapt to restore objects to their owning group
src/shapes/group.class.js - tidy code for easier subclassing + support backgroundColor
src/static_canvas.class.js
- pass target for added/removed events (used by IText)
- properly unset canvas to the entire object tree and not only to top most object
- center object in canvas plane even if it is nested
test/unit/group.js - fix failing test
test/unit/itext_click_behaviour.js - adapt tests for nested itext
test/unit/object.js
- test canvas added/removed event object
- isDescendantOf
- findCommonAncestors
- isInFrontOf
test/visual/group_layout.js - reactivate tests

Things Left Out

Layer - created a dedicated PR for it feat(Layer): 3rd Patch of Group Rewrite ♦️ #7860
src/mixins/canvas_grouping - I suspect it needs additional work. Committed some fixes and dumped the file from v6!.
Group#_renderObjects - we need to discuss this, opened dedicated PR feat(Group): render selected objects #7863
Caching - discussed in Group/Canvas Caching #7874

#7744 (review)

in case object is a collection

asturur · 2022-04-10T11:24:47Z

I'm out until the evening

have fun.

asturur · 2022-04-10T11:33:40Z

tasks:

before merging:

i need to figure out why isInFrontOf cant't reuse findCommonAncestors. it has to.
this riddle: https://github.com/fabricjs/fabric.js/pull/7859/files#r846734964 didn't help me understand.

after merging:

verify edge case with this._objectsToRender invalidation
solve the add/remove returned values inconsistency
find a different solution for _getTransformedDimensions, improve my understanding of the group layout needs for it

ShaMan123 · 2022-04-10T19:05:58Z

tasks:

before merging:

i need to figure out why isInFrontOf can't reuse findCommonAncestors. it has to. this riddle: https://github.com/fabricjs/fabric.js/pull/7859/files#r846734964 didn't help me understand.

You made me laugh aloud. A damn riddle it is. Sorry!
Look at the tests. It should cover all edge cases.

after merging:

add/remove returned values inconsistency

I have changed collection 3ec53b4
Stuck to what Array returns from push/splice
What should insertAt return? I think the new length, same as add.

ShaMan123 · 2022-04-10T19:29:38Z

The discussion is getting hectic.
I will update the description with a todo list

ShaMan123 · 2022-04-10T19:45:41Z

updated description

asturur · 2022-04-11T06:28:07Z

the only thing to do is me to dig isFrontOf, i have seen there are a lot of tests done so i can probably just use those to test my assumptions.
I will open issues for the other follow ups.

For add and remove, i just want to change so that they behave same, they return what they need to return, but they return the same thing. ( both the length, or the list of added remove objects, or both true / false )

ShaMan123 · 2022-04-11T06:40:17Z

I can now offer a mature explanation as to where your logic is missing.
Testing isInFrontOf needs the first fork of ancestors from the top (and then test indexOf of each ancestor). findCommonAncestors()[0] will return the parent holding the fork. Meaning it will not provide sufficient data for us to conclude anything about the fork. Only that it happens below this first common ancestor. We actually need to find the first non common ancestors to determine isInFrontOf

ShaMan123 · 2022-04-11T06:42:37Z

I am satisfied with the explanation. Are you?

asturur · 2022-04-19T20:07:53Z

i spent 30 minutes changing the function as i see it fit, then stuck hours to make the test pass. Something makes the deepEquals fails for me, even when those are identical.

asturur · 2022-04-19T20:10:14Z

I pushed up my branch discuss-common-ancenstor the idea is that i empowered findCommonAncestor to return useful data for solving isFronOf ( the rest of the ancestors fork rather than the indexes ). I m sure tomorrow fresh mind i can fix the tests. Now the test file is pretty much messed up.

ShaMan123 · 2022-04-21T15:29:33Z

@asturur can we merge this and then PR a dedicated change? I really want/need to move forward. And working between branches with merge conflicts is a horror.

I pushed up my branch discuss-common-ancenstor the idea is that i empowered findCommonAncestor to return useful data for solving isFronOf ( the rest of the ancestors fork rather than the indexes ). I m sure tomorrow fresh mind i can fix the tests. Now the test file is pretty much messed up.

ShaMan123 · 2022-04-22T08:28:17Z

opened #7900 for minors

ShaMan123 · 2022-04-26T05:59:05Z

@asturur I have encountered an inconsistency I would like to fix.
The question is who should conform to the other? Group or Canvas?

let obj;
obj.on('removed', opt => {
   obj.canvas // still referenced
   obj.group // not referenced
  let parent = opt.target  // group or canvas
})
canvas.add(obj);
canvas.remove(obj);
group.add(obj);
group.remove(obj);

ShaMan123 · 2022-04-26T08:19:14Z

because of the naming removed (e.g. as opposed to removed:before) I think we should dereference canvas.

…#7906)

github-actions · 2022-04-28T23:34:44Z

Code Coverage Summary

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

-----------|---------|----------|---------|---------|-----------------------------------------------
File       | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                             
-----------|---------|----------|---------|---------|-----------------------------------------------
All files  |   83.04 |    75.95 |    85.7 |   82.76 |                                               
 fabric.js |   83.04 |    75.95 |    85.7 |   82.76 | ...,30839,30913,30924-30989,31112,31211,31447 
-----------|---------|----------|---------|---------|-----------------------------------------------

asturur · 2022-04-28T23:38:12Z

i should be able to merge this tomorrow morning.

ShaMan123 · 2022-04-29T03:11:34Z

Great. Do you plan on merging #7900 first or right after?

asturur

I ll follow up with add/remove/insertAt ( maybe insertAt can be removed? ) consistency chnages and i need to check that getTransformedDimensions overrides.

asturur · 2022-04-29T22:30:50Z

Need to get a good run to the master branch, this is a first very large change also in behaviour

ShaMan123 · 2022-04-30T05:09:39Z

Excellent! A big step forward! Many thanks for your work Andrea.

ShaMan123 added 30 commits April 3, 2022 23:32

apply 7744 patch (no binaries)

1a191a7

apply 7744 patch (binaries)

26e8392

commit #7744 review

7d793b3

#7744 (review)

patch2

2708029

Update spray_brush.class.js

02a8ffa

enable text editing under group

0d3690f

migrate back to isDescendantOf

ff0346a

render bg

812f02a

fix(): clear canvas from entire object tree

053946c

in case object is a collection

fix(): center object in case object is nested

db76c44

Update eraser_brush.mixin.js

04aad5b

Update canvas.class.js

8c1c535

Update canvas_events.mixin.js

3fd4af2

lint

58e9be2

safeguard from adding group to itself

c2ef2f5

Update object_ancestry.mixin.js

f0145b5

patch tests from v6!

cb20b81

Update canvas_grouping.mixin.js

347f80d

Update canvas.class.js

a56d55e

Update group_layout.js

2a426e7

Update group_layout.js

5e3fc1f

remove redundant logic

2b1f680

Update canvas_grouping.mixin.js

1ad5cc8

fix(): calling sendPointToPlane

11c828f

Update group.class.js

b6fa3dc

refactor enterGroup

e9afc89

invalidate groups after active selection change

d8f8251

_shouldSetNestedCoords

c58a274

perf(): refrain from contains

c0f5571

Update canvas_events.mixin.js

9a223d0

ShaMan123 mentioned this pull request Apr 11, 2022

feat(Layer): 3rd Patch of Group Rewrite ♦️ #7860

Closed

ShaMan123 mentioned this pull request Apr 15, 2022

fix(ActiveSelection): 🔃 preserveObjectStacking 📌 #7878

Closed

1 task

ShaMan123 linked an issue Apr 15, 2022 that may be closed by this pull request

getBoundingRect of objects in an active selection weird #4778

Closed

ShaMan123 mentioned this pull request Apr 22, 2022

fix(Group): patch2 minors #7900

Closed

11 tasks

ShaMan123 requested a review from asturur April 22, 2022 08:29

ShaMan123 mentioned this pull request Apr 26, 2022

fix(canvas export): regression caused by safegurading #7907

Merged

refactor() change common ancenstors returned data to help isInFrontOf (…

97886a6

…#7906)

asturur approved these changes Apr 29, 2022

View reviewed changes

asturur merged commit fbb4581 into master Apr 29, 2022

ShaMan123 mentioned this pull request Jun 16, 2022

fix(Group): patch #7916 #8000

Merged

asturur deleted the v6-group-patch2 branch September 11, 2022 23:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Group): 2nd Patch of New Group! 🎉 #7859

feat(Group): 2nd Patch of New Group! 🎉 #7859

ShaMan123 commented Apr 4, 2022 •

edited

Loading

asturur commented Apr 10, 2022

asturur commented Apr 10, 2022

ShaMan123 commented Apr 10, 2022 •

edited

Loading

before merging:

after merging:

ShaMan123 commented Apr 10, 2022

ShaMan123 commented Apr 10, 2022

asturur commented Apr 11, 2022

ShaMan123 commented Apr 11, 2022 •

edited

Loading

ShaMan123 commented Apr 11, 2022

asturur commented Apr 19, 2022

asturur commented Apr 19, 2022

ShaMan123 commented Apr 21, 2022

ShaMan123 commented Apr 22, 2022

ShaMan123 commented Apr 26, 2022

ShaMan123 commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 28, 2022 •

edited

Loading

asturur commented Apr 28, 2022

ShaMan123 commented Apr 29, 2022

asturur left a comment

asturur commented Apr 29, 2022

ShaMan123 commented Apr 30, 2022 •

edited

Loading

feat(Group): 2nd Patch of New Group! 🎉 #7859

feat(Group): 2nd Patch of New Group! 🎉 #7859

Conversation

ShaMan123 commented Apr 4, 2022 • edited Loading

To Do - done on #7900

Changed Files

Things Left Out

asturur commented Apr 10, 2022

asturur commented Apr 10, 2022

before merging:

after merging:

ShaMan123 commented Apr 10, 2022 • edited Loading

before merging:

after merging:

ShaMan123 commented Apr 10, 2022

ShaMan123 commented Apr 10, 2022

asturur commented Apr 11, 2022

ShaMan123 commented Apr 11, 2022 • edited Loading

ShaMan123 commented Apr 11, 2022

asturur commented Apr 19, 2022

asturur commented Apr 19, 2022

ShaMan123 commented Apr 21, 2022

ShaMan123 commented Apr 22, 2022

ShaMan123 commented Apr 26, 2022

ShaMan123 commented Apr 26, 2022 • edited Loading

github-actions bot commented Apr 28, 2022 • edited Loading

Code Coverage Summary

asturur commented Apr 28, 2022

ShaMan123 commented Apr 29, 2022

asturur left a comment

Choose a reason for hiding this comment

asturur commented Apr 29, 2022

ShaMan123 commented Apr 30, 2022 • edited Loading

ShaMan123 commented Apr 4, 2022 •

edited

Loading

ShaMan123 commented Apr 10, 2022 •

edited

Loading

ShaMan123 commented Apr 11, 2022 •

edited

Loading

ShaMan123 commented Apr 26, 2022 •

edited

Loading

github-actions bot commented Apr 28, 2022 •

edited

Loading

ShaMan123 commented Apr 30, 2022 •

edited

Loading