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: Factor out workspace drag methods into utils. #8566

Merged
merged 1 commit into from
Sep 13, 2024

Conversation

johnnesky
Copy link
Member

The basics

The details

Resolves

Fixes #7926

Proposed Changes

This PR transfers the logic from WorkspaceSvg methods startDrag and moveDrag into a new utils file. It further factors out some shared coordinate math into a private function. It removes the private property dragDeltaXY from WorkspaceSvg (is this a breaking change?) and replaces it with an internal map from workspace to delta.

Reason for Changes

This feature is used by block comments and workspace comments, but is not generally used by workspace objects and thus is not really appropriate for WorkspaceSvg.

Test Coverage

Manually dragging and resizing comments seems to work as expected. I don't believe any existing tests directly call these methods on a workspace, maybe some do indirectly, but FWIW all existing tests pass.

Documentation

N/A

Additional Information

Is removing a private property considered a breaking change?

@johnnesky johnnesky requested a review from a team as a code owner September 7, 2024 00:43
@github-actions github-actions bot added the PR: fix Fixes a bug label Sep 7, 2024
Comment on lines +23 to +30
const point = browserEvents.mouseToSvg(
e,
workspace.getParentSvg(),
workspace.getInverseScreenCTM(),
);
// Fix scale of mouse event.
point.x /= workspace.scale;
point.y /= workspace.scale;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since I don't really know what a ScreenCTM is the following question is probably nonsense, but: can the post-mouseToSvg fixup be avoided by passing a better value as the third parameter of mouseToSvg?

And if so would such a change be beneficial?

(Side note: looking at the definitions of mouseToSvg and getInverseScreenCTM, it appears that it mightalternatively be possible to completely omit the third parameter to mouseToSvg with no change to the returned valuest—but perhaps that would be less efficient, since getInverseScreenCTM seems to do caching.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think, in some sense, the "correct" thing to do would be to pass workspace.getCanvas() as the second argument, to derive the transform matrix from the block canvas which should incorporate the zoom. However, the SVG APIs that mouseToSvg uses require an SVGSVGElement (<svg>), whereas the block canvas is a <g> element.

@johnnesky johnnesky merged commit 561b418 into google:develop Sep 13, 2024
11 checks passed
@johnnesky johnnesky deleted the nesky_drag_utils branch September 13, 2024 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move logic from startDrag and moveDrag into a utils file
2 participants