[4556] Add the support of row hierarchy in tables #4565

jerome-obeo · 2025-02-13T15:14:27Z

Bug: #4556

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-03-04T10:20:19Z

CHANGELOG.adoc

@@ -267,6 +267,7 @@ To achieve that a new concept, `ProjectSemanticData` has been added along with a
 - https://github.com/eclipse-sirius/sirius-web/issues/4411[#4411] [sirius-web] Display a message in the query view if the expression does not return anything or if the result type is unknown
 - https://github.com/eclipse-sirius/sirius-web/issues/4411[#4411] [sirius-web] Add support for expressions returning a collection of strings in the query view

+- https://github.com/eclipse-sirius/sirius-web/issues/4556[#4556] [table] Add the support of row hierarchy in tables


To be moved in 2025.4.0

sbegaudeau · 2025-03-04T10:20:38Z

...rius/components/formdescriptioneditors/widget/table/TableWidgetPreviewConverterProvider.java

-                        .helpTextProvider(vm -> "")
-                        .tableDescription(tableDescription)
-                        .build();
+                return TableWidgetPreviewConverterProvider.this.getTableWidgetDescription(viewTableDescription, id);


Why the prefix TableWidgetPreviewConverterProvider.this?

Because we are calling a method of the class TableWidgetPreviewConverterProvider within an anonymous class (extending TableWidgetSwitch). I don't see how I can avoid that.

sbegaudeau · 2025-03-04T10:22:27Z

...ackend/sirius-components-tables/src/main/java/org/eclipse/sirius/components/tables/Line.java

    private Line() {
        // Prevent instantiation
    }

