[3763] Implements the Selection Dialog TreeView #3870

florianbarbin · 2024-08-19T14:19:14Z

Bug: #3763

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 both :
testSelectionDialogModel.zip
TestSelectionDialogStudio.zip
Open the diagram in testSelectionDialogModel
Use the selection tool
Select an existing element (only entity1 are selectable)
Perform finish : the element should be renamed
Has the Kiwi TCMS test suite been updated with tests for this contribution?

AxelRICHARD

The coverage code fails. You should add some tests.
With this PR, it is not possible anymore to create a SelectionDialog with a list. Only trees are available. I didn't remember we talked about removing the support of the list to let only trees.
Furthermore, In the case we have decided that we only support trees in SelectionDialog, what about existing SelectionDialog with list? It seems they are not migrated.

florianbarbin · 2024-08-20T08:12:27Z

The coverage code fails. You should add some tests. With this PR, it is not possible anymore to create a SelectionDialog with a list. Only trees are available. I didn't remember we talked about removing the support of the list to let only trees. Furthermore, In the case we have decided that we only support trees in SelectionDialog, what about existing SelectionDialog with list? It seems they are not migrated.

Yes I know about the code coverage. I see what I can do but for the selection.graphql module it will be complicated to retrieve 100%

About the Tree, we discussed it with Stephane (the ADR will be updated). And we decided to keep only the Tree since if we want a flat list, we can just provide all elements as roots.
Normally, the migration from the previous commit should do the job to keep the existing SelectionDialog running.

AxelRICHARD · 2024-08-20T09:01:43Z

The coverage code fails. You should add some tests. With this PR, it is not possible anymore to create a SelectionDialog with a list. Only trees are available. I didn't remember we talked about removing the support of the list to let only trees. Furthermore, In the case we have decided that we only support trees in SelectionDialog, what about existing SelectionDialog with list? It seems they are not migrated.

Yes I know about the code coverage. I see what I can do but for the selection.graphql module it will be complicated to retrieve 100%

About the Tree, we discussed it with Stephane (the ADR will be updated). And we decided to keep only the Tree since if we want a flat list, we can just provide all elements as roots. Normally, the migration from the previous commit should do the job to keep the existing SelectionDialog running.

Ok to let only the tree in this case, but it seems the migration does not work.

florianbarbin · 2024-08-20T09:04:17Z

Ok to let only the tree in this case, but it seems the migration does not work.

Ok I will have a look.

sbegaudeau · 2024-08-26T10:25:58Z

I've rebased the PR to prepare its review, I'll try to review it this afternoon

sbegaudeau

I'll perform the minor changes necessary and update the PR to merge it

sbegaudeau · 2024-09-16T20:15:03Z

...ain/java/org/eclipse/sirius/components/view/emf/api/SelectionDialogDescriptionConverter.java

+ *
+ * Contributors:
+ *     Obeo - initial API and implementation
+ *******************************************************************************/


Wrong location for this class, this is not an API and this is related to diagrams

sbegaudeau · 2024-09-16T20:15:39Z

...ain/java/org/eclipse/sirius/components/view/emf/api/SelectionDialogDescriptionConverter.java

+    private Boolean hasChildren(AQLInterpreter interpreter, SelectionDialogTreeDescription selectionDialogTreeDescription, VariableManager variableManager,  Function<VariableManager, Boolean> isSelectableProvider) {
+        boolean hasChildren = false;
+        Object self = variableManager.getVariables().get(VariableManager.SELF);
+        hasChildren = !this.computeChildrenFromExpression(variableManager, interpreter, selectionDialogTreeDescription).isEmpty();
+        return hasChildren && this.hasCompatibleDescendants(interpreter, selectionDialogTreeDescription, variableManager, self, false, isSelectableProvider);
+    }
+
+    private boolean hasCompatibleDescendants(AQLInterpreter interpreter, SelectionDialogTreeDescription selectionDialogTreeDescription, VariableManager variableManager, Object object, boolean isDescendant, Function<VariableManager, Boolean> isSelectableProvider) {
+        VariableManager childVariableManager = variableManager.createChild();
+        childVariableManager.put(VariableManager.SELF, object);
+        return isDescendant && isSelectableProvider.apply(childVariableManager)
+                || this.computeChildrenFromExpression(childVariableManager, interpreter, selectionDialogTreeDescription).stream().anyMatch(eContent -> this.hasCompatibleDescendants(interpreter, selectionDialogTreeDescription, childVariableManager, eContent, true, isSelectableProvider));
+    }


This is MASSIVELY expensive for something which shouldn't be. You can't start recursively navigating the whole editing context like that and certainly not simply to answer the question "hasChildren". If the specifier choses to display useless data in the dialog that's their choice not ours.

