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.

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

ungroupOnCanvas does not ungroup and does not restore object state correctly. #6315

Closed
virror opened this issue May 5, 2020 · 10 comments
Closed

Comments

@virror
Copy link
Contributor

virror commented May 5, 2020

Version

3.6.3

Test Case

https://jsfiddle.net/Lmxb9qdu/3/

Information about environment

Firefox, Chrome

Steps to reproduce

Run fiddle, select both boxes and press "Group" button, then press "Ungroup" button and click on canvas to get rid of selection.

Expected Behavior

I expect what the docs says: "Destroys a group (restoring state of its objects) ".
That the group gets destroyed and the objects are restored.

Actual Behavior

Group is not destroyed at all, and the objects changes their position.

One way of working around this is to make sure the selection is removed first, and then destroy the group manually. This seems to get the expected behavior (imo):

canvas.discardActiveObject()
object.ungroupOnCanvas()
canvas.remove object

@awehring
Copy link

awehring commented May 5, 2020

I have never used a group (besides activeSelection). But there is a tutorial on how to work with them. Your example doesn't follow it.

http://fabricjs.com/fabric-intro-part-3#groups

BTW to create and expand an activeSelection works somewhat different. See #6130
Propably it is the same with groups.

@virror
Copy link
Contributor Author

virror commented May 5, 2020

Well, the functionality does not follow what the docs says either, so ether the bug is in the docs, or in the functionality : )

Edit: And the ungroupOnCanvas is not mentioned in the tutorial at all, so there is no way to know for certain how to use that function.

Edit2: Actually the tutorial does not at all mention how to get rid of groups.

@asturur
Copy link
Member

asturur commented May 5, 2020

my memory is short on what ungroupOnCanvas is suppoesed to do ( git blame to the rescue ).
The only real way to move from group to separated objects quickly is to use

toActiveSelection.

ActiveSelection.toGroup and Group.toActiveSelection are supposed to work on with the other.

http://fabricjs.com/manage-selection

@asturur
Copy link
Member

asturur commented May 5, 2020

unGroupOnCanvas seems something that was created ( 3 years ago ) never used elsewhere, maybe thought of being fo public utility but i do not think it is.

I think i should delete it in the beta of version 4 and just put it as a breaking change.

And by the way unGroupOnCanvas just call _restoreObjectsState that seems like just calling itself again, like if it was broken/misspelled.

Does it even work?

@awehring
Copy link

awehring commented May 5, 2020

With some distance:
The jsfiddle code is group = canvas.getActiveObject();
ActiveSelection is an extension to the Group Class. So simply assigning one to the other might give unexpected results.

@virror
Copy link
Contributor Author

virror commented May 5, 2020

Yes, i have seen that example also and "toActiveSelection" was the first thing i tried.
What im doing is that i dont want to have "stacked" groups, so when i group something with a group, i have to break down the group first and regroup it so all objects are in the base group.

For actual user ungrouping i use "toActiveSelection". But using "toActiveSelection" made stuff move around on the canvas unexpectedly when it was part of a selection and i did not get it to work correctly, thats why i looked for an alternative way on ungrouping.

@virror
Copy link
Contributor Author

virror commented May 5, 2020

Yes, might be best to just remove it. I might have to try again to get it to work with the "toActiveSelection" instead, even though it works fine now with "unGroupOnCanvas" when manually removing the group afterwards.

@asturur
Copy link
Member

asturur commented May 5, 2020

So you are looking that is like flattenGroup. That could be a feature eventually if it requires too much logic in an app.
I can see how someone does not want to pass trought many manual deselection process to do that.

@virror
Copy link
Contributor Author

virror commented May 5, 2020

Yes, that exactly what im trying to do. Saving the state is just to bothersome when its stacked : p
Im trying to make a fiddle and see if i can make it work with "toActiveSelection" instead.

@virror
Copy link
Contributor Author

virror commented May 5, 2020

Ok, finally managed to get it to work:
https://jsfiddle.net/1p732srw/

I guess this issue can be closed now then if "unGroupOnCanvas" is going to get removed anyways : )

@virror virror closed this as completed May 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants