[3982] Add support for command palette #4011

gcoutable · 2024-09-17T16:25:19Z

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 · 2024-10-02T09:03:23Z

CHANGELOG.adoc

@@ -31,7 +31,8 @@ Both `IInput` and `IDomainEvent` implement `ICause` and will thus be used to ind
 - https://github.com/eclipse-sirius/sirius-web/issues/3972[#3972] [diagram] The `InsideLabelStyle#displayHeaderSeparator` has been renamed to `headerSeparatorDisplayMode` and is no longer a boolean but an enum with three possible values: NEVER, ALWAYS, and IF-CHILDREN.
 Previously, the value false was equivalent to NEVER, and true to IF-CHILDREN.
 The new option ALWAYS allows the separator to be displayed in every case.
- https://github.com/eclipse-sirius/sirius-web/issues/3678[#3678] [core] Remove `IRepresentationMetadataSearchService#findByRepresentation`, use `IRepresentationMetadataSearchService#findByRepresentationId` instead. 
+- https://github.com/eclipse-sirius/sirius-web/issues/3678[#3678] [core] Remove `IRepresentationMetadataSearchService#findByRepresentation`, use `IRepresentationMetadataSearchService#findByRepresentationId` instead.
+- https://github.com/eclipse-sirius/sirius-web/issues/3982[#3982] [sirius-web] The edit project view needs to declare the `OmniboxProvider` to benefit from the command palette.


How is that a breaking change?

sbegaudeau · 2024-10-02T09:04:09Z

packages/core/backend/sirius-components-collaborative/pom.xml

+		<dependency>
+			<groupId>com.graphql-java</groupId>
+			<artifactId>graphql-java</artifactId>
+		</dependency>


Why? The collaborative part is supposed to be independent from that unless I'm mistaken

sbegaudeau · 2024-10-02T09:06:42Z

...orative/src/main/java/org/eclipse/sirius/components/collaborative/dto/PageInfoWithCount.java

@@ -10,7 +10,7 @@
 * Contributors:
 *     Obeo - initial API and implementation
 *******************************************************************************/
-package org.eclipse.sirius.web.application.dto;
+package org.eclipse.sirius.components.collaborative.dto;


Why not in org.eclipse.sirius.components.core.graphql.dto? This does not seem to be used in sirius-components-collaborative and thus the dependency to graphql-java seems useless here.

Moved in org.eclipse.sirius.components.core.graphql.dto

sbegaudeau · 2024-10-02T09:09:07Z

...ava/org/eclipse/sirius/components/collaborative/handlers/GetOmniboxCommandsEventHandler.java

+                    var label = this.objectService.getLabel(object);
+                    var kind = this.objectService.getKind(object);
+                    var iconURL = this.objectService.getImagePath(object);
+                    omniboxCommands.add(new OmniboxCommand(UUID.fromString(id), label, kind, iconURL, ""));


The id of the object will not be an UUID in various use cases

Add a description indicating that the element will be selected something like "Click to select the object"

sbegaudeau · 2024-10-02T09:10:14Z

...ages/core/backend/sirius-components-collaborative/src/main/resources/schema/omnibox.graphqls

