[3875] Refactor the tree event classes related to the explorer #3876

jerome-obeo · 2024-08-22T16:01:15Z

Bug: #3875

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?

sbegaudeau

It is absolutely critical that the reference widget keeps being usable without the explorer. As such, it will be necessary to have a modelBrowserEvent subscription with its event processor factory.

sbegaudeau · 2024-08-27T07:56:45Z

CHANGELOG.adoc

@@ -109,6 +109,7 @@ A migration participant has been added to automatically keep compatible all diag
 - https://github.com/eclipse-sirius/sirius-web/issues/3777[#3777] [sirius-web] Add support for any kind of object as semantic element in the tree representation
 - https://github.com/eclipse-sirius/sirius-web/issues/3392[#3392] [diagram] Prevent edge from passing through another node
 - https://github.com/eclipse-sirius/sirius-web/issues/3763[#3763] [diagram] Split the SelectionDialogDescription to prepare the Tree support
+- https://github.com/eclipse-sirius/sirius-web/issues/3875[#3875] [sirius-web] Move explorer related code from components-trees to sirius-web-application


sirius-components-trees

sbegaudeau · 2024-08-27T08:03:39Z

...es/forms/frontend/sirius-components-widget-reference/src/components/ModelBrowserTreeView.tsx

@@ -69,6 +69,8 @@ export const ModelBrowserTreeView = ({
          textToHighlight={state.filterBarText}
          markedItemIds={markedItemIds}
          treeItemActionRender={(props) => <WidgetReferenceTreeItemAction {...props} />}
+          eventType="explorerEvent"


I suspect that this make the reference widget unusable without the presence of the explorer. There are applications that are integrating Sirius Components without Sirius Web. Sirius Components cannot make any assumptions on the presence or capabilities of Sirius Web. This will have to change in favor of reference-widget specific capabilities.

Exactly, ModelBrowser has its own modelBrowserEvent subscription now.

sbegaudeau · 2024-08-27T08:11:38Z

...in/java/org/eclipse/sirius/web/application/views/explorer/ExplorerEventProcessorFactory.java

        VariableManager variableManager = new VariableManager();
-        variableManager.put(TreeConfiguration.TREE_ID, treeConfiguration.getId());
+        variableManager.put(ExplorerConfiguration.TREE_ID, treeConfiguration.getId());


Given that we now explicitly what we are looking for since we are not in some generic tree-related code but instead in some explorer related code, could you instead search for the description with the id ExplorerDescriptionProvider#DESCRIPTION_ID.

Thanks to that:

you don't need to have this TREE_ID variable here

you don't need to have a special canCreatPredicate in ExplorerDescriptionProvider

you can remove all usages of this TREE_ID

you don't have to introduce the useless interface ITreeConfiguration to hold this variable

And you will have to provide the concept necessary for the reference widget explicitly in its collaborative project.

You are right, I got rid of this variable.

sbegaudeau · 2024-08-27T08:13:15Z

packages/sirius-web/backend/sirius-web-application/src/main/resources/schema/explorer.graphqls

+
+input ExplorerEventInput {
+  id: ID!
+  treeId: String!


Just as an information, treeId is probably what will be replaced in favor of treeDescriptionId to let the frontend specify to the backend which tree description to use to compute the explorer later.

I suspect that the treeId variable could be fully removed in this PR without any impact, it is now a duplicate of the information contained in the fact that we are using a GraphQL field named explorerEvent.

Unchanged for the moment.
You are right this field will be renamed treeDescriptionId in the future task that tackles the explorer selection.
This ID will be sent from the frontend as the current description to build.

...ain/java/org/eclipse/sirius/web/tests/services/explorer/ExplorerEventSubscriptionRunner.java

sbegaudeau · 2024-08-27T08:15:54Z

...g/eclipse/sirius/web/application/controllers/trees/ModelBrowserExpandAllControllerTests.java

@@ -60,7 +60,7 @@ public class ModelBrowserExpandAllControllerTests extends AbstractIntegrationTes
    private IGivenInitialServerState givenInitialServerState;

    @Autowired
-    private TreeEventSubscriptionRunner treeEventSubscriptionRunner;
+    private ExplorerEventSubscriptionRunner treeEventSubscriptionRunner;


This should make you stop because such dependency between the reference widget from the form representation and the explorer cannot be introduced.

Right, there is a dedicated ModelBrowserEventSubscritionRunner helper class.
The test has been updated to use model browser related classes and not explorer one (EventInput for instance).

sbegaudeau · 2024-08-27T08:19:11Z

packages/trees/frontend/sirius-components-trees/src/views/TreeViewMachine.ts

+const isATreeEventData = (data: GQLTreeEventData | GQLExplorerEventData): data is GQLTreeEventData => {
+  return 'treeEvent' in data;
+};


That's a bit odd and I suspect that it will just become more and more untenable (try fixing the issue with the reference widget with this code and I suspect that it won't be possible without adding odds dependencies). I suspect that we will have to move to the same situation as in forms with the subscription moved outside of TreeView with a dedicated useExplorerEvent subscription and a TreeView component which would simply receive the payload.

Yes, the subscription has been moved outside the TreeView component. This is now the parent component of TreeView, which is in charge of managing the subscription. For instance, ModelBrowserTreeView retrieves the tree through the subscription and passes it to its inner TreeView via props.

sbegaudeau · 2024-08-27T08:20:42Z

packages/trees/frontend/sirius-components-trees/src/views/getTreeEventSubscription.ts

@@ -10,11 +10,11 @@
 * Contributors:
 *     Obeo - initial API and implementation
 *******************************************************************************/
-export const getTreeEventSubscription = (depth: number): string => {
+export const getTreeEventSubscription = (depth: number, eventType: string, eventInputType: string): string => {


From what I've seen in the code, if I'm right, you will have to duplicate this subscription to have a getExplorerEventSubscription in sirius-web-application instead and just keep the function recursiveGetChildren and the fragment here...

sbegaudeau · 2024-08-27T08:25:54Z

You should also keep an eye on this PR by @florianbarbin since it will introduce yet another usage of the tree representation. I don't know yet which PR will be merged first but the other one will have to be updated to take into account these changes.

On the same topic, the previous PR from Florian Barbin introduced another basic version of a TreeDescription in the view DSL in order to leverage it in the selection dialog. One of the goal of the future tree description that you will add in the view DSL is to replace both this one dedicated to the selection dialog (which is on purpose very minimal) and the one powering the form representation.

jerome-obeo · 2024-08-28T11:40:04Z

PR updated.
A new commit contains changes responding to comments.

sbegaudeau

I'll update the PR with fixes for my latest comments. After that I'll rebase it and merge it. No need to touch anything now

sbegaudeau · 2024-09-12T09:01:28Z

packages/trees/frontend/sirius-components-trees/src/views/TreeView.tsx

-      },
-    }
-  );
+  const [state, setState] = useState<TreeViewComponentState>({


TreeViewState to match our naming conventions

sbegaudeau · 2024-09-12T09:03:59Z

packages/trees/frontend/sirius-components-trees/src/views/TreeView.types.ts

@@ -16,42 +16,24 @@ import { TreeItemActionProps } from '../treeitems/TreeItemAction.types';

 export interface TreeViewComponentProps extends WorkbenchViewComponentProps {
  treeId: string;
+  tree: GQLTree;


The TreeView component cannot be used directly as a workbench view anymore, it does not have to keep extending WorkbenchViewComponentProps it doesn't make sense. ExplorerView needs to do it (which it does) but not TreeView.

sbegaudeau · 2024-09-12T09:18:26Z

packages/trees/frontend/sirius-components-trees/src/views/TreeView.types.ts

+
+export interface GQLTreeEventPayload {
+  __typename: string;
+}
+
+export interface GQLTreeRefreshedEventPayload extends GQLTreeEventPayload {
+  id: string;
+  tree: GQLTree;
+}
+
+export interface UseTreeSubscriptionValue {
+  loading: boolean;
+  tree: GQLTree | null;
+  complete: boolean;
+}
+
+export interface UseTreeSubscriptionState {
+  id: string;
+  tree: GQLTree | null;
+  complete: boolean;
+}


These are not the concern of sirius-components-trees it's not used in this npm package.

By putting this code here, you are trying to avoid having two almost identical copies of those types in useModelBrowserSubscription and useExplorerSubscription. Unless we have some maintainability issue with the code, some duplicated code is not an issue I care about. On the other hand, useless code (for sirius-components-trees) and useless coupling are both of the most critical issues that I'm trying to avoid.

Removing duplicated code always comes with one critical tradeoff, creating additional coupling.

Now both the lifecycle the explorer and the model browser are tied together with those types. What if someone wants to change one of those? Their first instinct will be to update UseTreeSubscriptionState and we will end up with useless code for the other one.

The state of useExplorerSubscription needs to fulfill the expectations of useExplorersubscription and the state of useModelBrowserSubscription needs to fulfill the expectations of useModelBrowserSubscription. It just happens that today both have the same expectations.

sbegaudeau · 2024-09-12T09:23:07Z

...es/forms/frontend/sirius-components-widget-reference/src/components/ModelBrowserTreeView.tsx

-          markedItemIds={markedItemIds}
-          treeItemActionRender={(props) => <WidgetReferenceTreeItemAction {...props} />}
-        />
+        {tree && (


Don't use this way for conditional rendering, be explicit because the implicit casting in JavaScript for boolean is funky

sbegaudeau · 2024-09-12T09:27:38Z

packages/trees/frontend/sirius-components-trees/src/views/TreeView.tsx

-      />
-    </>
+    <div data-testid={treeId}>
+      {tree ? (


Do not use this way to express conditional rendering

Why does tree can be null when it is explicitly described as non null in its type?

sbegaudeau · 2024-09-12T09:29:57Z

packages/sirius-web/frontend/sirius-web-application/src/views/explorer/ExplorerView.tsx

@@ -136,11 +147,12 @@ export const ExplorerView = ({ editingContextId, readOnly }: WorkbenchViewCompon
          editingContextId={editingContextId}
          readOnly={readOnly}
          treeId={'explorer://'}
+          tree={tree}


Here you are giving a GQLTree | null to TreeView which expects a GQLTree. On the other hand, in ModelBrowserTree you are not rendering TreeView unless you have a GQLTree. So who is responsible for testing that tree is null?

Given the fact that both ModelBrowser is testing for null and the type of tree in TreeViewProps is GQLTree, I suspect that ExplorerView should do the job.

Bug: eclipse-sirius#3875 Signed-off-by: Jerome Gout <[email protected]> Signed-off-by: Florian ROUËNÉ <[email protected]> Signed-off-by: Stéphane Bégaudeau <[email protected]>

sbegaudeau

I'll wait for the build to merge it

frouene added pr: to review soon priority: low labels Aug 22, 2024

frouene added this to the 2024.9.0 milestone Aug 22, 2024

frouene linked an issue Aug 22, 2024 that may be closed by this pull request

Refactor the tree event classes related to the explorer #3875

Closed

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch 4 times, most recently from 6bbab40 to dd27f6e Compare August 23, 2024 08:13

jerome-obeo marked this pull request as ready for review August 23, 2024 08:14

jerome-obeo requested review from pcdavid, sbegaudeau and AxelRICHARD as code owners August 23, 2024 08:14

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch 4 times, most recently from aad3475 to 69116ec Compare August 26, 2024 10:30

sbegaudeau requested changes Aug 27, 2024

View reviewed changes

sbegaudeau mentioned this pull request Aug 27, 2024

[3763] Implements the Selection Dialog TreeView #3870

Merged

39 tasks

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch 2 times, most recently from 2d9a402 to 98578a3 Compare August 28, 2024 11:35

frouene self-requested a review August 28, 2024 14:05

frouene approved these changes Aug 28, 2024

View reviewed changes

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch from 98578a3 to 4acc6b5 Compare August 29, 2024 07:08

jerome-obeo requested a review from sbegaudeau August 29, 2024 07:09

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch 3 times, most recently from 966fbb0 to 71bf311 Compare August 30, 2024 09:11

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch from 71bf311 to 2c8e030 Compare September 10, 2024 13:07

jerome-obeo force-pushed the jgo/enh/refactor_explorer_event branch 3 times, most recently from 4320da1 to b9d5e52 Compare September 11, 2024 14:15

frouene force-pushed the jgo/enh/refactor_explorer_event branch 3 times, most recently from 90a5879 to 7afa45f Compare September 11, 2024 15:35

sbegaudeau requested changes Sep 12, 2024

View reviewed changes

sbegaudeau force-pushed the jgo/enh/refactor_explorer_event branch from eb85278 to 3b1d36e Compare September 12, 2024 09:44

[3875] Refactor the tree event classes related to the explorer

e6afb31

Bug: eclipse-sirius#3875 Signed-off-by: Jerome Gout <[email protected]> Signed-off-by: Florian ROUËNÉ <[email protected]> Signed-off-by: Stéphane Bégaudeau <[email protected]>

sbegaudeau force-pushed the jgo/enh/refactor_explorer_event branch from 3b1d36e to e6afb31 Compare September 12, 2024 09:45

sbegaudeau approved these changes Sep 12, 2024

View reviewed changes

sbegaudeau merged commit bb6894d into eclipse-sirius:master Sep 12, 2024
3 checks passed

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

jerome-obeo deleted the jgo/enh/refactor_explorer_event branch September 12, 2024 13:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3875] Refactor the tree event classes related to the explorer #3876

[3875] Refactor the tree event classes related to the explorer #3876

jerome-obeo commented Aug 22, 2024

sbegaudeau left a comment

sbegaudeau Aug 27, 2024

jerome-obeo Aug 27, 2024

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024 •

edited

Loading

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024

sbegaudeau Aug 27, 2024

jerome-obeo Aug 28, 2024

sbegaudeau commented Aug 27, 2024

jerome-obeo commented Aug 28, 2024

sbegaudeau left a comment

sbegaudeau Sep 12, 2024

sbegaudeau Sep 12, 2024

sbegaudeau Sep 12, 2024

sbegaudeau Sep 12, 2024

sbegaudeau Sep 12, 2024

sbegaudeau Sep 12, 2024

sbegaudeau left a comment

[3875] Refactor the tree event classes related to the explorer #3876

[3875] Refactor the tree event classes related to the explorer #3876

Conversation

jerome-obeo commented Aug 22, 2024

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?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerome-obeo Aug 28, 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

sbegaudeau commented Aug 27, 2024

jerome-obeo commented Aug 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbegaudeau left a comment

Choose a reason for hiding this comment

jerome-obeo Aug 28, 2024 •

edited

Loading