-
-
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(ActiveSelection): reset positioning when cleared #9088
Conversation
Build Stats
|
removeAll
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.
ready
flipY: false, | ||
}); | ||
} | ||
} |
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.
is there a reason why resetting and not doing new ActiveSelection([])
?
In this way the reset becomes an ActiveSelection issue, while creating it is a canvas issue.
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.
To keep the ref so we can listen to events etc.
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.
Also if somrone subclasses they can set the ref and it will remain as is
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.
I think that this is an evidence that keeping the same reference for ActiveSelection is a bad breaking change and design. It causes bugs and workarounds for things that were more natural behaviour, e.g. #9066.
To keep the ref so we can listen to events etc.
What's an use case for listening events on the ActiveSelection? Is there an alternative that doesn't require keeping the same reference?
Also if somrone subclasses they can set the ref and it will remain as is
Not clear to me, can you make an example of use case + code?
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.
- Bug Scope: I have ben thinking of this specific fix. Could it be that it belongs to Group? I need to test this. Might be this is a bug in
enterGroup
when group is rotated. - Events: There are many requests from devs needing to listen to active selection events. Many speak of customizing the stack order in accordance to some business logic (which can now be accomplished using
multiSelectAdd
). Also toggling selectablility of objects, responding visually to stuff etc. - Subclassing: In our project I will subclass active selection to override
multiSelectAdd
to block objects that should not be selected from being added to the selection (formerly_createActiveSelection
,_updateActiveSelection
)
export class ActiveSelection extends fabric.ActiveSelection {
multiSelectAdd(...targets: fabric.Object[]): void {
super.multiSelectAdd(...targets.filter((object) => !isReadOnly(object)));
}
}
- The only place that was creating a new
ActiveSelection
was_createActiveSelection
. - I want group to be robost, it seems to me this bug is a group bug.
- I also think canvas should accept the active selection ref in options or create a default one if non was passed. We can make the constructor of
Activeselection
protected - that will help devs see the change. - Devs can also add logic to
setActiveObject
if they must continue creating active selections:
setActiveObject(object) {
if (object instaceof ActiveSelection) {
this._activeSelection = object;
object.set('canvas', this);
}
super.setActiveObject(object);
}
I do think it makes sense holding the ref but I am not fixed on it.
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.
Legit point of view
I am agnostic
It is a breaking change, and the worst kind because it is silent
However we want to publish with as less changes as possible at this point
@asturur I know you didn't like the const ref either. What do you think?
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.
i forgot to update myself on this. i ll do and let's clear this out soon
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.
First question, is there any reason why here we do this manual change while in _discardActiveObject we call the util resetObjectTransform
?
We could extend resetObjectTransform to handle top and left since seems clear to me that in both cases we use resetObjectTransform we don't care for survival of top/left.
My general idea is that is late to remove this reference activeSelection even if i don't like it at all, it would be nice to reuse resetObjectTransform to save code duplication.
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.
As long as fabric does not create instances of active selection I am ok with dropping the ref.
A bit late, but fine. Lets take advantage of beta.
Second, the resetting should occur in group in terms of expectation, not only active selection.
I do not remeber why I didn't use resetObjectTransform but I do remember considering it.
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 only reason why i don't want to remove the ref is because i don't like go back and forth on choices.
We made this, let's move forward with this. If some issue will arise we will have to reconsider and we will do a breaking change.
i will try to reuse resetObject trasform in a separate PR, i can't see any reason for it to be a problem.
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.
TESTED AND READY
tests pass locally not sure what is going on |
Motivation
closes #9087
Description
Since active selection is a ref it needs cleaning up
Changes
Gist
In Action