[2759] Reactivate the SelectionDialog #3658

florianbarbin · 2024-06-20T15:31:58Z

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?

Import the Studio
Studio.zip
This studio is based on default one but with two additional tools:
- One in the diagram palette which renamed the selected element
- One on the Entity1 node that also open a selection dialog to rename the selected element
Create a new project
Create a new model based on the studio MM
Create a new diagram
Create some element by using the node creation tools
Use the selectionTool in the diagram contextual palette:
- The selection dialog should be opened with all the model elements selectable
- Select an element and perform finished
- The element should be renamed
Use the selection tool on an Entity1
The selection dialog should only contain the selected element
Select it and perform finish
The element should be renamed
Has the Kiwi TCMS test suite been updated with tests for this contribution?

packages/view/backend/sirius-components-view-diagram-edit/src/main/resources/plugin.properties

packages/diagrams/frontend/sirius-components-diagrams/src/renderer/palette/Palette.tsx

packages/diagrams/frontend/sirius-components-diagrams/src/dialog/DialogContext.tsx

...rc/main/java/org/eclipse/sirius/components/compatibility/services/diagrams/ToolProvider.java

CHANGELOG.adoc

AxelRICHARD · 2024-06-27T08:52:20Z

packages/selection/frontend/sirius-components-selection/src/SelectionDialog.tsx

