fix(connectionHandler): apply perimeter style to new edges#1026
fix(connectionHandler): apply perimeter style to new edges#1026LOUISNOYEZ wants to merge 2 commits intomaxGraph:mainfrom
Conversation
WalkthroughChanged Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/view/mixin/ConnectionsMixin.ts (1)
202-207: Outdated comment no longer reflects the code behavior.The comment "Only writes 0 since 1 is default" is now misleading:
- The code writes
false/true(booleans), not0/1(strings)- The code now writes both
falseandtrue, not just the non-default valueConsider updating or removing the comment to reflect the current behavior.
📝 Suggested comment update or simplification
Option 1: Update the comment
- // Only writes 0 since 1 is default + // Writes perimeter constraint as boolean style value if (!constraint.perimeter) { this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]); } else { this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]); }Option 2: Simplify the logic (removes need for comment)
- // Only writes 0 since 1 is default - if (!constraint.perimeter) { - this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]); - } else { - this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]); - } + this.setCellStyles( + source ? 'exitPerimeter' : 'entryPerimeter', + !!constraint.perimeter, + [edge] + );
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 67284926-e1d8-4764-8741-f19bc7d0b515
📒 Files selected for processing (1)
packages/core/src/view/mixin/ConnectionsMixin.ts
tbouffard
left a comment
There was a problem hiding this comment.
Thanks for your contribution.
There are some minor errors to fix.
I need to manually test storybook with this PR, but it looks fine.
Regarding the possible improvements in the signature of setCellStyles, I have proposed #1028 that is able to catch errors like the one fixed here
| this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', false, [edge]); | ||
| } else { | ||
| this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', null, [edge]); | ||
| this.setCellStyles(source ? 'exitPerimeter' : 'entryPerimeter', true, [edge]); |
There was a problem hiding this comment.
suggestion: here we can keep null, the default value will be then used (true). This is what the comment line 202 tries to explain (I guess).
There was a problem hiding this comment.
From my tests in the Fixed Points story, the new edge fails to connect to the perimeter of the shape when the value is set to null. I've tried to search through the files but I couldn't find code indicating that the style value would default to true when the value null is used. The properties entryPerimeter and exitPerimeter do not appear to be set to a default value in the default stylesheet.
Incidentally, entryPerimeter/exitPerimeter is also set to null on line 195. I'm not sure if the intent is to set it to default, but I cannot find any default. I assume maxgraph treats it as "false" in this case.
There was a problem hiding this comment.
OK, fine, let's set it to true, this is also more explicit.
The default value for exitPerimeter is documented in
maxGraph/packages/core/src/types.ts
Lines 275 to 280 in 24357a7
true (the same applies to entryPerimeter).
In mxGraph, the default was actually true.
perimeter = mxUtils.getValue(edge.style, (source) ? mxConstants.STYLE_EXIT_PERIMETER :
mxConstants.STYLE_ENTRY_PERIMETER, true);But what I see in the maxGraph code is that the default is false 😭
maxGraph/packages/core/src/view/mixin/ConnectionsMixin.ts
Lines 170 to 175 in 24357a7
The default was
true in c89ce4c, removed in 648e324, then set to false in 413796a
Do you think we should fix this in this PR or do we create another PR? This may impact several features/stories, so maybe it is safer to treat this in a dedicated PR.
|



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).Overview
Restore mxgraph new edges endpoints positionning when using the connection handler to connect vertex cell shapes with non rectangular perimeters.
Bug description
When using the connection handler to connect cell shapes which have a non rectangular perimeter, the new edge preview displays correctly, but when the new edge is created, its endpoints are connected to connection points on the rectangular bounding box of the cell shape instead of to the connection points projected on the perimeter of cell shapes.
This pull request is intended to fix issue #840 although this bug occurs on any graph which uses the connection handler and vertex with non rectangular perimeters.
Demonstration using the Fixed Points story
Current maxgraph behavior : The new edge connects to the bounding box of the cell shape instead of connecting to the actual perimeter of the cell shape.
connection_perimeter_wrong.mp4
Intended mxgraph behavior : the new edge connects directly to the perimeter of the cell shape.
connection_perimeter_right.mp4
Analysis
As per the migration guide from mxgraph to maxgraph, some styling properties that used to be set using strings
"0"or"1"'have been replaced withbooleanvalues.maxGraph/packages/website/docs/usage/migrate-from-mxgraph.md
Line 533 in 24357a7
This includes the properties
entryPerimeterandexitPerimeterof edge's style, which dictate whether edges should take the perimeter into account when connecting.maxGraph/packages/website/docs/usage/migrate-from-mxgraph.md
Lines 545 to 546 in 24357a7
This type change has been taken into account accross the codebase, but not everywhere. Specifically it has not been taken into account for the connection handler in its connection handler mixin.
When creating new edges via connections, this style property is still set using a
stringin thesetConnectionConstraintmethod, and they are thus ignored :maxGraph/packages/core/src/view/mixin/ConnectionsMixin.ts
Lines 203 to 207 in 24357a7
This issue is fixed by setting the appropriate values :
Notes
setCellStylesmethod does not take advantage of the typescript type checker to catch these kinds of mistakes statically. Its signature iskeyand the type of the second argumentvalue. Its type is a union of all types ofCellStateStyleproperties, which includesstring, hence why typescript doesn't catch the error. I believe a possible solution might be to pass an object to setCellStyles to update styles instead of two separate arguments in order to catch errors, maybeKeywords
closes #840
Summary by CodeRabbit