[4348] Move quick tools declaration on the backend #4349

mcharfadi · 2024-12-27T09:02:16Z

Bug: #4348

Pull request template

General purpose

What is the main goal of this pull request?

Project management

Has the pull request been added to the relevant project and milestone? (Only if you know that your work is part of a specific iteration such as the current one)
Have the priority: and pr: labels been added to the pull request? (In case of doubt, start with the labels priority: low and pr: to review later)
Have the relevant issues been added to the pull request?
Have the relevant labels been added to the issues? (area:, difficulty:, type:)
Have the relevant issues been added to the same project and milestone as the pull request?
Has the CHANGELOG.adoc been updated to reference the relevant issues?
Have the relevant API breaks been described in the CHANGELOG.adoc? (Including changes in the GraphQL API)
In case of a change with a visual impact, are there any screenshots in the CHANGELOG.adoc? For example in doc/screenshots/2022.5.0-my-new-feature.png

Architectural decision records (ADR)

Does the title of the commit contributing the ADR start with [doc]?
Are the ADRs mentioned in the relevant section of the CHANGELOG.adoc?

Dependencies

Are the new / upgraded dependencies mentioned in the relevant section of the CHANGELOG.adoc?
Are the new dependencies justified in the CHANGELOG.adoc?

Frontend

This section is not relevant if your contribution does not come with changes to the frontend.

General purpose

Is the code properly tested? (Plain old JavaScript tests for business code and tests based on React Testing Library for the components)

Typing

We need to improve the typing of our code, as such, we require every contribution to come with proper TypeScript typing for both changes contributing new files and those modifying existing files.
Please ensure that the following statements are true for each file created or modified (this may require you to improve code outside of your contribution).

Backend

This section is not relevant if your contribution does not come with changes to the backend.

General purpose

Are all the event handlers tested?
Are the event processor tested?
Is the business code (services) tested?
Are diagram layout changes tested?

Architecture

Are data structure classes properly separated from behavioral classes?
Are all the relevant fields final?
Is any data structure mutable? If so, please write a comment indicating why
Are behavioral classes either stateless or side effect free?

Review

How to test this PR?

Please describe here the various use cases to test this pull request

Has the Kiwi TCMS test suite been updated with tests for this contribution?

Bug: #4348 Signed-off-by: Michaël Charfadi <[email protected]>

sbegaudeau · 2025-02-04T10:36:06Z

...ain/java/org/eclipse/sirius/components/collaborative/diagrams/api/DiagramImageConstants.java

@@ -19,6 +19,8 @@
 */
 public final class DiagramImageConstants {

+    public static final String HIDE_SVG = "/icons/full/obj16/HideTool.svg";


Where is this image coming from? Do not add coupling between unrelated modules. Is that coming from the diagram view DSL? If so, sirius-components-collaborative-diagrams can be reused with the view DSL so it cannot be referenced this way. Since I don't see any reference to this constant anywhere in sirius-components-collaborative-diagrams, you can remove it entirely

sbegaudeau · 2025-02-04T10:36:47Z

...grams/frontend/sirius-components-diagrams/src/renderer/collapse/useCollapseExpandElement.tsx

+  >(updateCollapsingStateMutation);
+
+  const collapseExpandElement = useCallback(
+    async (nodeId: string, collapsingState: GQLCollapsingState) => {


Do not use async not await

Yes it was already used so I did not change it but if there was no reason to use async I can remove it.

sbegaudeau · 2025-02-04T10:37:07Z

...grams/frontend/sirius-components-diagrams/src/renderer/collapse/useCollapseExpandElement.tsx

+      const { data: collapseNodeData } = await collapseExpandMutation({ variables: { input } });
+      if (collapseNodeData) {
+        const { collapseExpandDiagramElement } = collapseNodeData;
+        if (isErrorPayload(collapseExpandDiagramElement)) {
+          addMessages(collapseExpandDiagramElement.messages);
+        }
+      }


Just write this kind of code as we always do

sbegaudeau · 2025-02-04T10:42:54Z

...iagrams/frontend/sirius-components-diagrams/src/renderer/delete/useDiagramDeleteMutation.tsx

+const isSuccessPayload = (payload: GQLDeleteFromDiagramPayload): payload is GQLDeleteFromDiagramSuccessPayload =>
+  payload.__typename === 'DeleteFromDiagramSuccessPayload';
+
+export const useDiagramDeleteMutation = (): UseDiagramDeleteMutation => {


This hook should probably be named useDeleteFromDiagram to stay consistent with our naming conventions

sbegaudeau · 2025-02-04T10:43:20Z

...iagrams/frontend/sirius-components-diagrams/src/renderer/delete/useDiagramDeleteMutation.tsx

+  const deleteElements = useCallback(
+    async (nodeIds: string[], edgesId: string[], deletePocily: GQLDeletionPolicy) => {
+      const input: GQLDeleteFromDiagramInput = {
+        id: crypto.randomUUID(),
+        editingContextId,
+        representationId: diagramId,
+        nodeIds: nodeIds,
+        edgeIds: edgesId,
+        deletionPolicy: deletePocily,
+      };
+
+      const { data } = await deleteElementsMutation({ variables: { input } });
+      if (data) {
+        const { deleteFromDiagram } = data;
+        if (isErrorPayload(deleteFromDiagram) || isSuccessPayload(deleteFromDiagram)) {
+          addMessages(deleteFromDiagram.messages);
+        }
+      }
+    },
+    [editingContextId, diagramId, deleteElementsMutation]
+  );


Don't use async / await, we have tons of hooks using mutations, just do the same thing

sbegaudeau · 2025-02-04T10:44:30Z

...sirius-components-diagrams/src/renderer/palette/tools/useSingleClickOnDiagramElementTool.tsx

+          variables,
+        };
+
+        const { data } = await invokeSingleClickOnDiagramElementTool({


Don't use async / await

sbegaudeau · 2025-02-04T10:44:31Z

...sirius-components-diagrams/src/renderer/palette/tools/useSingleClickOnDiagramElementTool.tsx

+  >(invokeSingleClickOnDiagramElementToolMutation);
+
+  const executeTool = useCallback(
+    async (tool: GQLTool, variables: GQLToolVariable[]) => {


Don't use async / await

sbegaudeau · 2025-02-04T10:45:42Z

...es/diagrams/frontend/sirius-components-diagrams/src/renderer/palette/tools/useInvokeTool.tsx

+  const { fadeDiagramElements } = useFadeDiagramElements();
+  const { hideDiagramElements } = useHideDiagramElements();
+  const { pinDiagramElements } = usePinDiagramElements();
+  const { collapseExpandElement } = useCollapseExpandElement();
+  const { deleteElements } = useDiagramDeleteMutation();
+  const { showDeletionConfirmation } = useDeletionConfirmationDialog();
+  const { invokeSingleClickTool } = useSingleClickOnDiagramElementTool({
+    x,
+    y,
+    diagramElementId,
+    targetObjectId,
+    onDirectEditClick,
+  });
+
+  const { nodeLookup, edgeLookup } = useStoreApi<Node<NodeData>, Edge<EdgeData>>().getState();
+  const { hideDiagramPalette, setLastToolInvoked } = useDiagramPalette();
+  const { hideDiagramElementPalette } = useDiagramElementPalette();


Is that a good idea to create some coupling between all these concepts?

It seems to me that the coupling was already there in usePalette.

With this PR, we start to have a hook per mutation that only do the mutation, so it can be reused anywhere and even could be exported later on.

I think we could still reduce the coupling between useInvokeTool and usePalette.

The palette should probably call usePalette in order to only execute the query and useInvokeTool to get only handleToolClick would that be be better ? That would allow us to reuse useInvokeTool too.

Theses changes are helpful to reduce the complexity of the code and later on merge all palettes into one so we don't have the palette in the nodes and in order to simplify the states related to the palette.

mcharfadi added pr: to review soon priority: low labels Dec 27, 2024

mcharfadi added this to the 2025.2.0 milestone Dec 27, 2024

mcharfadi requested review from pcdavid and gcoutable as code owners December 27, 2024 09:02

mcharfadi linked an issue Dec 27, 2024 that may be closed by this pull request

Move quick tools declaration on the backend #4348

Open

mcharfadi force-pushed the mch/enh/quicktools branch 4 times, most recently from 57cca5c to 482ede5 Compare December 30, 2024 08:07

[4348] Move quick tools declaration on the backend

a62964f

Bug: #4348 Signed-off-by: Michaël Charfadi <[email protected]>

sbegaudeau force-pushed the mch/enh/quicktools branch from 482ede5 to a62964f Compare January 28, 2025 16:53

sbegaudeau requested changes Feb 4, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4348] Move quick tools declaration on the backend #4349

[4348] Move quick tools declaration on the backend #4349

mcharfadi commented Dec 27, 2024 •

edited

Loading

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

mcharfadi Feb 4, 2025

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

sbegaudeau Feb 4, 2025

mcharfadi Feb 4, 2025

[4348] Move quick tools declaration on the backend #4349

Are you sure you want to change the base?

[4348] Move quick tools declaration on the backend #4349

Conversation

mcharfadi commented Dec 27, 2024 • edited Loading

Pull request template

General purpose

Project management

Architectural decision records (ADR)

Dependencies

Frontend

General purpose

Typing

Backend

General purpose

Architecture

Review

How to test this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcharfadi commented Dec 27, 2024 •

edited

Loading