@@ -174,7 +176,7 @@ export const SelectionDialog = ({
            color="primary"
            onClick={() => {
              if (selectedObjectId) {
-                onFinish(selectedObjectId);
+                onFinish([{ name: 'selectedObject', value: selectedObjectId, type: GQLToolVariableType.objectId }]);


It seems you create a new GQLToolVariable with the selectedObjectId as value, but you name it selectedObject. Why you don't name it selectedObjectId?

The "specifier" will manipulate the object (the variable value will be the object and not its id)

So the "operation" that will transform an objectId to an object is done on the backend right?
So why you have to create a variable here, in the frontend?

The idea is that the backend part does not make any assumptions on the variables provided by the frontend component. For now, we have identified 3 kinds of values which can be provided by a Dialog: An Object: in that case we use the Id. An array of objects or a simple string.

I get that, but it still seems strange.
Here you set the variable type to GQLToolVariableType.objectId which indicates that the variable will contain an objectId, but you set the variable name to selectedObject which indicates that the variable will contain an Object

The type represents a technical information to describe which kind of value is set.
We can change the variable type by object instead of objectId, in that case, it will represent the value type once converted by the backend side.

Here you set the variable type to GQLToolVariableType.objectId which indicates that the variable will contain an objectId, but you set the variable name to selectedObject which indicates that the variable will contain an Object

No that's not how we should think about this. As the contributor of a dialog, I want my dialog to be used to create a variable named selectedObject. To populate this variable, I'll give the backend some string as the value and it should be interpreted as the identifier of an object to resolve.

CHANGELOG.adoc

sbegaudeau · 2024-07-02T14:06:34Z

CHANGELOG.adoc

+}
+```
+
+ * The `SingleClickOnDiagramElementTool#dialogDescriptionId` has been replaced by a `dialog: Dialog` field to provide the information about the dialog that the frontend needs to retrieve in the extension point.


Why isn't that a XXXDescription? Could you please use something like ToolDialogDescription or DiagramToolDialogDescription in order not to use such a common name which may be used elsewhere in the future.

Why the need for a whole concept instead of of the identifier?

It idea was to own both the id and the information about the dialog type. We need to change the id to own the information about the dialog type if we want to keep only a dialogDescriptionId field?

I still don't know why we would need both this two pieces of information. Why isn't an identifier enough?

I am modifying the way we generate this identifier to have something similar to the other representation: siriusComponents://dialogDescription?kind=selectionDialogDescription etc"

sbegaudeau · 2024-07-02T14:07:18Z

.../backend/sirius-components-collaborative-diagrams/src/main/resources/schema/diagram.graphqls

+  invokeSingleClickOnDiagramElementTool(
+    input: InvokeSingleClickOnDiagramElementToolInput!


No need to change the formatting

sbegaudeau · 2024-07-02T14:07:34Z

.../backend/sirius-components-collaborative-diagrams/src/main/resources/schema/diagram.graphqls

+  invokeSingleClickOnDiagramElementTool(
+    input: InvokeSingleClickOnDiagramElementToolInput!
+  ): InvokeSingleClickOnDiagramElementToolPayload!
+  invokeSingleClickOnTwoDiagramElementsTool(


No need to change the formatting

sbegaudeau · 2024-07-02T14:09:45Z

.../backend/sirius-components-collaborative-diagrams/src/main/resources/schema/diagram.graphqls

+  dialogDescriptionId: String!
+  dialogDescriptionType: String!


This should be a XxxDescription or be removed entirely since I'm not sure why it needs to be a dedicated type?

You could use id and type

I'm not sure why this concept isn't an interface and why you need more than id?

The id currently contains only an UUID that does not allow to identify the dialog type.
If we want to keep only an id, we need to modify the way we generate this id to own the dialog type information?

Ok so that's the limitation that I was missing. We should indeed change the way this identifier is computed. I don't even know how a simple UUID can be enough to open a dialog. For other description elements, the identifier has way more information. For example, the identifier of a node description is siriusComponents://nodeDescription?sourceKind=view&sourceId=c5857f07-7382-3215-8c53-b690ca983655&sourceElementId=fe82e80e-7308-35a9-986d-f010401063ee.

So, for a selection dialog, having an identifier like this would be necessary:

siriusComponents://selectionDialogDescription?sourceKind=view&sourceId=XXX&sourceElementId=XXXX

I don't know how we could retrieve the proper Dialog object otherwise since we don't know the id of its view model.

sbegaudeau · 2024-07-02T14:14:44Z

...s-components-diagrams/src/main/java/org/eclipse/sirius/components/diagrams/tools/Dialog.java

+ *
+ * @author fbarbin
+ */
+public record Dialog(String dialogDescriptionId, String dialogDescriptionType) {


This will have to be renamed not to use such a common concept name

sbegaudeau · 2024-07-02T14:24:23Z

...diagrams/frontend/sirius-components-diagrams/src/dialog/diagramDialogExtensionPoint.types.ts

+export interface DialogComponentProps {
+  editingContextId: string;
+  dialogDescriptionId: string;
+  targetObjectId: string;
+  onClose: () => void;
+  onFinish: (variables: GQLToolVariable[]) => void;


I'm pretty sure that the editingContextId is not needed and the targetObjectId too.

done, replaced by the useDialog hook to retrieve the editingContext from the DialogContext

sbegaudeau · 2024-07-02T14:24:36Z

...diagrams/frontend/sirius-components-diagrams/src/dialog/diagramDialogExtensionPoint.types.ts

+
+import { GQLToolVariable } from '../renderer/palette/Palette.types';
+export interface DiagramDialogContribution {
+  canHandle: (dialogTypeId: string) => boolean;


Why not use the descriptionId? Why have two pieces of information?

The descriptionId is just an UUID

removed and replaced by the descriptionId that now contains the type information.

sbegaudeau · 2024-07-02T14:25:44Z

packages/selection/frontend/sirius-components-selection/src/SelectionDialog.tsx

  targetObjectId,
  onClose,
  onFinish,
-}: SelectionDialogProps) => {
+}: DialogComponentProps) => {


DialogComponentProps is too generic

sbegaudeau · 2024-07-02T14:27:38Z

packages/diagrams/frontend/sirius-components-diagrams/src/renderer/palette/Palette.types.ts

+  type: GQLToolVariableType;
+}
+
+export enum GQLToolVariableType {


Do not use enum in TypeScript, it creates a weird result, use an union instead:

`export type GQLToolVariableType = 'STRING' | 'OBJECT_ID' | 'OBJECT_ID_ARRAY';

sbegaudeau · 2024-07-02T14:31:38Z

packages/selection/frontend/sirius-components-selection/src/SelectionDialog.tsx

@@ -174,7 +176,7 @@ export const SelectionDialog = ({
            color="primary"
            onClick={() => {
              if (selectedObjectId) {
-                onFinish(selectedObjectId);
+                onFinish([{ name: 'selectedObject', value: selectedObjectId, type: GQLToolVariableType.objectId }]);


Here you set the variable type to GQLToolVariableType.objectId which indicates that the variable will contain an objectId, but you set the variable name to selectedObject which indicates that the variable will contain an Object

No that's not how we should think about this. As the contributor of a dialog, I want my dialog to be used to create a variable named selectedObject. To populate this variable, I'll give the backend some string as the value and it should be interpreted as the identifier of an object to resolve.

AxelRICHARD · 2024-07-10T11:58:04Z

Please squash the commits before the merge

sbegaudeau

I'll have to review some details while running the code and I'll fix the last small issue remaining to merge it

...main/java/org/eclipse/sirius/components/collaborative/selection/SelectionEventProcessor.java

sbegaudeau · 2024-07-17T20:26:10Z

packages/selection/frontend/sirius-components-selection/package.json

@@ -32,6 +32,7 @@
  "peerDependencies": {
    "@apollo/client": "3.10.4",
    "@eclipse-sirius/sirius-components-core": "*",
+    "@eclipse-sirius/sirius-components-diagrams": "*",


I'm not sure why selection needs to depend on diagrams now...

...diagrams/frontend/sirius-components-diagrams/src/dialog/diagramDialogExtensionPoint.types.ts

packages/diagrams/frontend/sirius-components-diagrams/src/dialog/diagramDialogExtensionPoint.ts

sbegaudeau · 2024-07-17T20:32:20Z

packages/diagrams/frontend/sirius-components-diagrams/src/dialog/DialogContext.types.ts

+import { GQLToolVariable } from '../renderer/palette/Palette.types';
+
+export interface DialogContextValue {
+  currentEditingContextId: string | undefined;


Why this currentEditingContextId and why can it be undefined? even null would be very odd

sbegaudeau · 2024-07-17T20:47:48Z

.../java/org/eclipse/sirius/web/services/diagrams/ModelOperationDiagramDescriptionProvider.java

@@ -61,6 +62,8 @@ public class ModelOperationDiagramDescriptionProvider implements IEditingContext

    private NodeTool createNodeTool;

+    private NodeTool renameNodeTool;


A dedicated example should be created instead of just taking one to change the scope of its behavior

packages/view/backend/sirius-components-view-diagram-edit/src/main/resources/plugin.properties

...iew-emf/src/main/java/org/eclipse/sirius/components/view/emf/diagram/IDiagramIdProvider.java

...c/main/java/org/eclipse/sirius/components/view/emf/IRepresentationDescriptionIdProvider.java

sbegaudeau · 2024-07-17T21:00:31Z

...clipse/sirius/web/application/controllers/diagrams/ModelOperationDiagramControllerTests.java

@@ -130,4 +129,48 @@ public void givenDiagramWhenToolWithComplexModelOperationsIsExecutedThenItWorksA
                .thenCancel()
                .verify(Duration.ofSeconds(10));
    }
+
+    @Test


Can be moved to a dedicated test suite

sbegaudeau · 2024-07-19T19:30:39Z

So now things are working as expected but I still have two elements to review and possibly modify (the dependency to sirius-components-diagrams and the tests)

sbegaudeau · 2024-07-19T19:43:01Z

I've reduced the dependency to sirius-components-diagrams as much as possible but it will still remain for now since the impact is minimal, we will just have to ensure that it does not grow over time.

/cc @florianbarbin When we will work on the support for dialogs in the explorer, we will have to move DialogDescription in view.ecore (from diagram.ecore now), rename DiagramDialogComponentProps to DialogComponentProps and move it to sirius-components-core.

Let's not add more dependencies to sirius-components-diagrams in sirius-components-selection.

In addition, this commit introduces the generic concept of Dialog to make it possible to define any kind of dialog for diagram node tools. Bug: #2759 Signed-off-by: Florian Barbin <[email protected]>

sbegaudeau · 2024-07-19T20:55:33Z

Some tests will have to be moved later

florianbarbin added pr: to review soon priority: high labels Jun 20, 2024

florianbarbin added this to the 2024.7.0 milestone Jun 20, 2024

florianbarbin requested review from pcdavid, sbegaudeau, AxelRICHARD and gcoutable as code owners June 20, 2024 15:31

florianbarbin linked an issue Jun 20, 2024 that may be closed by this pull request

A tool cannot open a selection dialog in react flow #2759

Closed

1 task

florianbarbin force-pushed the fba/enh/SelectionDialog branch 4 times, most recently from 4c139c8 to 578411d Compare June 25, 2024 07:13

AxelRICHARD reviewed Jun 25, 2024

View reviewed changes

packages/view/backend/sirius-components-view-diagram-edit/src/main/resources/plugin.properties Outdated Show resolved Hide resolved

packages/diagrams/frontend/sirius-components-diagrams/src/renderer/palette/Palette.tsx Outdated Show resolved Hide resolved

florianbarbin force-pushed the fba/doc/adr_152 branch from a66d309 to 58c29f7 Compare June 25, 2024 13:57

florianbarbin force-pushed the fba/enh/SelectionDialog branch from 0ddba48 to f048137 Compare June 25, 2024 14:56

florianbarbin changed the title ~~WIP [2759] Reactivate the SelectionDialog~~ [2759] Reactivate the SelectionDialog Jun 25, 2024

florianbarbin force-pushed the fba/enh/SelectionDialog branch 3 times, most recently from b75142f to a20983f Compare June 26, 2024 15:27

florianbarbin requested a review from AxelRICHARD June 27, 2024 08:34

AxelRICHARD reviewed Jun 27, 2024

View reviewed changes

florianbarbin force-pushed the fba/enh/SelectionDialog branch from cf524ec to f830573 Compare June 27, 2024 13:55

AxelRICHARD reviewed Jun 27, 2024

View reviewed changes

CHANGELOG.adoc Show resolved Hide resolved

florianbarbin force-pushed the fba/enh/SelectionDialog branch 2 times, most recently from 29bc1b4 to 93b4a08 Compare June 28, 2024 14:06

florianbarbin force-pushed the fba/doc/adr_152 branch from 58c29f7 to f98960d Compare June 28, 2024 14:08

sbegaudeau requested changes Jul 2, 2024

View reviewed changes

florianbarbin requested a review from sbegaudeau July 2, 2024 15:10

florianbarbin force-pushed the fba/enh/SelectionDialog branch from 93b4a08 to 1edcd95 Compare July 3, 2024 16:37

florianbarbin force-pushed the fba/doc/adr_152 branch from f98960d to ebbdf3e Compare July 3, 2024 16:39

florianbarbin force-pushed the fba/enh/SelectionDialog branch 4 times, most recently from 3f6485e to f9ec7fb Compare July 8, 2024 13:12

florianbarbin force-pushed the fba/doc/adr_152 branch 2 times, most recently from 98497f7 to de754a8 Compare July 10, 2024 10:03

florianbarbin force-pushed the fba/enh/SelectionDialog branch from f9ec7fb to c022bec Compare July 10, 2024 10:10

florianbarbin changed the base branch from fba/doc/adr_152 to master July 10, 2024 10:13

florianbarbin force-pushed the fba/enh/SelectionDialog branch 6 times, most recently from 46095ef to e444ba2 Compare July 12, 2024 16:25

sbegaudeau requested changes Jul 17, 2024

View reviewed changes

sbegaudeau force-pushed the fba/enh/SelectionDialog branch 4 times, most recently from 4670a4b to 051527c Compare July 19, 2024 19:29

sbegaudeau force-pushed the fba/enh/SelectionDialog branch from 051527c to 06bd199 Compare July 19, 2024 20:32

[2759] Reactivate the SelectionDialog

89cdf78

In addition, this commit introduces the generic concept of Dialog to make it possible to define any kind of dialog for diagram node tools. Bug: #2759 Signed-off-by: Florian Barbin <[email protected]>

sbegaudeau force-pushed the fba/enh/SelectionDialog branch from 06bd199 to 89cdf78 Compare July 19, 2024 20:35

sbegaudeau approved these changes Jul 19, 2024

View reviewed changes

sbegaudeau merged commit 1aca131 into master Jul 19, 2024
4 checks passed

sbegaudeau deleted the fba/enh/SelectionDialog branch July 19, 2024 20:55

		invokeSingleClickOnDiagramElementTool(
		input: InvokeSingleClickOnDiagramElementToolInput!

		@@ -61,6 +62,8 @@ public class ModelOperationDiagramDescriptionProvider implements IEditingContext

		private NodeTool createNodeTool;

		private NodeTool renameNodeTool;

[2759] Reactivate the SelectionDialog #3658

[2759] Reactivate the SelectionDialog #3658

Conversation

florianbarbin commented Jun 20, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AxelRICHARD commented Jul 10, 2024

sbegaudeau left a 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

sbegaudeau commented Jul 19, 2024

sbegaudeau commented Jul 19, 2024

sbegaudeau commented Jul 19, 2024

florianbarbin commented Jun 20, 2024 •

edited

Loading