tiagobento commented on code in PR #2546:
URL:
https://github.com/apache/incubator-kie-tools/pull/2546#discussion_r1774134819
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
Review Comment:
This is not a mutation error. Please adjust the logs.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds},
edgeTargetBounds: ${edgeTargetBounds}`
+ );
Review Comment:
Not in a mutation. Please adjust the logs.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
Review Comment:
```suggestion
}
const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
if (edgeSourceBounds === undefined || edgeTargetBounds ===
undefined) {
```
100% nit-picking, but I usually bring local variables used in guard clauses
like this closer to the `if` itself, and give a little separation from the code
that came before it.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds},
edgeTargetBounds: ${edgeTargetBounds}`
+ );
+ return;
+ }
+
+ const sourceNode = nodesById.get(edge.source);
+ const targetNode = nodesById.get(edge.target);
+
+ if (sourceNode === undefined || targetNode === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data sourceNode: ${sourceNode}, targetNode:
${targetNode}`
+ );
+ return;
+ }
+
+ const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix !==
undefined;
+
+ addEdge({
+ definitions: state.dmn.model.definitions,
+ drdIndex: state.computed(state).getDrdIndex(),
+ edge: {
+ type: edge.type as EdgeType,
+ targetHandle: getHandlePosition({ shapeBounds: edgeTargetBounds,
waypoint: snappedPotentialWaypoint })
+ .handlePosition as PositionalNodeHandleId,
+ sourceHandle: getHandlePosition({ shapeBounds: edgeSourceBounds,
waypoint: snappedPotentialWaypoint })
+ .handlePosition as PositionalNodeHandleId,
+ autoPositionedEdgeMarker: undefined,
+ },
+ sourceNode: {
+ type: sourceNode.type as NodeType,
+ data: sourceNode.data,
+ href: edge.source,
+ bounds: edgeSourceBounds,
+ shapeId: edge.data?.dmnShapeSource["@_id"],
+ },
+ targetNode: {
+ type: targetNode.type as NodeType,
+ href: edge.target,
+ data: targetNode.data,
+ bounds: edgeTargetBounds,
+ index: nodesById.get(edge.target)?.data.index ?? 0,
+ shapeId: edge.data?.dmnShapeTarget["@_id"],
+ },
+ keepWaypoints: false,
+ requirementEdgeTargetingExternalNodeId: isTargetExternalNode ?
edgeId : undefined,
+ });
+
+ console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge was added
into diagram.`);
Review Comment:
Not inside a mutation.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -96,18 +173,27 @@ export function usePotentialWaypointControls(
}
dmnEditorStoreApi.setState((state) => {
+ const dmnEdgeIndex =
state.computed(state).indexedDrd().dmnEdgesByDmnElementRef.get(edgeId)?.index;
+ if (dmnEdgeIndex === undefined) {
+ console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge has missing
index.`);
+ return;
+ }
Review Comment:
I think we should `throw` here, as at this point, this operator should never
fail. If `edgeIndex` doesn't exist, you create a new edge, this querying for
the edgeId again should always give you a new edge. If that doesn't happen,
something exceptional happened, so we throw. Also not in a mutation.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds},
edgeTargetBounds: ${edgeTargetBounds}`
+ );
+ return;
+ }
+
+ const sourceNode = nodesById.get(edge.source);
+ const targetNode = nodesById.get(edge.target);
+
+ if (sourceNode === undefined || targetNode === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data sourceNode: ${sourceNode}, targetNode:
${targetNode}`
+ );
+ return;
+ }
+
+ const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix !==
undefined;
Review Comment:
```suggestion
const isTargetingExternalNode =
targetNode.data.dmnObjectQName.prefix !== undefined;
```
or
```suggestion
const isTargetExternal = targetNode.data.dmnObjectQName.prefix !==
undefined;
```
or
```suggestion
const targetsExternalNode = targetNode.data.dmnObjectQName.prefix
!== undefined;
```
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -74,10 +80,81 @@ 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 waypoint 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.data?.dmnShapeSource === undefined ||
edge.data?.dmnShapeTarget == undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edge: ${edge}, edge.data: ${edge?.data}`
+ );
+ return;
+ }
+ const edgeSourceBounds = edge.data?.dmnShapeSource["dc:Bounds"];
+ const edgeTargetBounds = edge.data?.dmnShapeTarget["dc:Bounds"];
+
+ if (edgeSourceBounds === undefined || edgeTargetBounds === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data edgeSourceBounds: ${edgeSourceBounds},
edgeTargetBounds: ${edgeTargetBounds}`
+ );
+ return;
+ }
+
+ const sourceNode = nodesById.get(edge.source);
+ const targetNode = nodesById.get(edge.target);
+
+ if (sourceNode === undefined || targetNode === undefined) {
+ console.debug(
+ `DMN MUTATION: We can not add DMNEdge for '${edgeId}' edge into
diagram. There are missing data sourceNode: ${sourceNode}, targetNode:
${targetNode}`
+ );
Review Comment:
Not in a mutation. Please adjust the logs.
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -402,8 +410,13 @@ function ackRequirementEdges(
if (dmnObject.__$$element === "decision") {
(dmnObject.informationRequirement ?? []).forEach((ir, index) => {
const irHref = parseXmlHref((ir.requiredDecision ??
ir.requiredInput)!["@_href"]);
+ // Search for definitions[`@_xmlns:included${includedIndex}`] that
holds proper namespace
+ // and store the proper prefix: `included${includedIndex}` value
+ const namespaceIncludedPrefix = Object.entries(definitions)
+ .find(([key, val]) => val === namespace)?.[0]
+ ?.replace("@_xmlns:", "");
ackEdge({
- id: ir["@_id"]!,
+ id: (namespaceIncludedPrefix ? `${namespaceIncludedPrefix}:` : "") +
ir["@_id"]!,
Review Comment:
Why not
```suggestion
id: (drgElementsNamespace === thisDmnsNamespace
? ir["@_id"]
: buildXmlHref({ namespace: drgElementsNamespace, id: ir["@_id"]
}),
```
?
----
External nodes are currently using an HREF (`namespace#id`) as ID, I think
we should keep the same strategy for edges, instead of using a QName
(`prefix:id`).
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -96,18 +173,27 @@ export function usePotentialWaypointControls(
}
dmnEditorStoreApi.setState((state) => {
+ const dmnEdgeIndex =
state.computed(state).indexedDrd().dmnEdgesByDmnElementRef.get(edgeId)?.index;
+ if (dmnEdgeIndex === undefined) {
+ console.debug(`DMN MUTATION: DMNEdge for '${edgeId}' edge has missing
index.`);
+ return;
+ }
addEdgeWaypoint({
definitions: state.dmn.model.definitions,
drdIndex,
beforeIndex: i - 1,
- edgeIndex,
+ edgeIndex: dmnEdgeIndex,
waypoint: snappedPotentialWaypoint,
});
Review Comment:
Probably worth changing the name of this parameter to `dmnEdgeIndex`, since
it represents a DMNEdge object.
##########
packages/dmn-editor/src/diagram/edges/usePotentialWaypointControls.ts:
##########
@@ -96,18 +173,27 @@ export function usePotentialWaypointControls(
}
dmnEditorStoreApi.setState((state) => {
+ const dmnEdgeIndex =
state.computed(state).indexedDrd().dmnEdgesByDmnElementRef.get(edgeId)?.index;
Review Comment:
I think it's worth to add a comment here explaining why you're getting
`dmnEdgeIndex` when you already have `edgeIndex` in scope.
##########
packages/dmn-editor/src/diagram/connections/isValidConnection.ts:
##########
@@ -39,14 +39,19 @@ 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
+ const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix !=
undefined;
+ if (!extraArg?.allowExternalTarget && isTargetExternalNode) {
return false;
}
Review Comment:
This way, it is clear that `false` is the default for when `extraArg` is
`undefined`.
```typescript
export function _checkIsValidConnection(
sourceNode: { type?: string; data: DmnDiagramNodeData } | undefined,
targetNode: { type?: string; data: DmnDiagramNodeData } | undefined,
edgeType: string | null | undefined,
extraArg?: { allowExternalTarget: boolean; }
) {
if (!sourceNode?.type || !targetNode?.type || !edgeType) {
return false;
}
// External nodes cannot be targeted by default. However there are
exceptions,
// for example adding a waypoint on an edge between two external nodes.
const isTargetExternalNode = targetNode.data.dmnObjectQName.prefix !=
undefined;
const allowExternalTarget = extraArg?.allowExternalTarget ?? false;
if (isTargetExternalNode && !allowExternalTarget) {
return false;
}
}
```
--
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]
