-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(Canvas): setActiveObject
should update canvas#_activeSelection
#9336
Conversation
eh right. Said so the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very good, exacxtly what we were looking for to finilize the change.
@asturur removing readonly is a must because the ref changes now.
Please add a test in src/shapes/ActiveSelection.spec.ts
If we want to ensure that activeSelection is only readonly, we need to split the object when setting it, and we also need to consider event triggering, something like this?
The incomplete code above will trigger add and delete callback events. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent job
A few small changes to the test and this is good to merge
setActiveObject
should update canvas#_activeSelection
There is no need, I didn't consider this use case when I made it readonly |
Ahh I think we need to call |
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
Co-authored-by: Shachar <34343793+ShaMan123@users.noreply.github.com>
If the user actively chooses to cancel.
Do we need to help the user cancel it when setting it up? Or we need to provide a new method ?
|
good questions. |
Also consider what happens without discarding because fabric does that under the hood
|
em... Do I need to modify and submit it like this?
|
My bad, this is incorrect. If an object was present in the active selection it should be removed by fabric once added to a new one, I think I need to fix a small thing about that. We should add a test for this specific case to see all is as expected including fired events. |
Yes i didn't mean it needs to be readonly, i meant to bring up the point that there is a readonly and to think why. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change request i made is optional, it doesn't change anything is just some bytes saved
There is a bug I will look into because I think I caused it. |
This reverts commit 2ccfbde. fix
Fixed. |
Update SelectableCanvas.ts
8660184
to
5fc9171
Compare
@asturur I have found another related bug handled in #9349 |
done. |
Since my english is never understood, from now on i will close PRs that try to fix 2 bugs in one pr. |
@@ -86,7 +86,9 @@ export class ActiveSelection extends Group { | |||
// save ref to group for later in order to return to it | |||
const parent = object.group; | |||
parent._exitGroup(object); | |||
object.__owningGroup = parent; | |||
// make sure we are setting the correct owning group | |||
// in case `object` is transferred between active selections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there shouldn't be transferring between active selections.
How that happens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you can do:
new ActiveSelection(canvas.getActiveObjects())
And that will hit this code
src/canvas/SelectableCanvas.ts
Outdated
@@ -1105,6 +1105,13 @@ export class SelectableCanvas<EventSpec extends CanvasEvents = CanvasEvents> | |||
} | |||
this._activeObject = object; | |||
|
|||
if (object instanceof ActiveSelection && this._activeSelection !== object) { | |||
this._activeSelection.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a later addition that is problematic.
Disposing an activeSelection means disposing contained objects.
We can't do that. This is just setActiveObject() and we shouldn't dispose inside it because all the resources the developer is using may be still completely valid and needed for his/her app.
The old active selection being dereferenced will be garbage collected at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right - this should be a removeAll call
Your English is well understood most of the time |
it is bad indeed to dispose anyways it is safe to assume that the prev active selection has no children because it has been discarded and onDelect removed all the children
@asturur you want anything else? Or I merge |
@zhe-he Thanks, XieXie |
totally forgout about this, i thought it was done |
This sounds fine to me.
Start their own rabbit of holes of new possibilities, that i don't want to follow. |
I think we should consider throwing in cases we find unfit |
before:
after: