[vscode] Support ViewBadge and tree/webview view badge property #12330

rschnekenbu · 2023-03-21T16:06:58Z

What it does

Adds support for ViewBadge and badge for tree views and Webview views

Fixes #12020

Contributed on behalf of STMicroelectronics

How to test

This PR adds a badge to Tree based views and webviews contributed by extensions. Thus 2 extensions based on vscode samples are to be tested with this patch.

custom-view-samples-0.0.2.zip This one provides a tree view with 2 actions (change badge value and change tooltip value). The first one will ask for a new value, the second for a new tooltip.

calico-colors-0.0.13.zip This one slightly updates the webview view vscode extensions sample, to change the tooltip when the add color is triggered, and to remove the tooltip (set to undefined) when the clear colors is triggered.

To test, install the extensions and trigger the action:

'Test View: change badge/tooltip value' to change the badge or its tooltip on the Test View
'Calico Colors: Add color' to add a tooltip to the calico colors test view. The first color does not add a badge, it only shows after the second one.
'Calico Colors: Clear color' to set the badge to undefined, and as indicated by the doc of ViewBadge, the badge is deleted.

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

tsmaeder

First reading of the code.

packages/core/src/browser/view-container.ts

tsmaeder · 2023-03-23T09:11:46Z

packages/plugin-ext/src/plugin/tree/tree-views.ts

+    }
+    set badge(badge: ViewBadge | undefined) {
+        this._badge = badge;
+        this.proxy.$setBadge(this.treeViewId, this._badge);


This is a potential problem: the badge object is created by the plugin. It could contain arbitrary structures, which might no be serializable via our rpc algorithm (this has happened before 🤷) We need to copy the values like

this.proxy.$setBadge(this.treeViewId, {value: badge.value, tooltip: badge.tooltip});

This is adressed in the new version, with a slight change due to badge being potentially undefined:
this.proxy.$setBadge(this.treeViewId, badge ? { value: badge.value, tooltip: badge.tooltip } : undefined);

There may be a shorter syntax I am not aware of.

tsmaeder · 2023-03-23T09:22:43Z

packages/plugin-ext/src/main/browser/view/plugin-view-widget.ts

@@ -138,6 +150,38 @@ export class PluginViewWidget extends Panel implements StatefulWidget, Descripti
        this.onDidChangeDescriptionEmitter.fire();
    }

+    get badge(): number | undefined {
+        if (this._badge === undefined) {
+            for (const w of this.widgets) {


This algorithm seems strange. Other places seem to assume that the "wrapped" widget is this.widgets[0]. Why is the algorithm for badge different from descritption?

That is indeed more coherent here with this.widget[0].
This is adressed in the new version.

tsmaeder · 2023-03-23T09:25:00Z

packages/plugin-ext/src/plugin/webview-views.ts

+        this.assertNotDisposed();
+        if (this._badge !== badge) {
+            this._badge = badge;
+            this.proxy.$setBadge(this.handle, badge);


Same as above.

This has been adressed also in the new version, with same adaptation due to badge being potentially undefined.

tsmaeder · 2023-03-23T10:05:13Z

The "Calico Colors" example does not work reliably for me in electron, Invoking the action the first time, I see this in the back end log:

2023-03-23T10:02:55.325Z root ERROR [hosted-plugin: 22876] Promise rejection not handled in one second: Error: No webview found , reason: Error: No webview found
2023-03-23T10:02:55.325Z root ERROR [hosted-plugin: 22876] With stack trace: Error: No webview found
    at WebviewViewsExtImpl.getWebviewView (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\plugin\webview-views.js:75:19)
    at WebviewViewsExtImpl.$onDidChangeWebviewViewVisibility (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\plugin\webview-views.js:62:34)
    at RpcInvocationHandler.onNotification (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\common\proxy-handler.js:94:28)
    at C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\common\proxy-handler.js:87:43
    at C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:103:69
    at CallbackList.invoke (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:109:26)
    at Emitter.fire (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:224:29)
    at RpcProtocol.handleNotify (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:188:36)
    at RpcProtocol.handleMessage (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:68:26)
    at C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:47:66

Sometimes, the update of the badge via "add color" or "clear colors" works, though.

tsmaeder · 2023-03-27T14:37:06Z

@rschnekenbu it's usually a good idea to keep changes due to feedback in separate commits until we're ready to merge: it allows reviewers to see the latest changes, instead of having to re-review the whole thing.

rschnekenbu · 2023-03-27T20:01:14Z

@rschnekenbu it's usually a good idea to keep changes due to feedback in separate commits until we're ready to merge: it allows reviewers to see the latest changes, instead of having to re-review the whole thing.

@tsmaeder, thanks for advice! I will add commit on top of the reviewed one starting from now.

rschnekenbu · 2023-03-27T20:49:58Z

The "Calico Colors" example does not work reliably for me in electron, Invoking the action the first time, I see this in the back end log:

2023-03-23T10:02:55.325Z root ERROR [hosted-plugin: 22876] Promise rejection not handled in one second: Error: No webview found , reason: Error: No webview found
2023-03-23T10:02:55.325Z root ERROR [hosted-plugin: 22876] With stack trace: Error: No webview found
    at WebviewViewsExtImpl.getWebviewView (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\plugin\webview-views.js:75:19)
    at WebviewViewsExtImpl.$onDidChangeWebviewViewVisibility (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\plugin\webview-views.js:62:34)
    at RpcInvocationHandler.onNotification (C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\common\proxy-handler.js:94:28)
    at C:\Users\thomas\code\EclipseSource\theia\packages\plugin-ext\lib\common\proxy-handler.js:87:43
    at C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:103:69
    at CallbackList.invoke (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:109:26)
    at Emitter.fire (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\event.js:224:29)
    at RpcProtocol.handleNotify (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:188:36)
    at RpcProtocol.handleMessage (C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:68:26)
    at C:\Users\thomas\code\EclipseSource\theia\packages\core\lib\common\message-rpc\rpc-protocol.js:47:66

I sometimes get also the same issue, depending if the view is already selected or not in the tool. I have the same kind of issue on master, and I prefered not to spend too much time on debugging the vscode samples. There may be a test to add on the extension itself.

Sometimes, the update of the badge via "add color" or "clear colors" works, though.

Is this working only sometimes or does it works always after the second action? I could not have any issue while testing once the second action has been triggered, the first one failing with the view visibility.

I also have now an issue since rebasing on latest master (basically since pr #12326 was merged). The split icon terminal appears on all views, especially the calico color one. And the badges are hidden as soon as an action is in the toolbar. I had to patch the split command registration in order to get the badge working.

tsmaeder · 2023-03-29T11:27:37Z

Besides the "Split Terminal"-Problems above, the changes work fine for me.

tsmaeder · 2023-03-29T11:28:24Z

@rschnekenbu this one needs a rebase, as well.

Extend BadgeWidget with tooltip support Provide support for badge on tree based views and webview views Contributed on behalf of STMicroelectronics Signed-off-by: Remi SCHNEKENBURGER <rschnekenburger@eclipsesource.com>

rschnekenbu · 2023-03-29T12:12:26Z

@tsmaeder, as expected, the license check is failing, as for #12358. As the PR does not influence dependencies, is this still possible to approve the PR and merge it?

tsmaeder

Looks good to me, now.

tsmaeder · 2023-03-29T12:16:43Z

@rschnekenbu I guess I could: the licensing issues are not caused by this PR and they won't be fixed by it. The important thing re: licensing are releases, anyway, as far as I understand.

rschnekenbu mentioned this pull request Mar 21, 2023

[vscode] Support ViewBadge and tree/web view badge property eclipsesource/theia#72

Closed

2 tasks

JonasHelming requested a review from msujew March 22, 2023 07:43

tsmaeder self-requested a review March 23, 2023 08:33

tsmaeder reviewed Mar 23, 2023

View reviewed changes

rschnekenbu force-pushed the issues/12020 branch from beda943 to f72bf9b Compare March 27, 2023 13:49

rschnekenbu requested review from tsmaeder and removed request for msujew March 27, 2023 20:51

[vscode] Support ViewBadge and tree/web view badge property

e955bfe

Extend BadgeWidget with tooltip support Provide support for badge on tree based views and webview views Contributed on behalf of STMicroelectronics Signed-off-by: Remi SCHNEKENBURGER <rschnekenburger@eclipsesource.com>

rschnekenbu force-pushed the issues/12020 branch from f72bf9b to e955bfe Compare March 29, 2023 11:41

tsmaeder approved these changes Mar 29, 2023

View reviewed changes

tsmaeder merged commit 11ade90 into eclipse-theia:master Mar 29, 2023

vince-fugnitto added this to the 1.36.0 milestone Mar 30, 2023

vince-fugnitto added the vscode issues related to VSCode compatibility label Mar 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vscode] Support ViewBadge and tree/webview view badge property #12330

[vscode] Support ViewBadge and tree/webview view badge property #12330

rschnekenbu commented Mar 21, 2023

tsmaeder left a comment

tsmaeder Mar 23, 2023

rschnekenbu Mar 27, 2023

tsmaeder Mar 23, 2023

rschnekenbu Mar 27, 2023

tsmaeder Mar 23, 2023

rschnekenbu Mar 27, 2023

tsmaeder commented Mar 23, 2023

tsmaeder commented Mar 27, 2023

rschnekenbu commented Mar 27, 2023

rschnekenbu commented Mar 27, 2023 •

edited

Loading

tsmaeder commented Mar 29, 2023

tsmaeder commented Mar 29, 2023

rschnekenbu commented Mar 29, 2023

tsmaeder left a comment

tsmaeder commented Mar 29, 2023

[vscode] Support ViewBadge and tree/webview view badge property #12330

[vscode] Support ViewBadge and tree/webview view badge property #12330

Conversation

rschnekenbu commented Mar 21, 2023

What it does

How to test

Review checklist

Reminder for reviewers

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder Mar 23, 2023

Choose a reason for hiding this comment

rschnekenbu Mar 27, 2023

Choose a reason for hiding this comment

tsmaeder Mar 23, 2023

Choose a reason for hiding this comment

rschnekenbu Mar 27, 2023

Choose a reason for hiding this comment

tsmaeder Mar 23, 2023

Choose a reason for hiding this comment

rschnekenbu Mar 27, 2023

Choose a reason for hiding this comment

tsmaeder commented Mar 23, 2023

tsmaeder commented Mar 27, 2023

rschnekenbu commented Mar 27, 2023

rschnekenbu commented Mar 27, 2023 • edited Loading

tsmaeder commented Mar 29, 2023

tsmaeder commented Mar 29, 2023

rschnekenbu commented Mar 29, 2023

tsmaeder left a comment

Choose a reason for hiding this comment

tsmaeder commented Mar 29, 2023

rschnekenbu commented Mar 27, 2023 •

edited

Loading