So for you it is the specifier responsibility to define this rules?
It can be complicated to implement in the studio

Let's consider the following tree:

- A - B - C - D - E - F (with 1 million elements under F)

For every semantic change in the workbench, for each tree item displayed (here A, B, C, D, E, F) we will execute hasChildren (so 6 times per refresh) which will each perform an eAllContents on the whole tree just because we want to be smarter than the specifier who has told us to display something by trying to not display potentially useless stuff. That's a massive performance penalty for everybody on the whole project just because ONE person has opened the selection dialog.

If the specifier tells us to display something, we display it. I can understand the intention behind this feature but we can't accept such a performance it for something like that. On top of it, I disagree with the need for such feature. One may need to see the children of an element in order to decide which one should be selected. I don't see why we should decide to hide something that the specifier has explicitly told us to display thanks to the childrenExpression. If the specifiers wants to perform eAllContents everywhere that's their choice not ours.

I understand the arguments. For information, I used the same logic we already have in org.eclipse.sirius.components.collaborative.widget.reference.browser.ModelBrowsersDescriptionProvider.

(●__● )

In that case, I may have missed something there. I'll look into it...

( ｀▽´)Ψ

FWIW this behavior in ModelBrowsersDescriptionProvider was added explicitly via #2171 (commit a76a04b). I wrote the issue (and PR), but form what I remember this was originally from user feedback on the first version.

sbegaudeau · 2024-09-16T20:27:37Z

packages/selection/backend/sirius-components-collaborative-selection/pom.xml

+			<artifactId>
+				sirius-components-collaborative-trees
+			</artifactId>


Should be on one line

sbegaudeau · 2024-09-17T07:53:42Z

...ius/components/collaborative/selection/handlers/SelectionDescriptionMessageEventHandler.java