+}
+input OmniboxContextEntry {
+    id: ID!
+    label: String!


The label can be removed, it will be useful on the client, not the backend

sbegaudeau · 2024-10-02T09:25:23Z

packages/diagrams/frontend/sirius-components-diagrams/package.json

+    "picomatch": "4.0.2",
+    "mini-svg-data-uri": "1.4.4",


These shouldn't be necessary

I know, I will try again without

npm install worked without these (as it should) ...

sbegaudeau · 2024-10-02T09:25:56Z

packages/releng/backend/sirius-components-test-coverage/pom.xml

+		<dependency>
+			<groupId>org.eclipse.sirius</groupId>
+			<artifactId>sirius-components-core-graphql</artifactId>
+			<version>2024.9.1</version>


Don't forget to update it to 2024.9.3

sbegaudeau · 2024-10-02T09:26:05Z

packages/sirius-web/backend/sirius-web-starter/pom.xml

+		<dependency>
+			<groupId>org.eclipse.sirius</groupId>
+			<artifactId>sirius-components-core-graphql</artifactId>
+			<version>2024.9.1</version>


Don't forget to update it to 2024.9.3

sbegaudeau · 2024-10-02T09:27:12Z

packages/sirius-web/frontend/sirius-web/package.json

@@ -52,6 +53,7 @@
    "xstate": "4.32.1"
  },
  "devDependencies": {
+    "@rollup/plugin-image": "2.1.1",


This shouldn't be necessary for this PR

I will try again without, but maybe this change (or other dependency changes in this PR) should have been done in a previous PR.
What I mean is, sometimes, the team does not generate package-lock.json well, so the next dev who does it correctly, have to make the change that should have been done in a previous PR.

sbegaudeau · 2024-10-02T09:28:19Z

...ponents-forms/src/propertysections/__tests__/__snapshots__/ListPropertySection.test.tsx.snap

@@ -469,7 +469,7 @@ exports[`should render a readOnly list from widget properties 1`] = `
            class="MuiTableCell-root MuiTableCell-body MuiTableCell-sizeSmall css-18jhh0s-cell css-dsuxgy-MuiTableCell-root"
          >
            <p
-              class="MuiTypography-root MuiTypography-body1  css-qh6f2j-style css-1gqyz35-MuiTypography-root"
+              class="MuiTypography-root MuiTypography-body1 css-qh6f2j-style css-1gqyz35-MuiTypography-root"


Are those changes necessary? Similar question for the next file.

I had an error on test execution with the previous version (with two spaces).
It should not have been modified previously, why two spaces?
So, yes, those changes are necessary.

sbegaudeau

Given the state of what you have shown last week, I'll need the following user experience improvements to consider merging this PR:

The position of the palette should not change when results are received from the server
The size of the results parts should have a maximum size allowing the display of tens results max, after that it should offer a scrollbar
The results should be order by size for the moment (the backend should do it on the search results). The shortest results should come first. If I search for EditingContext I want to see EditingContext, IEditingContext and IEMFEditingContext before IEditingContextEventProcessor. That's a very naive way to rank results but it's better than nothing
Was the button in the header available? I don't remember seeing it
Update the current placeholder in the palette to indicate to type enter to execute the command
Remove the icon indicator that you have used. Instead make the enter button have a disable color when everything is synchronized and the action color when the users can press enter to have some results
Either make the name of the context in the command palette nice or remove it because the difference in size of the fonts does not look good

sbegaudeau · 2024-10-07T01:39:00Z

...ava/org/eclipse/sirius/components/collaborative/handlers/GetOmniboxCommandsEventHandler.java

+            var queryBasedObjectsInput = new QueryBasedObjectsInput(UUID.randomUUID(), "aql:editingContext.allContents()->select(e | e.name.contains('" + getOmniboxCommandsInput.query() + "'))", Map.of());
+            var objectsPayload = this.queryService.execute(editingContext, queryBasedObjectsInput);


Don't use the name use the label, countless of objects do not have a name. Just remove that and use:

var results = new ArrayList<Notifier>(); var iterator = editingDomain.getResourceSet().getAllContents(); while (iterator.hasNext()) { Notifier notifier = iterator.next(); var adapter = editingDomain.getAdapterFactory().adapt(notifier, IItemLabelProvider); if (adapter instanceof IItemLabelProvider itemLabelProvider && itemLabelProvider.getLabel(notifier).contains(query)) { results.add(notifier); } }

sbegaudeau · 2024-10-14T08:56:56Z

@gcoutable Unless I encounter some bug, I'll finish the review and ensure that it gets merged

sbegaudeau

I've not completed the review yet but with one or two minor improvements, it's starting to look really nice and useful

sbegaudeau · 2024-10-14T09:07:28Z

...rg/eclipse/sirius/components/core/graphql/datafetchers/ViewerOmniboxCommandsDataFetcher.java

+        var input = new GetOmniboxCommandsInput(UUID.randomUUID(), query);
+
+        if (!omniboxContextEntries.isEmpty()) {
+            var omniboxContextEntry = omniboxContextEntries.get(0);


We should consider the one with the editing context type but that's a detail for now.

sbegaudeau · 2024-10-14T09:08:50Z

packages/core/frontend/sirius-components-omnibox/package.json

+    "publish:local": "yalc push"
+  },
+  "peerDependencies": {
+    "@eclipse-sirius/sirius-components-core": "*",


All the peerDependencies of sirius-components-core should be here and all the peerDependencies from here should be in packages that depends on sirius-components-omnibox

sbegaudeau · 2024-10-14T09:12:01Z

packages/core/frontend/sirius-components-omnibox/src/Omnibox.tsx

+              },
+            }}
+          />
+        </FormControl>


It's better but not exactly what I had in mind, I'll see if I can update it quickly

sbegaudeau · 2024-10-14T09:12:34Z

packages/core/frontend/sirius-components-omnibox/src/OmniboxButton.tsx

+  },
+}));
+
+export const OmniboxButton = ({ size = 'small' }: OmniboxButtonProps) => {


The button is still not visible in the user interface, end users cannot discover this feature otherwise.

sbegaudeau · 2024-10-14T09:34:22Z

packages/core/frontend/sirius-components-omnibox/src/Omnibox.tsx

+              onClick={() => handleOnActionClick(action)}
+              onKeyDown={handleListItemKeyDown}>
+              <ListItemIcon>{action.icon}</ListItemIcon>
+              <ListItemText>{action.label}</ListItemText>


The text should be on one line without any wrap and with an ellipsis

Bug: #3982 Signed-off-by: Stéphane Bégaudeau <[email protected]> Signed-off-by: Guillaume Coutable <[email protected]>

Signed-off-by: Stéphane Bégaudeau <[email protected]>

gcoutable added this to the 2024.11.0 milestone Sep 17, 2024

gcoutable linked an issue Sep 17, 2024 that may be closed by this pull request

Add support for a command palette #3982

Closed

gcoutable added pr: to review later priority: low labels Sep 17, 2024

gcoutable force-pushed the gco/enh/add-command-palette branch 3 times, most recently from afb9817 to b7668b6 Compare September 24, 2024 09:45

gcoutable marked this pull request as ready for review September 24, 2024 09:45

gcoutable requested a review from sbegaudeau as a code owner September 24, 2024 09:45

gcoutable force-pushed the gco/enh/add-command-palette branch 2 times, most recently from b585571 to 6c226a3 Compare September 24, 2024 12:49

gcoutable requested a review from AxelRICHARD as a code owner September 24, 2024 12:49

gcoutable force-pushed the gco/enh/add-command-palette branch from 6c226a3 to aa444ef Compare September 24, 2024 13:38

sbegaudeau requested changes Oct 2, 2024

View reviewed changes

gcoutable force-pushed the gco/enh/add-command-palette branch 7 times, most recently from 801e64c to c2ddb0d Compare October 4, 2024 14:28

sbegaudeau requested changes Oct 7, 2024

View reviewed changes

sbegaudeau reviewed Oct 7, 2024

View reviewed changes

gcoutable force-pushed the gco/enh/add-command-palette branch 2 times, most recently from a7cad24 to b705110 Compare October 8, 2024 09:15

gcoutable requested a review from sbegaudeau October 9, 2024 09:18

gcoutable force-pushed the gco/enh/add-command-palette branch from b705110 to 20b51f1 Compare October 10, 2024 13:28

sbegaudeau self-assigned this Oct 14, 2024

sbegaudeau removed pr: to review later priority: low labels Oct 14, 2024

sbegaudeau added pr: to review soon priority: high labels Oct 14, 2024

gcoutable force-pushed the gco/enh/add-command-palette branch from 20b51f1 to 2407865 Compare October 14, 2024 08:52

sbegaudeau reviewed Oct 14, 2024

View reviewed changes

sbegaudeau force-pushed the gco/enh/add-command-palette branch from 2407865 to 1e18dbd Compare October 14, 2024 09:46

sbegaudeau requested a review from pcdavid as a code owner October 14, 2024 09:47

sbegaudeau force-pushed the gco/enh/add-command-palette branch from 2a75f85 to bed4b9e Compare October 14, 2024 09:56

gcoutable and others added 2 commits October 14, 2024 13:56

[3982] Add support for command palette

b84b89d

Bug: #3982 Signed-off-by: Stéphane Bégaudeau <[email protected]> Signed-off-by: Guillaume Coutable <[email protected]>

[fix] Fix invalid Spring versions in the tree DSL projects

322fd77

Signed-off-by: Stéphane Bégaudeau <[email protected]>

sbegaudeau force-pushed the gco/enh/add-command-palette branch from bed4b9e to 322fd77 Compare October 14, 2024 11:56

sbegaudeau approved these changes Oct 14, 2024

View reviewed changes

sbegaudeau merged commit 9e07dee into master Oct 14, 2024
4 checks passed

sbegaudeau deleted the gco/enh/add-command-palette branch October 14, 2024 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3982] Add support for command palette #4011

[3982] Add support for command palette #4011

gcoutable commented Sep 17, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 3, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 3, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

gcoutable Oct 2, 2024 •

edited

Loading

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024

sbegaudeau Oct 2, 2024

gcoutable Oct 2, 2024 •

edited

Loading

sbegaudeau left a comment

sbegaudeau Oct 7, 2024

sbegaudeau commented Oct 14, 2024

sbegaudeau left a comment

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

sbegaudeau Oct 14, 2024

		var queryBasedObjectsInput = new QueryBasedObjectsInput(UUID.randomUUID(), "aql:editingContext.allContents()->select(e \| e.name.contains('" + getOmniboxCommandsInput.query() + "'))", Map.of());
		var objectsPayload = this.queryService.execute(editingContext, queryBasedObjectsInput);

[3982] Add support for command palette #4011

[3982] Add support for command palette #4011

Conversation

gcoutable commented Sep 17, 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?

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

gcoutable Oct 2, 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

Choose a reason for hiding this comment

gcoutable Oct 2, 2024 • edited Loading

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

sbegaudeau commented Oct 14, 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

Choose a reason for hiding this comment

gcoutable Oct 2, 2024 •

edited

Loading

gcoutable Oct 2, 2024 •

edited

Loading