[5651] Add support for undo redo on label appearance and label layout data by mcharfadi · Pull Request #5652 · eclipse-sirius/sirius-web

mcharfadi · 2025-10-08T15:32:31Z

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 · 2025-10-23T09:37:24Z

packages/diagrams/frontend/sirius-components-diagrams/src/renderer/move/useLabelMove.ts

+const addUndoForLayout = (mutationId: string) => {
+  var storedUndoStack = sessionStorage.getItem('undoStack');
+  var storedRedoStack = sessionStorage.getItem('redoStack');
+
+  if (storedUndoStack && storedRedoStack) {
+    var undoStack: String[] = JSON.parse(storedUndoStack);
+    if (!undoStack.find((id) => id === mutationId)) {
+      sessionStorage.setItem('undoStack', JSON.stringify([mutationId, ...undoStack]));
+    }
+  }
+};


How many time would this function be copied and paster in the frontend?

This is moved in the last PR, I wanted to have most recorders implemented before moving this function directly inside synchronizeLayoutData in order to capture all graphical changes

sbegaudeau · 2025-10-23T09:37:58Z

...c/main/java/org/eclipse/sirius/components/collaborative/diagrams/DiagramCreationService.java


+        List<DiagramLabelLayoutEvent> diagramLabelLayoutEvents = diagramEvents.stream()
+                .filter(DiagramLabelLayoutEvent.class::isInstance)
+                .map(DiagramLabelLayoutEvent.class::cast).toList();


.toList() should be on the next line

sbegaudeau · 2025-10-23T09:38:53Z

...org/eclipse/sirius/web/application/undo/services/api/ILabelAppearanceChangeUndoRecorder.java

+    List<IAppearanceChange> computeUndoNodeLabelAppearanceChanges(Node previousNode, String labelId, Optional<ILabelAppearanceChange> change);
+
+    List<IAppearanceChange> computeUndoEdgeLabelAppearanceChanges(Edge previousEdge, String labelId, Optional<ILabelAppearanceChange> change);


Two entry points? That's odd, why not one or two interfaces which would call a same service to share some common code.

sbegaudeau · 2025-10-23T09:40:20Z

...a/org/eclipse/sirius/web/application/undo/services/recorder/DiagramLayoutChangeRecorder.java

+    private boolean isSameLabelLayoutData(LabelLayoutData previousLayout, LabelLayoutDataInput updatedLayout) {
+        return isSamePosition(previousLayout.position(), updatedLayout.position());
+    }


This method is useless

sbegaudeau · 2025-10-23T09:41:15Z

...eclipse/sirius/web/application/undo/services/recorder/LabelAppearanceChangeUndoRecorder.java

+    public List<IAppearanceChange> computeUndoNodeLabelAppearanceChanges(Node previousNode, String labelId, Optional<ILabelAppearanceChange> optionalChange) {
+        List<IAppearanceChange> appearanceChanges = new ArrayList<>();
+        if (optionalChange.isPresent()) {
+            var change = optionalChange.get();
+            getNodeOutsideLabel(previousNode, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getAppearanceChanges(change, previousLabel.customizedStyleProperties(), previousLabel.style())));
+            getNodeInsideLabel(previousNode, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getAppearanceChanges(change, previousLabel.getCustomizedStyleProperties(), previousLabel.getStyle())));
+        } else {
+            getNodeOutsideLabel(previousNode, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getResetLabelApperanceChange(labelId, previousLabel.customizedStyleProperties(), previousLabel.style())));
+            getNodeInsideLabel(previousNode, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getResetLabelApperanceChange(labelId, previousLabel.getCustomizedStyleProperties(), previousLabel.getStyle())));
+        }
+        return appearanceChanges;


