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

fix(): stop transforming on discardActiveObject #7954

Merged
merged 5 commits into from
Jun 30, 2022

Conversation

ShaMan123
Copy link
Contributor

@ShaMan123 ShaMan123 commented May 20, 2022

Motivation
closes #7953

@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2022

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.03 |    75.94 |   85.69 |   82.75 |                                               
 fabric.js |   83.03 |    75.94 |   85.69 |   82.75 | ...,30854,30928,30939-31004,31127,31226,31462 
-----------|---------|----------|---------|---------|-----------------------------------------------

@ShaMan123 ShaMan123 changed the title fix(): finalize transform on discardObject fix(): stop transforming on discardObject May 20, 2022
@ShaMan123 ShaMan123 changed the title fix(): stop transforming on discardObject fix(): stop transforming on discardActiveObject May 20, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2022

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.02 |    75.91 |   85.69 |   82.74 |                                               
 fabric.js |   83.02 |    75.91 |   85.69 |   82.74 | ...,30855,30929,30940-31005,31128,31227,31463 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented May 20, 2022

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.03 |    75.94 |   85.69 |   82.75 |                                               
 fabric.js |   83.03 |    75.94 |   85.69 |   82.75 | ...,30855,30929,30940-31005,31128,31227,31463 
-----------|---------|----------|---------|---------|-----------------------------------------------

@github-actions
Copy link
Contributor

github-actions bot commented May 21, 2022

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.03 |    75.94 |   85.69 |   82.75 |                                               
 fabric.js |   83.03 |    75.94 |   85.69 |   82.75 | ...,30855,30929,30940-31005,31128,31227,31463 
-----------|---------|----------|---------|---------|-----------------------------------------------

@@ -1243,6 +1243,11 @@
if (obj.onDeselect({ e: e, object: object })) {
return false;
}
if (this._currentTransform && this._currentTransform.target === obj) {
this._finalizeCurrentTransform(e);
this._currentTransform = null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we call this._resetTransformEventData();?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@asturur this is not outdated
Thoughts?

Copy link
Contributor Author

@ShaMan123 ShaMan123 Jun 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually Canvas._target , I think it should be reset but I'm not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have encountered the case that demands to nullify _target
When an object is being moved by the user and discarded meanwhile.
Currently the object will continue moving and will be inactive only on mouseup.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note for future self

setting canvas._target on mousedown:before hijacks the event and fires it on the hijacker

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Issue marked as stale by the stale bot label Jun 12, 2022
@ShaMan123 ShaMan123 removed the stale Issue marked as stale by the stale bot label Jun 13, 2022
@asturur
Copy link
Member

asturur commented Jun 26, 2022

I m ok with this PR but i would like there was a canvas method that says (cancel/stop/end)CurrentTransform and does what it needs to do to to cancel the current transform, and we use it on discardActiveObject

@github-actions
Copy link
Contributor

github-actions bot commented Jun 28, 2022

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.18 |    76.14 |   85.86 |   82.91 |                                               
 fabric.js |   83.18 |    76.14 |   85.86 |   82.91 | ...,30869,30943,30954-31019,31142,31241,31477 
-----------|---------|----------|---------|---------|-----------------------------------------------

@asturur
Copy link
Member

asturur commented Jun 28, 2022

i grouped the 2 calls in a method. i m ok merginging this.
The only doubt i have left and we can solve whenever is if that the discard of active objects belong to _discardActiveObject or discardActiveObject.

for example on object removed calls _discardActiveObject, and it could call _discardActiveObject and endCurrentTransform, instead of relying on the inner part, focused on discardind, now also fires an object modified event ( eventually ).

Is not a big deal, can probably be changed later.
Too much automatic behaviours usually brought to issues in the past

@ShaMan123
Copy link
Contributor Author

for example on object removed calls _discardActiveObject, and it could call _discardActiveObject and endCurrentTransform, instead of relying on the inner part, focused on discardind, now also fires an object modified event ( eventually ).

Sounds a good point to me.

I am not sure but I think I walked into a bug with this PR somewhere...
Well, I'll try reproduce and PR a fix is so.

@asturur
Copy link
Member

asturur commented Jun 28, 2022

is ok to merge this part as it is, is reasonably correct and help us moving forward

@asturur
Copy link
Member

asturur commented Jun 28, 2022

is small, is focused, has a test, has docs, can be merged imho.

@asturur asturur merged commit 12e0481 into master Jun 30, 2022
@asturur asturur deleted the discard-object-finalize-transform branch September 11, 2022 23:03
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

Successfully merging this pull request may close these issues.

discardActiveObject is not prevent moving of object
2 participants