[cleanup] Improve identification of view representation descriptions #1876

mcharfadi · 2023-03-17T10:13:13Z

Bug : #1860

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 · 2023-03-29T14:36:30Z

CHANGELOG.adoc

@@ -11,6 +11,7 @@

 === Breaking changes

+- https://github.com/eclipse-sirius/sirius-components/issues/1860[1860] [view] Changed the type of descriptionId of Node and Edge from UUID to String


Please add your entries to the end of their section, not the beginning. On top of that, this change is not related to view but diagram and will thus impact every user of the diagram component.

sbegaudeau · 2023-03-29T14:36:53Z

.../eclipse/sirius/components/compatibility/emf/modeloperations/CreateViewOperationHandler.java

+            .filter(IDiagramContext.class::isInstance)
+            .map(IDiagramContext.class::cast)
+                .ifPresent(diagramContext -> diagramContext.getViewCreationRequests().add(viewCreationRequest));


Do not change the indentation of the existing code

sbegaudeau · 2023-03-29T14:37:04Z

...se/sirius/components/compatibility/emf/compatibility/diagrams/EdgeMappingConverterTests.java

                nodeMappingUUID, this.createNodeDescription(nodeMappingUUID),
                containerMappingUUID, this.createNodeDescription(containerMappingUUID)
-        );
+                );


Do not change the indentation of the existing code

sbegaudeau · 2023-03-29T14:37:12Z

...se/sirius/components/compatibility/emf/compatibility/diagrams/EdgeMappingConverterTests.java

                nodeMappingUUID, this.createNodeDescription(nodeMappingUUID),
                containerMappingUUID, this.createNodeDescription(containerMappingUUID)
-        );
+                );


Do not change the indentation of the existing code

sbegaudeau · 2023-03-29T14:37:16Z

...se/sirius/components/compatibility/emf/compatibility/diagrams/EdgeMappingConverterTests.java

                sourceContainerMappingUUID, this.createNodeDescription(sourceContainerMappingUUID),
                targetContainerMappingUUID, this.createNodeDescription(targetContainerMappingUUID)
-        );
+                );


Do not change the indentation of the existing code

sbegaudeau · 2023-03-29T15:43:13Z

...-web-services/src/main/java/org/eclipse/sirius/web/services/representations/ViewService.java

-    public ViewService(IDocumentRepository documentRepository, EPackage.Registry ePackageRegistry) {
+    private final IdDiagramProvider idProvider;
+
+    public ViewService(IdDiagramProvider idProvider, IDocumentRepository documentRepository, EPackage.Registry ePackageRegistry) {


Try to keep the coherence in the order of the parameters / fields

sbegaudeau · 2023-03-29T15:44:56Z

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

+ *
+ * @author mcharfadi
+ */
+public interface IIdDiagramProvider {


This should probably be named IDiagramIdProvider

sbegaudeau · 2023-03-29T15:51:42Z

...-web-services/src/main/java/org/eclipse/sirius/web/services/representations/ViewService.java

+        if (description instanceof DiagramDescription diagramDescription) {
+            return this.idProvider.getIdDiagramDescription(diagramDescription);
+        }
+


Here, we have something odd, the only goal of this method is to filter one representation description in a list of representation description to find the right one.

The real behavior that we want is instead to detect that the representation description has one of the new identifier and thus find the right one without having to look in all representation descriptions.

sbegaudeau · 2023-03-29T15:58:45Z

...-emf/src/test/java/org/eclipse/sirius/components/view/emf/diagram/IdDiagramProviderTest.java

+ *
+ * @author mcharfadi
+ */
+public class IdDiagramProviderTest {


You need to many mocks and small stuff to have an efficient test here. This will be very brittle. Add an assertion on a test of the converter for example.

sbegaudeau · 2023-03-30T07:18:05Z

...ain/java/org/eclipse/sirius/components/view/emf/diagram/ViewDiagramDescriptionConverter.java

-        // DiagramElementDescription should have a proper id.
-        return UUID.nameUUIDFromBytes(EcoreUtil.getURI(diagramElementDescription).toString().getBytes()).toString();
-    };
+    private final Function<org.eclipse.sirius.components.view.DiagramElementDescription, String> idDiagramElementProvider;


I'm not sure that you need to keep that and the diagram id provider above. Keep one only

…tring Signed-off-by: Michaël Charfadi <[email protected]>

…iption elements Signed-off-by: Michaël Charfadi <[email protected]>

…and Edge Signed-off-by: Michaël Charfadi <[email protected]>

mcharfadi force-pushed the mch/enh/ids-description branch from 4d92431 to aaa67c9 Compare March 17, 2023 10:14

mcharfadi changed the title ~~[1860] https://github.com/eclipse-sirius/sirius-components/issues/1860~~ [1860] The palette of reused node descriptions from another diagram description is always empty Mar 17, 2023

mcharfadi force-pushed the mch/enh/ids-description branch 5 times, most recently from 5b9436c to 24e9afc Compare March 20, 2023 14:40

sbegaudeau added this to the 2023.4.0 milestone Mar 21, 2023

sbegaudeau added pr: to review soon priority: low labels Mar 21, 2023

mcharfadi force-pushed the mch/enh/ids-description branch 8 times, most recently from 4ac8a83 to 2b215f2 Compare March 23, 2023 11:59

sbegaudeau self-assigned this Mar 27, 2023

sbegaudeau requested changes Mar 30, 2023

View reviewed changes

sbegaudeau marked this pull request as ready for review March 30, 2023 07:48

sbegaudeau requested review from pcdavid and gcoutable as code owners March 30, 2023 07:48

sbegaudeau force-pushed the mch/enh/ids-description branch from cb3e909 to 41951eb Compare March 30, 2023 12:31

[cleanup] Change the type of {Node|Edge}#descriptionId from UUID to S…

0ea8d6e

…tring Signed-off-by: Michaël Charfadi <[email protected]>

sbegaudeau force-pushed the mch/enh/ids-description branch from 41951eb to 2383435 Compare March 30, 2023 12:31

mcharfadi added 2 commits March 30, 2023 14:39

[cleanup] Use an URL instead of an UUID for the id of some view descr…

d171a23

…iption elements Signed-off-by: Michaël Charfadi <[email protected]>

[cleanup] Add a common interface IDiagramElement implemented by Node …

b5a32d0

…and Edge Signed-off-by: Michaël Charfadi <[email protected]>

sbegaudeau force-pushed the mch/enh/ids-description branch from 2383435 to b5a32d0 Compare March 30, 2023 12:39

sbegaudeau approved these changes Mar 30, 2023

View reviewed changes

sbegaudeau changed the title ~~[1860] The palette of reused node descriptions from another diagram description is always empty~~ [cleanup] Improve identification of view representation descriptions Mar 30, 2023

sbegaudeau merged commit 2211cf2 into eclipse-sirius:master Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[cleanup] Improve identification of view representation descriptions #1876

[cleanup] Improve identification of view representation descriptions #1876

mcharfadi commented Mar 17, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 29, 2023

sbegaudeau Mar 30, 2023

		@@ -11,6 +11,7 @@

		=== Breaking changes

		- https://github.com/eclipse-sirius/sirius-components/issues/1860[1860] [view] Changed the type of descriptionId of Node and Edge from UUID to String

[cleanup] Improve identification of view representation descriptions #1876

[cleanup] Improve identification of view representation descriptions #1876

Conversation

mcharfadi commented Mar 17, 2023

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