Use `this.``

This should be a node specific service

sbegaudeau · 2025-10-23T09:41:39Z

...eclipse/sirius/web/application/undo/services/recorder/LabelAppearanceChangeUndoRecorder.java

+    public List<IAppearanceChange> computeUndoEdgeLabelAppearanceChanges(Edge previousEdge, String labelId, Optional<ILabelAppearanceChange> optionalChange) {
+        List<IAppearanceChange> appearanceChanges = new ArrayList<>();
+        if (optionalChange.isPresent()) {
+            var change = optionalChange.get();
+            getEdgeLabel(previousEdge, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getAppearanceChanges(change, previousLabel.customizedStyleProperties(), previousLabel.style())));
+        } else {
+            getEdgeLabel(previousEdge, labelId)
+                    .ifPresent(previousLabel -> appearanceChanges.addAll(getResetLabelApperanceChange(labelId, previousLabel.customizedStyleProperties(), previousLabel.style())));
+        }
+        return appearanceChanges;
+    }


Use this.

This should be an edge specific service

sbegaudeau · 2025-10-23T09:41:58Z

...eclipse/sirius/web/application/undo/services/recorder/LabelAppearanceChangeUndoRecorder.java

+        return appearanceChanges;
+    }
+
+    private List<IAppearanceChange> getAppearanceChanges(ILabelAppearanceChange change, Set<String> previousCustomizedStyleProperties, LabelStyle previousLabelStyle) {


This should be a common service reused by the other two

sbegaudeau

I've fixed some minor issue in the PR (indicated in the comments) and I'll use it to publish 2025.12.5 as RC1. Nevertheless I think that there may be some small issues in some part because some kind of label appearance changes were not taken into account but we can see this later for RC2.

I'll wait for the build to merge it

sbegaudeau · 2026-01-16T09:42:17Z

...pse/sirius/web/application/undo/services/recorder/EdgeLabelAppearanceChangeUndoRecorder.java

+        if (diagramElement instanceof Edge previousEdge) {
+            if (optionalChange.isPresent()) {
+                var change = optionalChange.get();
+                getEdgeLabel(previousEdge, labelId)


Missing this.

sbegaudeau · 2026-01-16T09:44:30Z

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

+        var outsideLabel = new OutsideLabelDescriptionBuilder()
+                .labelExpression("aql:self.name")
+                .style(DiagramFactory.eINSTANCE.createOutsideLabelStyle())
+                .build();


Don't reuse existing test diagram description. It creates coupling between independent tests

sbegaudeau

I've made a small mistake in the PR, I'll retrigger the build

… data Bug: #5651 Signed-off-by: Michaël Charfadi <michael.charfadi@obeosoft.com>

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

mcharfadi added this to the 2025.12.0 milestone Oct 8, 2025

mcharfadi added pr: to review soon priority: high labels Oct 8, 2025

mcharfadi linked an issue Oct 8, 2025 that may be closed by this pull request

Add support for undo redo on label appearance and label layout data #5651

Closed

mcharfadi force-pushed the mch/enh/undo-redo-label branch 14 times, most recently from 866a326 to 0898403 Compare October 10, 2025 08:01

mcharfadi marked this pull request as ready for review October 10, 2025 09:01

mcharfadi assigned sbegaudeau Oct 14, 2025

sbegaudeau requested changes Oct 23, 2025

View reviewed changes

mcharfadi force-pushed the mch/enh/undo-redo-label branch from 0898403 to 9842c16 Compare October 24, 2025 09:07

mcharfadi modified the milestones: 2025.12.0, 2026.1.0 Nov 28, 2025

mcharfadi force-pushed the mch/enh/undo-redo-label branch 6 times, most recently from dde9df9 to 748f2ec Compare January 14, 2026 15:58

mcharfadi force-pushed the mch/enh/undo-redo-label branch 3 times, most recently from a2ec81e to e175c5e Compare January 14, 2026 16:35

sbegaudeau force-pushed the mch/enh/undo-redo-label branch from e175c5e to ef41420 Compare January 16, 2026 10:03

sbegaudeau reviewed Jan 16, 2026

View reviewed changes

sbegaudeau force-pushed the mch/enh/undo-redo-label branch 2 times, most recently from 0dcf873 to b1d1537 Compare January 16, 2026 10:20

sbegaudeau requested changes Jan 16, 2026

View reviewed changes

mcharfadi and others added 2 commits January 16, 2026 13:45

[5651] Add support for undo redo on label appearance and label layout…

c496d57

… data Bug: #5651 Signed-off-by: Michaël Charfadi <michael.charfadi@obeosoft.com>

[releng] Bump version to 2025.12.5

79d2fe6

Signed-off-by: Stéphane Bégaudeau <stephane.begaudeau@obeo.fr>

sbegaudeau force-pushed the mch/enh/undo-redo-label branch from b1d1537 to 79d2fe6 Compare January 16, 2026 12:46

sbegaudeau approved these changes Jan 16, 2026

View reviewed changes

sbegaudeau merged commit b7ebc9b into master Jan 16, 2026
4 checks passed

sbegaudeau deleted the mch/enh/undo-redo-label branch January 16, 2026 13:39

		List<IAppearanceChange> computeUndoNodeLabelAppearanceChanges(Node previousNode, String labelId, Optional<ILabelAppearanceChange> change);

		List<IAppearanceChange> computeUndoEdgeLabelAppearanceChanges(Edge previousEdge, String labelId, Optional<ILabelAppearanceChange> change);

Conversation

mcharfadi commented Oct 8, 2025

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbegaudeau left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbegaudeau left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants