[VSX] Display plugins counts #11248

alvsan09 · 2022-06-02T16:56:42Z

What it does

Adds a plugin count badge to the title of each instance of
VSXExtensionsWidget except for SEARCH_RESULT as the count may be
partial.

The solution extends the functionality under view-container by adding a BadgeWidget interface that can be
implemented by widgets wrapped in a ViewContainerPart, this is the reference implementation for it.
This solution does not show the badge for view parts that contain a toolbar populated with more than one item.

How to test

Add an extensions.json file with the following content to your workspace .theia folder:

{
    "recommendations": [
        "ms-azuretools.vscode-docker",
        "exiasr.hadolint",
        "timonwong.shellcheck",
        "coenraads.bracket-pair-colorizer-2",
        "scalameta.metals"
    ]
}

Start theia and open the EXTENSIONS view
Install few extensions
Verify the counter values for each widget i.e. INSTALLED, BUILT-IN, RECOMMENDED
As per the listed extensions above the expected count for RECOMMENDED is 5
Make sure other view parts don't display a badge e.g. WATCH under the DEBUG view.

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

vince-fugnitto

@alvsan09 if you're up for it I think we should try and aim to do something similar to vscode in this case, perhaps by updating ViewContainerPart to accept a badge property that can be applied if no toolbar items are registered. It can of course be styled similarly to vscode:

I'm also wondering if we should drop the count from the context-menu, it's not something I see done in other areas or in vscode:

msujew

Looks good from a surface viewpoint. Haven't looked into the code yet. I noticed some minor layouting issues, where the badge clips into the focus outline. I believe we need a larger margin there:

VSCode:

alvsan09 · 2022-06-09T03:33:20Z

Looks good from a surface viewpoint. Haven't looked into the code yet. I noticed some minor layouting issues, where the badge clips into the focus outline. I believe we need a larger margin there:

I also received some comments off-line from @vince-fugnitto , I am in the process to change the code, i.e.

Give the responsibility to render the badge to the ViewContainerPart rather than from the VSXExtensionsWidget
Make the BadgeWidget interface more specific to contain a badge of type number | undefined rather that node | undefined
Make sure that if a part contains a toolbar the badge is not displayed.

I will add a right margin to it as you have pointed out as well,
Additional comments are very welcome !
Thanks @msujew !

vince-fugnitto · 2022-06-13T15:49:19Z

packages/core/src/browser/style/notification.css

@@ -34,6 +34,10 @@
    text-align: center;
 }

+.notification-count-container-margin {


We should drop -margin from the classname.

vince-fugnitto · 2022-06-13T15:50:01Z

packages/core/src/browser/style/notification.css

@@ -34,6 +34,10 @@
    text-align: center;
 }

+.notification-count-container-margin {


I believe this file is strictly for notifications styling, shouldn't we put our styling under view-container.css?
The use of notifications for badge styling in view-container seems a little odd.

vince-fugnitto · 2022-06-13T15:51:46Z

packages/vsx-registry/src/browser/vsx-extensions-widget.ts

+    protected async computeTitle(): Promise<void> {
+        let label: string;
        switch (this.options.id) {
            case VSXExtensionsSourceOptions.INSTALLED:
-                return nls.localizeByDefault('Installed');
+                label = nls.localizeByDefault('Installed');
+                break;
            case VSXExtensionsSourceOptions.BUILT_IN:
-                return nls.localizeByDefault('Built-in');
+                label = nls.localizeByDefault('Built-in');
+                break;
            case VSXExtensionsSourceOptions.RECOMMENDED:
-                return nls.localizeByDefault('Recommended');
+                label = nls.localizeByDefault('Recommended');
+                break;
            case VSXExtensionsSourceOptions.SEARCH_RESULT:
-                return nls.localize('theia/vsx-registry/openVSX', 'Open VSX Registry');
+                label = nls.localize('theia/vsx-registry/openVSX', 'Open VSX Registry');
+                break;
            default:
-                return '';
+                label = '';
+        }
+        const title = `${label}`;
+        this.title.label = title;
+        this.title.caption = title;
+
+        this.badge = await this.resolveCount();
+    }


I believe this method is doing too much work and merges two concepts together (which is confusing), the title of the view-part and handling the badge.

vince-fugnitto · 2022-06-13T15:52:56Z

packages/vsx-registry/src/browser/vsx-extensions-widget.ts

+    protected async resolveCount(): Promise<number | undefined> {
+        if (this.options.id !== VSXExtensionsSourceOptions.SEARCH_RESULT) {
+            const elements = await this.source?.getElements() || [];
+            return [...elements].length;
        }
+        return undefined;


The changes do not properly handle a 0 count:

In this case I would still expect 0 to be displayed for recommended and installed.

I have submitted a new commit to address these comments !!
Thanks @vince-fugnitto !!

packages/vsx-registry/src/browser/vsx-extensions-widget.ts

vince-fugnitto

I confirm that the changes work as expected:

a badge of 0 is supported and displayed
the 'builtin', 'recommended' and 'installed' tree-view-parts have badges
the badges correctly display the extension count for each part
the badges are correctly updated when the count is updated

vince-fugnitto · 2022-06-14T16:21:15Z

packages/vsx-registry/src/browser/vsx-extensions-widget.ts

+        }));
+    }
+
+    private _badgeCount: number | undefined = undefined;


minor: we should rename to _badge due to the getter and setter, or just use badgeCount (dropping the unnecessary _). I'm in favor of using _badge however.

https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#no_underscore_private

I agree with you, _badge is more appropriate,
It has been updated in the latest commit,
I have also squashed the commits and re-based it to later master.

Thanks @vince-fugnitto !

Adds a plugin count to the title and captions of each instance of `VSXExtensionsWidget` except for `SEARCH_RESULT` as the count may be partial. The solution extends the functionality under view-container by adding a BadgeWidget interface that can be implemented by widgets wrapped in a ViewContainerPart, this is the reference implementation for it. This solution does not show the badge for view parts that contain a toolbar populated with more than one item. Signed-off-by: Alvaro Sanchez-Leon <alvaro.sanchez-leon@ericsson.com>

msujew

Looks good to me 👍

The extension count is displayed on the view parts as a badge (not the context menu)
Not other view parts/widgets are affected by this change
The badge updates when installing an extension

vince-fugnitto

LGTM 👍

alvsan09 · 2022-06-15T17:06:32Z

@msujew, @vince-fugnitto, Thanks !! 👍

vince-fugnitto added ui/ux issues related to user interface / user experience vsx-registry Issues related to Open VSX Registry Integration labels Jun 3, 2022

vince-fugnitto reviewed Jun 3, 2022

View reviewed changes

alvsan09 force-pushed the asl/display_plugins_count branch from 9808f3b to be3202c Compare June 8, 2022 13:22

msujew self-requested a review June 8, 2022 13:39

msujew reviewed Jun 8, 2022

View reviewed changes

alvsan09 force-pushed the asl/display_plugins_count branch from be3202c to 3b38e20 Compare June 10, 2022 16:56

alvsan09 requested review from msujew and vince-fugnitto June 10, 2022 18:47

vince-fugnitto reviewed Jun 13, 2022

View reviewed changes

alvsan09 force-pushed the asl/display_plugins_count branch from 3b38e20 to 4791a08 Compare June 13, 2022 17:56

alvsan09 requested a review from vince-fugnitto June 13, 2022 18:00

paul-marechal reviewed Jun 13, 2022

View reviewed changes

packages/vsx-registry/src/browser/vsx-extensions-widget.ts Outdated Show resolved Hide resolved

alvsan09 force-pushed the asl/display_plugins_count branch from 2c08a97 to d538744 Compare June 13, 2022 19:05

vince-fugnitto reviewed Jun 14, 2022

View reviewed changes

alvsan09 force-pushed the asl/display_plugins_count branch from 5670264 to a535501 Compare June 14, 2022 16:55

msujew approved these changes Jun 15, 2022

View reviewed changes

vince-fugnitto approved these changes Jun 15, 2022

View reviewed changes

alvsan09 merged commit f6c0689 into master Jun 15, 2022

github-actions bot added this to the 1.27.0 milestone Jun 15, 2022

alvsan09 deleted the asl/display_plugins_count branch June 15, 2022 17:05

vince-fugnitto mentioned this pull request Nov 4, 2022

[enhancement] display the number of 'enabled' plugins #6710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VSX] Display plugins counts #11248

[VSX] Display plugins counts #11248

alvsan09 commented Jun 2, 2022 •

edited

Loading

vince-fugnitto left a comment

msujew left a comment

alvsan09 commented Jun 9, 2022 •

edited

Loading

vince-fugnitto Jun 13, 2022

vince-fugnitto Jun 13, 2022

vince-fugnitto Jun 13, 2022

vince-fugnitto Jun 13, 2022

alvsan09 Jun 13, 2022

vince-fugnitto left a comment

vince-fugnitto Jun 14, 2022

alvsan09 Jun 14, 2022

msujew left a comment

vince-fugnitto left a comment

alvsan09 commented Jun 15, 2022

[VSX] Display plugins counts #11248

[VSX] Display plugins counts #11248

Conversation

alvsan09 commented Jun 2, 2022 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

vince-fugnitto left a comment

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

alvsan09 commented Jun 9, 2022 • edited Loading

vince-fugnitto Jun 13, 2022

Choose a reason for hiding this comment

vince-fugnitto Jun 13, 2022

Choose a reason for hiding this comment

vince-fugnitto Jun 13, 2022

Choose a reason for hiding this comment

vince-fugnitto Jun 13, 2022

Choose a reason for hiding this comment

alvsan09 Jun 13, 2022

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

vince-fugnitto Jun 14, 2022

Choose a reason for hiding this comment

alvsan09 Jun 14, 2022

Choose a reason for hiding this comment

msujew left a comment

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

alvsan09 commented Jun 15, 2022

alvsan09 commented Jun 2, 2022 •

edited

Loading

alvsan09 commented Jun 9, 2022 •

edited

Loading