+        if (input instanceof SelectionDescriptionMessageInput selectionDescriptionMessageInput) {
+            String targetObjectId = selectionDescriptionMessageInput.targetObjectId();
+            Optional<Object> optionalTargetObject = this.objectService.getObject(editingContext, targetObjectId);
+            if (optionalTargetObject.isPresent()) {


You still need to emit a payload in all cases, including if we do not enter the "if"

sbegaudeau · 2024-09-17T07:57:34Z

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

+    if (!loading && selectionDescription) {
+      setState((prevState) => ({
+        ...prevState,
+        message: selectionDescription.message,
+        treeDescriptionId: selectionDescription.treeDescription.id,
+      }));


I'll look a bit more into the code but you don't seems to need a state and useEffect just for that

Indeed, it just a query so the component will be rerender a soon as we receive the data ?

sbegaudeau · 2024-09-17T08:07:17Z

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

@@ -80,59 +31,48 @@ export const SelectionDialog = ({
  onClose,
  onFinish,
 }: DiagramDialogComponentProps) => {
-  const { classes } = useSelectionObjectModalStyles();
+  const initialState: DiagramDialogState = {


SelectionDialogState

sbegaudeau · 2024-09-17T08:07:51Z

packages/selection/frontend/sirius-components-selection/src/useSelectionDescription.types.ts

+  targetObjectId: string;
+}
+
+export interface SelectionDescription {


GQLSelectionDescription to keep indicating that this type comes from the backend

sbegaudeau · 2024-09-17T08:08:00Z

packages/selection/frontend/sirius-components-selection/src/useSelectionDescription.types.ts

+  treeDescription: TreeDescription;
+}
+
+export interface TreeDescription {


GQLTreeDescription

sbegaudeau · 2024-09-17T08:09:47Z

packages/selection/frontend/sirius-components-selection/src/SelectionDialog.types.ts

+  treeDescriptionId: string;
+  message: string;


string or null to properly identify that you don't have the relevant data. Your code only works because if(aVariableContainingAnEmptyString) evaluates to false and it does not enter the if. That's a very narrow assumption that empty string equals false.

And you probably don't need this in the state too

The state and the useEffect were indeed not needed

sbegaudeau · 2024-09-17T08:17:16Z

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

+const useTreeStyle = makeStyles()((theme) => ({
+  borderStyle: {
+    border: '1px solid',
+    borderColor: theme.palette.grey[500],


theme.palette.divider to stay with the same style as the rest of the application

sbegaudeau · 2024-09-17T08:32:04Z

...ain/java/org/eclipse/sirius/components/view/emf/api/SelectionDialogDescriptionConverter.java

+        result.removeIf(object -> {
+            if (object instanceof EObject eObject) {
+                VariableManager childVariableManager = variableManager.createChild();
+                childVariableManager.put(VariableManager.SELF, eObject);
+                return !isSelectableProvider.apply(childVariableManager) && !this.hasChildren(interpreter, selectionDialogTreeDescription, childVariableManager, isSelectableProvider);
+            } else {
+                return false;
+            }
+        });


You can't decide that, let the specifiers display what they want. And you can't decide that without the MASSIVE performance impact from before.

sbegaudeau · 2024-09-17T08:43:43Z

I'll wait for the build, squash the commits and merge the PR

…eView Bug: #3763 Signed-off-by: Florian Barbin <[email protected]>

Signed-off-by: Stéphane Bégaudeau <[email protected]>

sbegaudeau · 2024-09-17T08:51:38Z

I'll wait for the build and merge it 👍

florianbarbin added pr: to review later priority: high labels Aug 19, 2024

florianbarbin added this to the 2024.9.0 milestone Aug 19, 2024

florianbarbin requested review from pcdavid, sbegaudeau and AxelRICHARD as code owners August 19, 2024 14:19

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch from c2a90ac to 0e467aa Compare August 19, 2024 14:20

florianbarbin force-pushed the fba/enh/treeViewForSelectionDialog branch from af8ac8a to c59f3a4 Compare August 19, 2024 14:25

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 0e467aa to 91e4ae5 Compare August 19, 2024 14:26

AxelRICHARD linked an issue Aug 19, 2024 that may be closed by this pull request

Make it possible to display the content of the Selection Dialog using a tree viewer #3763

Closed

1 task

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 91e4ae5 to 45307c2 Compare August 19, 2024 15:37

florianbarbin force-pushed the fba/enh/treeViewForSelectionDialog branch from c59f3a4 to dd123a8 Compare August 20, 2024 08:02

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 45307c2 to f27aab5 Compare August 20, 2024 08:03

AxelRICHARD reviewed Aug 20, 2024

View reviewed changes

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch 3 times, most recently from a37c36e to 539f83d Compare August 21, 2024 14:47

florianbarbin requested a review from gcoutable as a code owner August 21, 2024 14:47

florianbarbin force-pushed the fba/enh/treeViewForSelectionDialog branch from dd123a8 to 251994c Compare August 21, 2024 14:48

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch 4 times, most recently from c4cb422 to 92bc756 Compare August 23, 2024 06:11

sbegaudeau force-pushed the fba/enh/treeViewForSelectionDialog branch from 251994c to eb4c34b Compare August 26, 2024 09:27

Base automatically changed from fba/enh/treeViewForSelectionDialog to master August 26, 2024 09:50

sbegaudeau force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 92bc756 to b6e8baf Compare August 26, 2024 10:25

florianbarbin removed the pr: to review later label Aug 29, 2024

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch 6 times, most recently from 6e5bf9e to 56eeef2 Compare September 5, 2024 15:27

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch 2 times, most recently from 665d5ad to 94f530d Compare September 11, 2024 13:23

sbegaudeau modified the milestones: 2024.9.0, 2024.11.0 Sep 11, 2024

florianbarbin force-pushed the fba/enh/implementTreeViewSelectionDialog branch 3 times, most recently from 7a07b30 to 36df83b Compare September 12, 2024 21:12

sbegaudeau force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 36df83b to bdb118c Compare September 16, 2024 19:48

sbegaudeau requested changes Sep 17, 2024

View reviewed changes

sbegaudeau reviewed Sep 17, 2024

View reviewed changes

florianbarbin and others added 2 commits September 17, 2024 10:51

[3763] Update the Selection Dialog to display its content using a Tre…

96708a8

…eView Bug: #3763 Signed-off-by: Florian Barbin <[email protected]>

[test] Add selection dialog support to Papaya

1d4158d

Signed-off-by: Stéphane Bégaudeau <[email protected]>

sbegaudeau force-pushed the fba/enh/implementTreeViewSelectionDialog branch from 3729fad to 1d4158d Compare September 17, 2024 08:51

sbegaudeau approved these changes Sep 17, 2024

View reviewed changes

sbegaudeau merged commit c03981c into master Sep 17, 2024
4 checks passed

sbegaudeau deleted the fba/enh/implementTreeViewSelectionDialog branch September 17, 2024 09:06

[3763] Implements the Selection Dialog TreeView #3870

[3763] Implements the Selection Dialog TreeView #3870

Conversation

florianbarbin commented Aug 19, 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?

AxelRICHARD left a comment

Choose a reason for hiding this comment

florianbarbin commented Aug 20, 2024

AxelRICHARD commented Aug 20, 2024

florianbarbin commented Aug 20, 2024

sbegaudeau commented Aug 26, 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 Sep 17, 2024 • edited Loading

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

sbegaudeau commented Sep 17, 2024

sbegaudeau commented Sep 17, 2024

florianbarbin commented Aug 19, 2024 •

edited

Loading

sbegaudeau Sep 17, 2024 •

edited

Loading