[4403] Add support of actions in table rows context menu #4405

jerome-obeo · 2025-01-13T09:00:35Z

Bug: #4403

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?

packages/tables/frontend/sirius-components-tables/src/rows/RowContextMenu.tsx

packages/tables/frontend/sirius-components-tables/src/rows/useRowContextMenuEntries.ts

packages/tables/frontend/sirius-components-tables/src/rows/useRowContextMenuEntries.types.ts

packages/tables/frontend/sirius-components-tables/src/rows/RowContextMenu.tsx

packages/tables/frontend/sirius-components-tables/src/rows/RowContextMenu.types.ts

frouene · 2025-01-14T08:03:00Z

packages/tables/frontend/sirius-components-tables/src/rows/RowContextMenu.types.ts

@@ -1,5 +1,5 @@
 /*******************************************************************************


Github indicates that this file is moved from ResizeRowHandler (that thus does not exist anymore), is it normal

ResizeRowHandler has been removed from the table/row folder since it has been copied into rows.
RowContextMenu.types.ts is a new file.

frouene · 2025-01-14T08:04:50Z

packages/tables/frontend/sirius-components-tables/src/table/row/ResizeRowHandler.tsx

@@ -1,73 +0,0 @@
-/*******************************************************************************


Why deleted

It was already copied in rows folder but not removed at this time.

frouene · 2025-01-14T08:06:50Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

@@ -126,9 +127,9 @@ export const TableContent = memo(
      onPaginationChange(pagination.cursor, pagination.direction, pagination.size);
    }, [pagination.cursor, pagination.size, pagination.direction]);

-    const handleRowHeightChange = (rowId, height) => {
+    function handleRowHeightChange(rowId, height) {


Why this change ?

This change is to avoid massive code changes.
This function is now passed as a parameter of useTableColumns located at the beginning of the component.
It would have been moved before it was passed to useTableColumns and bring with it other codes.

????

I don't understand why changing the signature of this function solves any issue

I found out by reverting this, you were using code hoisting to have access to this value before it was declared.

O______ô

The whole row-resizing feature is already in a dire need of a complete re-write, let's not add additional reason to this situation. Just move the code 20lines above instead of trying to access variables before they exist.

...bles/backend/sirius-components-collaborative-tables/src/main/resources/schema/table.graphqls

sbegaudeau · 2025-01-22T09:21:14Z

.../org/eclipse/sirius/web/papaya/representations/table/PackageTableRowContextMenuProvider.java

+
+    @Override
+    public List<RowContextMenuEntry> getRowContextMenuEntries(IEditingContext editingContext, TableDescription tableDescription, Table table, Line row) {
+        return List.of(new RowContextMenuEntry(DELETE_ID, DELETE_LABEL, List.of("/icons/full/obj16/DeleteTool.svg")));


Don't just reference images from other unrelated modules like that. The Papaya example table has no real links to the support for diagrams in the view DSL.

Changed, I added a dedicated icon for this action

sbegaudeau · 2025-01-22T10:25:37Z

...e/sirius/components/collaborative/tables/handlers/InvokeRowContextMenuEntryEventHandler.java

+        if (tableInput instanceof InvokeRowContextMenuEntryInput invokeRowContextMenuEntryInput) {
+            var optionalRow = this.tableQueryService.findLineById(tableContext.getTable(), invokeRowContextMenuEntryInput.rowId());
+            if (optionalRow.isPresent()) {
+                Line row = optionalRow.get();


We should really clarify this naming convention of line vs row one day.

It was originally named line but as the standard name is row we use it for new developments.
We did not refactor existing code because is not related to the current feature and we thought that a dedicated issue could be filed in future with the goal to align naming convention over all table code.

.../org/eclipse/sirius/components/collaborative/tables/handlers/RowContextMenuEventHandler.java

...orative-tables/src/main/resources/messages/sirius-components-collaborative-tables.properties

...bles/backend/sirius-components-collaborative-tables/src/main/resources/schema/table.graphqls

packages/tables/frontend/sirius-components-tables/src/rows/useRowContextMenuEntries.types.ts

sbegaudeau · 2025-01-22T10:43:50Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

@@ -126,9 +127,9 @@ export const TableContent = memo(
      onPaginationChange(pagination.cursor, pagination.direction, pagination.size);
    }, [pagination.cursor, pagination.size, pagination.direction]);

-    const handleRowHeightChange = (rowId, height) => {
+    function handleRowHeightChange(rowId, height) {


????

I don't understand why changing the signature of this function solves any issue

sbegaudeau · 2025-01-22T10:44:13Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

+      enableColumnPinning: true,
+      initialState: {
+        showGlobalFilter: enableGlobalFilter,
+        columnPinning: { left: ['mrt-row-header'], right: ['mrt-row-actions'] },


What's the link with row actions?

Row header and actions columns should be sticky

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

sbegaudeau · 2025-01-22T10:46:19Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

+          representationId={representationId}
+          tableId={table.id}
+          row={row}
+          table={MRT_table}


table prop has been removed

sbegaudeau · 2025-01-22T13:20:35Z

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryExecutor.java

+
+    private final IObjectSearchService objectSearchService;
+
+    private final Function<EObject, UUID> idProvider = (eObject) -> UUID.nameUUIDFromBytes(EcoreUtil.getURI(eObject).toString().getBytes());


Why does that has to be a field?

Comes from copy/paste.

And now it's just some dead code...

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryExecutor.java

...-tables/src/main/java/org/eclipse/sirius/components/tables/descriptions/LineDescription.java

sbegaudeau · 2025-01-22T13:29:15Z

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryProvider.java

+        if (precondition != null && !precondition.isBlank()) {
+            return this.evaluateBoolean(variableManager, interpreter, precondition);
+        }
+        return false;


Is it normal? Why return false by default instead of true?

You are right. Should be true

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryProvider.java

sbegaudeau · 2025-01-22T13:31:46Z

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryProvider.java

+
+    private boolean isValidActionPrecondition(org.eclipse.sirius.components.view.table.RowContextMenuEntry viewContextAction, VariableManager variableManager, AQLInterpreter interpreter) {
+        var precondition = viewContextAction.getPreconditionExpression();
+        if (precondition != null && !precondition.isBlank()) {


The fact that the expression may be null or blank is not your concern, remove those checks they are done within the interpreter already.

the expression evaluation has been inlined and this method removed

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryProvider.java

sbegaudeau

I will update the PR and merge it. As it stands today, it simply does not work in half of the use cases and crashes. I'll make sure it fails gracefully but it's really incomplete.

Tons of contributions have been submitted on tables without taking into account the whole picture on this subject. I shouldn't be the only one testing for real given that I have ensured that examples are available on Papaya.

sbegaudeau · 2025-01-27T16:35:12Z

...ipse/sirius/web/application/controllers/tables/PapayaTableRowControllerIntegrationTests.java

+                    "rowId", rowId.get().toString()
+            );
+            var result = this.rowContextMenuQueryRunner.run(variables);
+            Object document = Configuration.defaultConfiguration().jsonProvider().parse(result);


As I suspected it, it does nothing, I've removed it

sbegaudeau · 2025-01-27T16:36:20Z

...pse/sirius/web/application/controllers/tables/PapayaViewTableControllerIntegrationTests.java

+                    "tableId", tableId.get(),
+                    "rowId", rowId.get().toString());
+            var result = this.rowContextMenuQueryRunner.run(variables);
+            Object document = Configuration.defaultConfiguration().jsonProvider().parse(result);


Same comment

sbegaudeau · 2025-01-28T10:07:02Z

...ava/org/eclipse/sirius/components/collaborative/tables/api/IRowContextMenuEntryExecutor.java

+ *  @author Jerome Gout
+ */
+public interface IRowContextMenuEntryExecutor {
+    boolean canExecute(TableDescription tableDescription, String tableId, String rowId, String rowMenuContextEntryId);


It's a bit odd to provide only id's here and instance below

sbegaudeau · 2025-01-28T10:11:20Z

...rc/main/java/org/eclipse/sirius/components/collaborative/tables/dto/RowContextMenuEntry.java

+ *
+ * @author Jerome Gout
+ */
+public record RowContextMenuEntry(String id, String label, List<String> iconURLs) { }


require non null

sbegaudeau · 2025-01-28T10:12:01Z

...ava/org/eclipse/sirius/components/collaborative/tables/dto/RowContextMenuSuccessPayload.java

+ *
+ * @author Jerome Gout
+ */
+public record RowContextMenuSuccessPayload(UUID id, List<RowContextMenuEntry> entries) implements IPayload {


require non null

sbegaudeau · 2025-01-28T12:33:19Z

packages/tables/frontend/sirius-components-tables/src/rows/RowContextMenuContent.types.ts

+  viewer: GQLGetAllRowContextMenuEntriesViewer;
+}
+
+export interface GQLGetAllRowContextMenuEntriesViewer {


GQLViewer containing a GQLEditingContext containing a GQLRepresentationMetadata containing a GQLRepresentationDescription which can be a GQLTableDescription... The types are wrong and thus the code can crash

sbegaudeau · 2025-01-28T12:38:46Z

packages/tables/frontend/sirius-components-tables/src/table/useTableColumns.tsx

+    const rowHeaderColumn: MRT_ColumnDef<GQLLine, string> = {
+      id: 'mrt-row-header',
+      header: '',
+      size: 200,


Why 200? Why hardcode this?

sbegaudeau · 2025-01-28T12:40:52Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

@@ -126,9 +127,9 @@ export const TableContent = memo(
      onPaginationChange(pagination.cursor, pagination.direction, pagination.size);
    }, [pagination.cursor, pagination.size, pagination.direction]);

-    const handleRowHeightChange = (rowId, height) => {
+    function handleRowHeightChange(rowId, height) {


I found out by reverting this, you were using code hoisting to have access to this value before it was declared.

O______ô

sbegaudeau · 2025-01-28T12:42:08Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.tsx

@@ -126,9 +127,9 @@ export const TableContent = memo(
      onPaginationChange(pagination.cursor, pagination.direction, pagination.size);
    }, [pagination.cursor, pagination.size, pagination.direction]);

-    const handleRowHeightChange = (rowId, height) => {
+    function handleRowHeightChange(rowId, height) {


The whole row-resizing feature is already in a dire need of a complete re-write, let's not add additional reason to this situation. Just move the code 20lines above instead of trying to access variables before they exist.

sbegaudeau · 2025-01-28T12:44:46Z

.../main/java/org/eclipse/sirius/components/view/emf/table/ViewRowContextMenuEntryExecutor.java

+
+    private final IObjectSearchService objectSearchService;
+
+    private final Function<EObject, UUID> idProvider = (eObject) -> UUID.nameUUIDFromBytes(EcoreUtil.getURI(eObject).toString().getBytes());


And now it's just some dead code...

sbegaudeau · 2025-01-28T12:52:39Z

Most issues detected should be fixed now, I'll wait for the build to merge it.

Bug: eclipse-sirius#4403 Signed-off-by: Jerome Gout <[email protected]>

I'll dismiss this review to move forward and merge the PR. I've personally fixed most of the issues detected.

jerome-obeo requested a review from sbegaudeau as a code owner January 13, 2025 09:00

frouene added pr: to review soon priority: low labels Jan 13, 2025

frouene added this to the 2025.2.0 milestone Jan 13, 2025

frouene linked an issue Jan 13, 2025 that may be closed by this pull request

Add support of actions in table rows context menu #4403

Closed

1 task

frouene previously requested changes Jan 14, 2025

View reviewed changes

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch 5 times, most recently from 3aee130 to 34b2e24 Compare January 15, 2025 08:49

jerome-obeo requested a review from pcdavid as a code owner January 15, 2025 08:49

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch 3 times, most recently from bb65198 to b6f2a74 Compare January 15, 2025 12:19

frouene force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from b6f2a74 to da1a252 Compare January 15, 2025 14:59

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from da1a252 to 7d9aaac Compare January 22, 2025 09:26

sbegaudeau requested changes Jan 22, 2025

View reviewed changes

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from 7d9aaac to 89d48b9 Compare January 22, 2025 17:41

jerome-obeo requested a review from sbegaudeau January 22, 2025 17:42

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch 9 times, most recently from 1d28a58 to 6bb3e40 Compare January 28, 2025 08:43

jerome-obeo force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from 6bb3e40 to 7c1fa9f Compare January 28, 2025 09:19

sbegaudeau requested changes Jan 28, 2025

View reviewed changes

sbegaudeau force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from 7c1fa9f to 612a8d2 Compare January 28, 2025 12:52

sbegaudeau force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from 612a8d2 to b1dd897 Compare January 28, 2025 12:53

jerome-obeo added 2 commits January 28, 2025 13:57

[4403] Add support of actions in table rows context menu

4d292ab

Bug: eclipse-sirius#4403 Signed-off-by: Jerome Gout <[email protected]>

[4403] Add support of row actions in table view DSL

09ec261

Bug: eclipse-sirius#4403 Signed-off-by: Jerome Gout <[email protected]>

sbegaudeau force-pushed the jgo/enh/4403_add_actions_in_table_rows_context_menu branch from b1dd897 to 09ec261 Compare January 28, 2025 12:58

sbegaudeau approved these changes Jan 28, 2025

View reviewed changes

sbegaudeau merged commit a9ca3b9 into eclipse-sirius:master Jan 28, 2025
3 checks passed

jerome-obeo deleted the jgo/enh/4403_add_actions_in_table_rows_context_menu branch January 28, 2025 16:28

		@@ -1,5 +1,5 @@
		/*******************************************************************************

		@@ -1,73 +0,0 @@
		/*******************************************************************************


		private final IObjectSearchService objectSearchService;

		private final Function<EObject, UUID> idProvider = (eObject) -> UUID.nameUUIDFromBytes(EcoreUtil.getURI(eObject).toString().getBytes());

[4403] Add support of actions in table rows context menu #4405

[4403] Add support of actions in table rows context menu #4405

Conversation

jerome-obeo commented Jan 13, 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?

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

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

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 Jan 28, 2025