Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix rending of quickpick buttons #13342

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

tortmayr
Copy link
Contributor

@tortmayr tortmayr commented Feb 1, 2024

What it does

Ensure that the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item.

More context:
The buttons property is assigned in the QuickOpenMainImpl. This works perfectly fine when creating a InputBox via the quickopen service but for the normal QuickPick
this assignment gets lost because the actual monaco quickpick item is wrapped and the buttons property assignment is not forwarded.

Fixes #13076

Contributed on behalf of STMicroelectronics

How to test

Follow the steps to reproduced described in #13076 (comment)
(vsix for testing is provided).
After installling the plugins., execute the Hello World command. Notice that buttons are now properly rendered (while not being rendered at all on master)

Follow-ups

Review checklist

Reminder for reviewers

@@ -580,6 +580,14 @@ class MonacoQuickPick<T extends QuickPickItem> extends MonacoQuickInput implemen
return this.wrapped.items.map(item => QuickPickSeparator.is(item) ? item : item.item);
}

get buttons(): ReadonlyArray<IQuickInputButton> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typing here is not right: there is no "buttons" property on QuickPick. Returning monaco-specific types here is not appropriate. QuickPick is a wrapper around IQuickPick.

Copy link
Contributor Author

@tortmayr tortmayr Feb 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So how should we handle this?
The current implementation in the quick-open-main does not do any type checks at all. It simply iterates through the "params" (of type TransferQuickPick) list and assigns them as property of the input element.

So basically we would have to make sure that all properties of the TransferQuickPick are present on the wrapper and properly forwared to the underyling monaco specific element.
Note: Ignore that statement, seems like I have misinterpreted the code everything should work as expected here.

On a sidenote: There is also a wrapper for MonacoQuickInput which seems currently unused. Thats why the buttons works when using QuickInput` because in that case we use the monaco type directly.
We should probably also wrap that correctly ore remove this class altogether

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add the buttons types readonly QuickInputButton[] to the QuickPick interface and fix the typing by appropriately casting/converting.
Which wraper is unused? Can you explain, please?

Copy link
Contributor Author

@tortmayr tortmayr Mar 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmaeder

Unfortunatley the QuickInputButton and Monacos IQuickInputButton interface are not compatible.
They use different URI types for the iconPath:

  • QuickInputButton: import from 'vscode-uri';
  • IQuickInputButton: import from '@theia/monaco-editor/lib/vs/base/common/uri`

They difference is the UriComponent definition which has partial optional properties in the monaco variant

export interface UriComponents {
	scheme: string;
	authority?: string;
	path?: string;
	query?: string;
	fragment?: string;
}

but has no optional properties in the vscode-uri variant:

export interface UriComponents {
    scheme: string;
    authority: string;
    path: string;
    query: string;
    fragment: string;
}

Any ideas how to fix this without leaking monaco-internal types into the QuickInputButton definition?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you just cast the relevant structures? The values are not changed by the cast anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏼 done

@JonasHelming
Copy link
Contributor

@tortmayr

Ensure that  the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item.

Fixes eclipse-theia#13076

Contributed on behalf of STMicroelectronics
@tortmayr tortmayr force-pushed the tortmayr/13076 branch 3 times, most recently from 958e541 to 96f5955 Compare March 8, 2024 13:10
@tortmayr tortmayr requested a review from tsmaeder March 8, 2024 14:04
@tortmayr tortmayr merged commit de1c9a9 into eclipse-theia:master Mar 13, 2024
14 checks passed
jonah-iden pushed a commit that referenced this pull request Mar 15, 2024
Ensure that  the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item.

Fixes #13076

Contributed on behalf of STMicroelectronics
jonah-iden added a commit that referenced this pull request Mar 22, 2024
* basics for dev-container support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic creating and connecting to container working

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* open workspace when opening container

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* save and reuse last USed container per workspace

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* restart container if running

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* better container creation extension features

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added dockerfile support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* rebuild container if devcontainer.json has been changed since last use

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fix build

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed checking if container needs rebuild

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* working port forwarding via exec instance

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* review changes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fix import

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* smaller fixes and added support for multiple devcontainer configuration files

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic output window for devcontainer build

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* smaller review changes and nicer dockerfile.json detection code

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed build and docuemented implemented devcontainer.json properties

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* Fix unneeded URI conversion (#13415)

* Fix quickpick problems found in IDE testing (#13451)

Fixes #13450, #13449

contributed on behalf of STMicroelectronics

Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>

* Fix rending of quickpick buttons (#13342)

Ensure that  the Theia specific wrapper for the MonacoQuickPickItem properly forwards assignments of the "buttons" property to the wrapped item.

Fixes #13076

Contributed on behalf of STMicroelectronics

* electron: allow accessing the metrics endpoint for performance analysis (#13380)

By default, when running Theia in Electron, all endpoints are protected
by the ElectronTokenValidator.
This patch allows accessing the '/metrics' endpoint without a token,
which enables us to collect metrics for performance analysis.

For this, ElectronTokenValidator is extended to allow access to the
metrics endpoint. All other endpoints are still protected.

Contributed on behalf of STMicroelectronics

Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>

* fixed renaming and moving of open notebooks (#13467)

* fixed renameing of open notebooks

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed moving of notebook editors to other areas

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

---------

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* [playwright] Update documentation

Since a recent enhancement/refactoring of @theia/playwright, to permit using it in Theia
Electron applications, the way to load an application has changed. This commit is an
attempt to update the examples that are part of the documentation. I validated the changes
in the "theia-playwright-template" repository, and so I have adapted the sample code to
that repo's linting rules (using single quotes instead of double).

It's possible that other things have changed, that I have not yet encountered, but this
should be a good step forward, at least for those just getting started integrating
playwright to test their Theia-based app.

Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>

* basics for dev-container support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic creating and connecting to container working

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added dockerfile support

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* added port forwarding inlcuding ui

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* basic port/address validation

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed allready forwarded port checking

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* rebase  fixes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* removed unused file

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* review changes

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* fixed widget focus and message margin

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

* default port binding now shows as 0.0.0.0

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>

---------

Signed-off-by: Jonah Iden <jonah.iden@typefox.io>
Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
Signed-off-by: Olaf Lessenich <olessenich@eclipsesource.com>
Signed-off-by: Marc Dumais <marc.dumais@ericsson.com>
Co-authored-by: Alexander Taran <Alexander-Taran@users.noreply.github.com>
Co-authored-by: Thomas Mäder <tsmaeder@users.noreply.github.com>
Co-authored-by: Tobias Ortmayr <tortmayr@eclipsesource.com>
Co-authored-by: Olaf Lessenich <olessenich@eclipsesource.com>
Co-authored-by: Marc Dumais <marc.dumais@ericsson.com>
@jfaltermeier jfaltermeier added this to the 1.48.0 milestone Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

QuickPick buttons do not render
4 participants