+    public static Builder newLine(UUID id) {


I don't understand why you are changing the order of the methods here.

I guess that this comes from my formatter triggered on “save”.
I didn't do it on purpose.

sbegaudeau · 2025-03-04T10:22:43Z

...ckend/sirius-components-tables/src/main/java/org/eclipse/sirius/components/tables/Table.java

    private Table() {
        // Prevent instantiation
    }

+    public static Builder newTable(String id) {


And again...

sbegaudeau · 2025-03-04T10:24:48Z

...tables/src/main/java/org/eclipse/sirius/components/tables/descriptions/TableDescription.java

    private TableDescription() {
        // Prevent instantiation
    }

+    public static Builder newTableDescription(String id) {


And again...

sbegaudeau · 2025-03-04T10:34:38Z

packages/tables/frontend/sirius-components-tables/src/table/TableContent.types.ts

+export interface TreeRow extends GQLLine {
+  children: TreeRow[];
+}


Why is a datastructure from the frontend extending the datastructure from the backend like that, this feels odd.

sbegaudeau · 2025-03-04T10:35:46Z

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

@@ -167,7 +243,7 @@ export const TableContent = memo(
      enableGlobalFilter,
      manualFiltering: true,
      onGlobalFilterChange: setGlobalFilter,
-      enableColumnPinning: false,
+      enableColumnPinning: true,


To fix a bug on sticky column. This change avoid having overlapping lines when horizontal scroll:

sbegaudeau · 2025-03-04T10:42:16Z

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

@@ -92,7 +163,7 @@ export const TableContent = memo(
      onColumnFiltersChange,
      enableColumnFilters
    );
-    const [linesState, setLinesState] = useState<GQLLine[]>(table.lines);
+    const [linesState, setLinesState] = useState<GQLLine[]>([]);


This state should ultimately be removed, I has been added for the wrong reasons (i.e. because the resizing of the rows is messing up the datastructure received from the backend which should be considered as immutable), the goal is not in any way to add more state and more behavior in the frontend.

The only change acceptable for this state is its removal.

As it is written in the ADR (last paragraph), we are only implementing this feature on the front end side.
This means that the front end handles the tree logic of hiding and revealing lines according to user interactions.
This has to be handled as a React state in the TableContent component.

We went trough this multiple times in the past over multiple features to ensure that they would work server-side to not let the frontend make more hypothesis than necessary or to have more state than necessary, we even had a version of this discussion recently on row filters for example.

Philosophically this can not work the sirius-way by being implemented client side. It's even more complex client side than server-side. Half of the features on this table representation are related to hiding stuff and time and time again I have explained that this should be done server-side. I've had variants of this discussion on:

pagination

column visibility

column filters

global search

row filters

and even in other representations like collapsing tree items, hiding diagram nodes, collapsing diagram nodes, deleting unsynchronized nodes, hiding form widgets and I'm sure that I'm forgetting some other occurrences of "how should we let users hide stuff" in Sirius Web.

I don't understand how this approach could be considered as a viable option now. How is collapsing/expanding rows different than any other methods we have to hide some rows?

As it is written in the ADR

I was curious about that so I went back to check, the first version of the ADR that I've reviewed was quite low on details regarding this very specific part but it mentioned that there should be changes in the subscription. Since this is how it would be necessary to handle this feature (just like it is done for tons of other related features in table for example), I was in agreement with this part even if I understand now that we did not understood the same thing from this line. When the ADR was updated, since I was lead to believe that the changes where done following my comments and since I had some verbal exchanges with others on how this feature should be implemented, I somehow missed this complete reversal of philosophy compared to everything that has been done up until now in this table representation.

I was too trustful and did not imagine that such a change could slipped in after a first review while just taking into accounts my comments (as I imagined it). I'm sorry, I will do better and be more pessimistic during my future code reviews.

sbegaudeau · 2025-03-04T10:43:55Z

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

@@ -36,6 +36,64 @@ import { GQLLine, TableContentProps, TablePaginationState } from './TableContent
 import { useGlobalFilter } from './useGlobalFilter';
 import { useTableColumns } from './useTableColumns';

+const keepRow = (row: GQLLine, visibleRows: GQLLine[]): number => {


I haven't read in details the changes in this file but from what I understand by reading the function signatures, you seem to be doing a lot of calculation and thus hypothesis on the frontend. The backend should be in charge of the computation so I don't understand what is being done here. Can you detail the lifecycle of this feature? Especially with regards to the pagination.

This is all the tree logic done in the front end.
Backend returns a list of lines according to semantic candidates, pagination, filters then the front handle the depthLevel and the state (collapsed or expanded) of each row to display those lines as a tree.

Lines are computed on back end side

sbegaudeau · 2025-03-04T10:44:39Z

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

+          <RowHeader row={row.original}>
+            {table.enableSubRows && (
+              <RowChevronButton
+                row={row.original}
+                isExpanded={expandedRowIds.includes(row.original.id)}
+                onExpandCollapse={onExpandCollapse}
+                hasChildren={hasChildren(row.original.id, table.lines)}
+              />
+            )}
+          </RowHeader>


Just make this button part of the row header, the table columns hook doesn't have to be impacted that much by this concern.

Bug: #4556 Signed-off-by: Jerome Gout <[email protected]>

jerome-obeo added type: enhancement package: table labels Feb 13, 2025

jerome-obeo added this to the 2025.2.0 milestone Feb 13, 2025

jerome-obeo requested review from pcdavid and sbegaudeau as code owners February 13, 2025 15:14

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from 1761c0c to f7e759f Compare February 13, 2025 15:17

jerome-obeo added priority: high pr: to review soon and removed priority: high labels Feb 13, 2025

jerome-obeo marked this pull request as draft February 13, 2025 16:40

sbegaudeau modified the milestones: 2025.2.0, 2025.4.0 Feb 14, 2025

sbegaudeau linked an issue Feb 14, 2025 that may be closed by this pull request

Add the support of row hierarchy in tables #4556

Open

1 task

sbegaudeau added priority: low pr: to review later and removed package: table type: enhancement pr: to review soon labels Feb 14, 2025

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from f7e759f to f1d870d Compare February 14, 2025 09:36

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from f1d870d to dbc5a1d Compare February 25, 2025 10:18

jerome-obeo marked this pull request as ready for review February 25, 2025 10:19

jerome-obeo requested a review from AxelRICHARD as a code owner February 25, 2025 10:19

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch 8 times, most recently from 8fe2a42 to 9f7401a Compare February 25, 2025 14:02

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch 5 times, most recently from dbb1315 to 4d0bae1 Compare February 26, 2025 08:39

jerome-obeo added pr: to review soon priority: high and removed pr: to review later priority: low labels Feb 26, 2025

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from 4d0bae1 to 7094872 Compare March 3, 2025 08:39

sbegaudeau requested changes Mar 4, 2025

View reviewed changes

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from 7094872 to 943d471 Compare March 5, 2025 07:56

jerome-obeo requested a review from sbegaudeau March 5, 2025 07:57

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch 6 times, most recently from 92e8315 to 65cf01d Compare March 6, 2025 16:45

[4556] Add the support of row hierarchy in tables

4926c42

Bug: #4556 Signed-off-by: Jerome Gout <[email protected]>

jerome-obeo force-pushed the jgo/enh/add_support_of_row_hierarchy_in_tables branch from 65cf01d to 4926c42 Compare March 6, 2025 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[4556] Add the support of row hierarchy in tables #4565

[4556] Add the support of row hierarchy in tables #4565

jerome-obeo commented Feb 13, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

sbegaudeau Mar 5, 2025

jerome-obeo Mar 6, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 4, 2025

jerome-obeo Mar 6, 2025

sbegaudeau Mar 4, 2025

jerome-obeo Mar 5, 2025

[4556] Add the support of row hierarchy in tables #4565

Are you sure you want to change the base?

[4556] Add the support of row hierarchy in tables #4565

Conversation

jerome-obeo commented Feb 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