tiagobento commented on code in PR #2154:
URL:
https://github.com/apache/incubator-kie-tools/pull/2154#discussion_r1492762687
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -160,6 +170,7 @@ export function computeDiagramData(
const _shape = indexes.dmnShapesByHref.get(id);
if (!_shape) {
drgElementsWithoutVisualRepresentationOnCurrentDrd.push(id);
+ drgNodeIdsBySourceNodeId.get(id)?.forEach((id) =>
nodeHasHiddenSource.add(id));
Review Comment:
I don't think this is correct. As you can see below, `ackEdge` is called for
external DMNs, so `drgNodeIdsBySourceNodeId` is not complete yet.
Actually, you don't really need `nodeHasHiddenSource`. Let's think about it:
You have an indexed adjacency list representation of the graph with
`drgNodeIdsBySourceNodeId`.
After all edges have been acked, we're sure that `drgNodeIdsBySourceNodeId`
is complete. You can iterate through the nodes, and query `nodesById` to see if
all direct dependents exist in the graph. If not, you assign
`data.hasHiddenSource.`
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -171,6 +182,7 @@ export function computeDiagramData(
dmnObject,
shape,
index,
+ hasHiddenSource: false,
parentRfNode: undefined,
Review Comment:
Can you leave a comment saying that these two get overridden further along?
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -327,6 +343,7 @@ export function computeDiagramData(
return {
drgEdges,
+ drgNodeIdsBySourceNodeId,
Review Comment:
Yeah I think `drgAdjacencyList` is the best one :P Since we're dealing with
a directed graph too, I would have the type be:
```ts
export type DrgAdjacencyList = Map<string, { dependents: Set<string>,
dependencies: Set<string> }>;
```
If you don't want to populate the `dependencies` part of it, just leave it
out of the type for now.
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -88,6 +95,8 @@ export function computeDiagramData(
const edges: RF.Edge<DmnDiagramEdgeData>[] = [];
const drgEdges: DrgEdge[] = [];
+ const drgNodeIdsBySourceNodeId: DrgNodeIdsBySourceNodeId = new Map();
Review Comment:
See my comment below regarding these names.
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -327,6 +343,7 @@ export function computeDiagramData(
return {
drgEdges,
+ drgNodeIdsBySourceNodeId,
Review Comment:
At some point we're probably going to need the transitive closure, but for
now we only need direct edges mapped.
##########
packages/dmn-editor/src/diagram/nodes/NodeSvgs.tsx:
##########
@@ -499,3 +520,44 @@ function NodeCollectionMarker(__props: NodeSvgProps & {
anchor: "top" | "bottom"
</>
);
}
+
+function NodeHiddenInformationMarker(__props: NodeSvgProps & { anchor:
"middle" | "left" }) {
Review Comment:
We're reusing `NodeSvgProps` here for a component that doesn't represent a
Node. I think we shouldn't do this. Or we should have a specific prop that is
called `nodeProps`, making it explicit that this component needs to know its
container node props.
##########
packages/dmn-editor/src/svg/DmnDiagramSvg.tsx:
##########
@@ -78,7 +81,18 @@ export function DmnDiagramSvg({
isAlternativeInputDataShape: boolean;
allDataTypesById: DataTypeIndex;
allTopLevelItemDefinitionUniqueNames: UniqueNameIndex;
+ drgNodeIdsBySourceNodeId: DrgNodeIdsBySourceNodeId;
+ drgElementsWithoutVisualRepresentationOnCurrentDrd: string[];
}) {
+ const nodesWithHiddenSource = useMemo(() => {
+ return drgElementsWithoutVisualRepresentationOnCurrentDrd.reduce((acc,
hiddenElementId) => {
+ drgNodeIdsBySourceNodeId.get(hiddenElementId)?.forEach((targetIds) => {
+ acc.add(targetIds);
+ });
+ return acc;
+ }, new Set<string>());
+ }, [drgElementsWithoutVisualRepresentationOnCurrentDrd,
drgNodeIdsBySourceNodeId]);
Review Comment:
Why do we need this when we already have `data.hasHiddenSource` assigned to
nodes?
##########
packages/dmn-editor/src/mutations/deleteNode.ts:
##########
@@ -107,10 +107,11 @@ export function deleteNode({
if (!dmnObjectQName.prefix) {
// Delete the dmnObject itself
if (nodeNature === NodeNature.ARTIFACT) {
- if (mode === NodeDeletionMode.FROM_DRG_AND_ALL_DRDS) {
- const nodeIndex = (definitions.artifact ?? []).findIndex((a) =>
a["@_id"] === dmnObjectId);
- dmnObject = definitions.artifact?.splice(nodeIndex, 1)?.[0];
- }
+ const nodeIndex = (definitions.artifact ?? []).findIndex((a) =>
a["@_id"] === dmnObjectId);
+ dmnObject =
+ mode === NodeDeletionMode.FROM_DRG_AND_ALL_DRDS
+ ? definitions.artifact?.splice(nodeIndex, 1)?.[0]
+ : definitions.artifact?.[nodeIndex];
Review Comment:
Not sure we should allow this. Hiding `artifacts` from a DRD won't ever
allow them to come back, as we don't have the option to add `artifacts` to a
DRD. This will make the DMN file contain phantom elements, which won't ever be
removed. I think we should throw in this case and disallow hiding a node from a
DRD when it is not a `drgElement`.
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -244,6 +256,10 @@ export function computeDiagramData(
} else if (localNodes[i].type === NODE_TYPES.decisionService) {
localNodes[i].zIndex = NODE_LAYERS.DECISION_SERVICE_NODE;
}
+
+ if (nodeHasHiddenSource.has(localNodes[i].id)) {
+ localNodes[i].data.hasHiddenSource = true;
+ }
Review Comment:
External nodes can have hidden source nodes too. Please see my comment above
to understand what I think is the best strategy here.
##########
packages/dmn-editor/src/diagram/nodes/NodeSvgs.tsx:
##########
@@ -499,3 +520,44 @@ function NodeCollectionMarker(__props: NodeSvgProps & {
anchor: "top" | "bottom"
</>
);
}
+
+function NodeHiddenInformationMarker(__props: NodeSvgProps & { anchor:
"middle" | "left" }) {
+ const { strokeWidth, x, y, width, height, fillColor, strokeColor, props } =
normalize(__props);
+
+ const dotRadius = 1;
+ const xPosition = props.anchor === "middle" ? x + width / 2 : x + width / 4;
+ const xSpacing = 7;
+ const yPosition = props.anchor === "middle" ? y + height - 18 : y + height -
11;
Review Comment:
I wouldn't mind some comments here regarding the magic numbers...
##########
packages/dmn-editor/src/store/computed/computeDiagramData.ts:
##########
@@ -327,6 +343,7 @@ export function computeDiagramData(
return {
drgEdges,
+ drgNodeIdsBySourceNodeId,
Review Comment:
Although correct, I think we could have a better name for this map. I would
call it `directDepenentsByNodeId`. Or even `drgAdjacencyList`. Note that the
DRG of a DMN can include nodes frm other DMNs too, so a DRG is not simply
`definitions.drgElement`, as `definitions.drgElement` represents the local
elements of the DRG, but since there can be references to external DRG
elements, a DRG includes both local and external nodes.
##########
packages/dmn-editor/src/svg/DmnDiagramSvg.tsx:
##########
@@ -59,6 +59,7 @@ import { Text } from "@visx/text";
import { TypeOrReturnType } from "../store/ComputedStateCache";
Review Comment:
Lint warning here. Missing hook dependencies.
--
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]
