fix: correct rotation handle positioning during vertex rotation #581
Conversation
…x rotation The rotation handle was incorrectly positioned while rotating, it was always on the right side after any rotation. This fix: - Aligns the rotation handle properly based on current rotation value Fixes maxGraph#482
|
|
||
| if (this.rotationShape) { | ||
| const alpha = toRadians(this.currentAlpha); | ||
| const alpha = toRadians(this.state.style.rotation ?? 0); |
There was a problem hiding this comment.
suggestion: Good catch! The problem was introduced during the migration from mxGraph.
Could you match the original mxGraph behavior? AFAIK, this.currentAlpha was used if not nullish. See https://github.com/jgraph/mxgraph/blob/v4.2.2/javascript/src/js/handler/mxVertexHandler.js#L2019
ℹ️ there is no need have a single commit with the initial implementation and the suggested change that would be forced pushed in the PR.
I will squash all commits when merging the PR
There was a problem hiding this comment.
I can try to pull it off tomorrow but if I recall correctly from code in the mxGraph currentAlpha was null by deafult, in our case its 100 for some reason. I tried setting initial alpha to 0 but the rotation was still not working. I am sure the calculation in rotation function is badly calculated when setting new alpha.
There was a problem hiding this comment.
OK I will also check on my side (I am not sure that I will be able to do it during the week-end)
There was a problem hiding this comment.
I looked all around this file and when declaring alpha the variable was derived from this.state.style.rotation. I logged the value of currentAlpha and this.style.rotation and they do have same values at the at but after I release there were 2 redrawHandle function calls where both style.rotation and currentAlpha was 0 (it should be 90 since i rotated for 90 deg). Confusing thing is the values are always the same but for some reason results differ. My assumption is that alpha is not set until we finish rotating while style.rotation properly updates while we rotate. This could cause this behaviour but im not sure.
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/core/src/view/handler/VertexHandler.ts (2)
1890-1893: The rotation handle positioning fix looks good!The change to use
state.style.rotationinstead ofcurrentAlphacorrectly fixes the rotation handle positioning issue. This aligns with the original mxGraph behavior and ensures the handle position accurately reflects the vertex's current rotation.Consider adding a comment explaining why
style.rotationis preferred overcurrentAlphato help future maintainers understand this important implementation detail.- const alpha = toRadians(this.state.style.rotation ?? 0); + // Use style.rotation instead of currentAlpha to ensure handle position + // matches the vertex's actual rotation state + const alpha = toRadians(this.state.style.rotation ?? 0);
Line range hint
1898-1902: Improve rotation handle visibility managementThe visibility logic for the rotation handle mixes multiple concerns. Consider extracting this into a dedicated method for better maintainability.
- this.rotationShape.node.style.visibility = - this.state.view.graph.isEditing() || !this.handlesVisible ? 'hidden' : ''; + this.rotationShape.node.style.visibility = this.isRotationHandleVisible() ? '' : 'hidden'; + private isRotationHandleVisible(): boolean { + return !this.state.view.graph.isEditing() && this.handlesVisible; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/src/view/handler/VertexHandler.ts(1 hunks)
🔇 Additional comments (1)
packages/core/src/view/handler/VertexHandler.ts (1)
1890-1893: Verify rotation preservation during operations
While the rotation handle positioning is fixed, we should verify that the rotation value is properly preserved during all vertex operations (e.g., resizing, moving, etc.).
Also applies to: 1052-1054
|
@HalilFocic here are my 2 cents about the My guess is that the value To go ahead with the fix, I suggest we keep the implementation you're currently proposing, which doesn't use the
Does this solution suit you? I can create the issue and let you make the changes in the PR code. |
Yeah, seems right. I think the handles are also not using the alpha. I investigated a bit and for some reason the currentAlpha has same value as rotation when we are holding mousedown but after release there is redraw where rotation is correct but alpha is incorrect. |
|
@HalilFocic I have just created #590 |
|
@tbouffard I will close this PR since #592 is updated example that resembles mxGraph functionality better and fixes both this issue and #590 |
The rotation handle was incorrectly positioned while rotating, it was always on the right side after any rotation. This fix:
PR Checklist
maxGraph, and you are assigned to the issue.packages/core/_tests_or a new or altered Storybook story inpackages/html/stories(an existing story may also demonstrate the change).I have added or edited necessary documentation,or no docs changes are needed.Overview
Code for sizers was properly getting the current rotation angle and positioning sizers correctly while rotation handle was using different logic which always though rotation was some specific angle.
Both stencils and handles stories now properly work while rotating:
handles.mov
stencils.mov
Notes
Summary by CodeRabbit
New Features
Bug Fixes