danielzhe commented on code in PR #2546:
URL:
https://github.com/apache/incubator-kie-tools/pull/2546#discussion_r1763922068
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,86 @@ export function usePotentialWaypointControls(
}, [snapGrid, potentialWaypoint]);
const onDoubleClick = useCallback(() => {
- if (!potentialWaypoint || !snappedPotentialWaypoint || edgeIndex ===
undefined) {
+ if (!potentialWaypoint || !snappedPotentialWaypoint) {
return;
}
+ if (edgeIndex === undefined) {
+ /**
+ * This means we are adding a first wayipoint to one of following edges:
Review Comment:
Typo in "wayipoint" = "waypoint"
##########
packages/dmn-editor/src/diagram/connections/isValidConnection.ts:
##########
@@ -39,14 +39,18 @@ export function checkIsValidConnection(
export function _checkIsValidConnection(
sourceNode: { type?: string; data: DmnDiagramNodeData } | undefined,
targetNode: { type?: string; data: DmnDiagramNodeData } | undefined,
- edgeType: string | null | undefined
+ edgeType: string | null | undefined,
+ extraArg?: {
+ allowExternalTarget: boolean;
+ }
) {
if (!sourceNode?.type || !targetNode?.type || !edgeType) {
return false;
}
- // External nodes cannot be targeted
- if (targetNode.data.dmnObjectQName.prefix) {
+ // External nodes cannot be targeted by default
+ // However there are exceptions, for example adding a waypoint on the edge
+ if (!extraArg?.allowExternalTarget && targetNode.data.dmnObjectQName.prefix)
{
Review Comment:
In the future, will we have local nodes with a prefix, @tiagobento ? We were
talking about that in pvt chat a few weeks ago.
Anyway, I don't feel confident concluding that nodes with a `prefix` are
always remote. Maybe if someone else in the future looks at this code doesn't
understand why we're checking if there is a `prefix` or not.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,86 @@ export function usePotentialWaypointControls(
}, [snapGrid, potentialWaypoint]);
const onDoubleClick = useCallback(() => {
- if (!potentialWaypoint || !snappedPotentialWaypoint || edgeIndex ===
undefined) {
+ if (!potentialWaypoint || !snappedPotentialWaypoint) {
return;
}
+ if (edgeIndex === undefined) {
+ /**
+ * This means we are adding a first wayipoint to one of following edges:
+ * - an edge in a non default DRD
+ * - an edge targeting an external node
+ */
+ dmnEditorStoreApi.setState((state) => {
+ const nodesById =
state.computed(state).getDiagramData(externalModelsByNamespace).nodesById;
+ const edge =
state.computed(state).getDiagramData(externalModelsByNamespace).edgesById.get(edgeId);
+ if (
+ edge === undefined ||
+ edge.type === undefined ||
+ edge.data === undefined ||
+ edge.data?.dmnShapeSource === undefined ||
+ edge.data?.dmnShapeTarget === undefined
+ ) {
+ console.debug(`DMN MUTATION: We can not add DMNEdge for '${edgeId}'
edge into diagram.`);
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
Review Comment:
I think you can discard the null checks in lines 100-101 and merge with
those, adding `?.` before the `["dc:Bounds"]`
```
const edgeSourceBounds = edge.data?.dmnShapeSource?.["dc:Bounds"];
const edgeTargetBounds = edge.data?.dmnShapeTarget?.["dc:Bounds"];
```
But doing this, in a few lines bellow, this will fail because we didn't null
check only for `dmnShapeSource`.
`edge.data?.dmnShapeSource["@_id"]`
So, maybe we can re-work those null checks, making more clear error messages
about what failed and joining what makes sense to be together (for example, the
null checks about `dmnShapeTarget`).
What do you think? Does it make sense?
##########
packages/dmn-editor/src/mutations/addEdgeWaypoint.ts:
##########
@@ -38,7 +38,7 @@ export function addEdgeWaypoint({
const diagramElement = diagramElements[edgeIndex];
if (diagramElement.__$$element !== "dmndi:DMNEdge") {
- throw new Error("DMN MUTATION: Can't remove a waypoint from an element
that is not a DMNEdge.");
+ throw new Error("DMN MUTATION: Can't add a waypoint for an element that is
not a DMNEdge.");
Review Comment:
👍
##########
packages/dmn-editor/tests-e2e/drds/modelDrd.spec.ts:
##########
@@ -136,13 +136,13 @@ test.describe("Model DRD", () => {
await drds.toggle();
await drds.navigateTo({ name: "Second DRD" });
await drds.toggle();
- await drgNodes.open();
Review Comment:
🎉
##########
packages/dmn-editor/src/mutations/addStandaloneNode.ts:
##########
@@ -18,7 +18,7 @@
*/
import { switchExpression } from "@kie-tools-core/switch-expression-ts";
-import { DmnBuiltInDataType, generateUuid } from
"@kie-tools/boxed-expression-component/dist/api";
Review Comment:
I've been doing that kind of cleanup every time I see it. Nice!
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,86 @@ export function usePotentialWaypointControls(
}, [snapGrid, potentialWaypoint]);
const onDoubleClick = useCallback(() => {
- if (!potentialWaypoint || !snappedPotentialWaypoint || edgeIndex ===
undefined) {
+ if (!potentialWaypoint || !snappedPotentialWaypoint) {
return;
}
+ if (edgeIndex === undefined) {
+ /**
+ * This means we are adding a first wayipoint to one of following edges:
+ * - an edge in a non default DRD
+ * - an edge targeting an external node
+ */
+ dmnEditorStoreApi.setState((state) => {
+ const nodesById =
state.computed(state).getDiagramData(externalModelsByNamespace).nodesById;
+ const edge =
state.computed(state).getDiagramData(externalModelsByNamespace).edgesById.get(edgeId);
+ if (
+ edge === undefined ||
+ edge.type === undefined ||
+ edge.data === undefined ||
+ edge.data?.dmnShapeSource === undefined ||
+ edge.data?.dmnShapeTarget === undefined
+ ) {
+ console.debug(`DMN MUTATION: We can not add DMNEdge for '${edgeId}'
edge into diagram.`);
Review Comment:
Maybe we can add more details about why we can not add the DMNEdge here. We
don't know if we can't because the edge is not defined, type, data,
shapeSource, or shapeTarget and we have the exactly same message bellow in
other 2 places. So, just looking at the console, we can not determine what
caused the error or if it was in the first, second or third place.
##########
packages/dmn-editor/src/mutations/addEdge.ts:
##########
@@ -74,14 +74,20 @@ export function addEdge({
autoPositionedEdgeMarker: AutoPositionedEdgeMarker | undefined;
};
keepWaypoints: boolean;
+ extraArg?: {
Review Comment:
I'm being annoying here: I don't like the `extraArgs`. Why they are "extra"?
If are optional args, I think we can use
`requirementEdgeTargetingExternalNodeId?: string` for example.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
