Add support for sub heading in extension views #9909

aaronrhodes · 2021-08-16T10:08:58Z

What it does

Adds the description field to TreeView from the vscode API.

resolves #9903

How to test

Add a plugin that utilises the TreeView description field.

There is a modified version of the vscode tree-view-sample available in the following repo:

https://github.com/aaronrhodes/vscode-extension-samples

build, and copy or symlink this example into the plugins directory and select a file using the "file explorer" panel in the explorer tab.

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

msujew

Thank you for your contribution, the changes look good to me so far. The description property does its job.

FYI the title of your PR is a bit misleading, as you're adding support for the description property for views contributed by extensions, not only for outlines.

I have a few remarks before approving.

packages/core/src/browser/shell/description-widget.ts

vince-fugnitto

I noticed a couple of things:

the commit message needs to be updated, we mention outline-view but I believe we mean all view-container-parts.
the description is only visible if the view-container-part is currently expanded in vscode - I believe we should align.

vince-fugnitto · 2021-08-16T14:21:37Z

packages/core/src/browser/shell/description-widget.ts

+export interface DescriptionWidget {
+    description: string;
+    onDescriptionChange: Emitter<void>;
+}
+
+export namespace DescriptionWidget {
+    /* eslint-disable @typescript-eslint/no-explicit-any */
+    export function is(arg: any): arg is DescriptionWidget {
+        return arg !== undefined && !!arg['onDescriptionChange'];
+    }


I wasn't so sure the file was necessary, and instead it could be directly added to the tree-view.
Adding it under shell/ is also a bit awkward.

I'm now moved this file into view-container.ts as it only really applies to the container widgets

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

aaronrhodes · 2021-08-18T15:11:03Z

I noticed a couple of things:

the commit message needs to be updated, we mention outline-view but I believe we mean all view-container-parts.

the description is only visible if the view-container-part is currently expanded in vscode - I believe we should align.

thanks for the feedback! Have updated the commit message and the subheading now hides and shows when the panel is collapsed/expanded respectively.

msujew · 2021-08-18T16:06:26Z

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

+            if (DescriptionWidget.is(this.wrapped) && !this.collapsed) {
+                subHeading.innerText = !!this.wrapped.description ? this.wrapped.description : '';
+            } else {
+                subHeading.innerText = '';
+            }


Nitpick: We can simplify this

Suggested change

if (DescriptionWidget.is(this.wrapped) && !this.collapsed) {

subHeading.innerText = !!this.wrapped.description ? this.wrapped.description : '';

} else {

subHeading.innerText = '';

}

subHeading.innerText = DescriptionWidget.is(this.wrapped) && !this.collapsed && this.wrapped.description || '';

vince-fugnitto · 2021-08-18T18:55:59Z

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

@@ -47,6 +47,17 @@ export class ViewContainerIdentifier {
    progressLocationId?: string;
 }

+export interface DescriptionWidget {
+    description: string;
+    onDescriptionChange: Emitter<void>;


Given our coding guidelines, the event name should be renamed to something like:

Suggested change

onDescriptionChange: Emitter<void>;

onDidChangeDescription: Emitter<void>;

vince-fugnitto · 2021-08-18T18:57:09Z

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

+    protected readonly onSubHeadingChangedEmitter = new Emitter<void>();
+    readonly onSubHeadingChanged = this.onSubHeadingChangedEmitter.event;


Similarly to the above comment, the event name should be renamed:

Suggested change

protected readonly onSubHeadingChangedEmitter = new Emitter<void>();

readonly onSubHeadingChanged = this.onSubHeadingChangedEmitter.event;

protected readonly onDidChangeSubHeadingEmitter = new Emitter<void>();

readonly onDidChangeSubHeading = this.onDidChangeSubHeadingEmitter.event;

I understand we have onTitleChanged but I believe that example is legacy, perhaps before we put in place the guideline.

vince-fugnitto · 2021-08-18T19:00:38Z

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

@@ -699,6 +712,12 @@ export class ViewContainerPart extends BaseWidget {
        this.wrapped.title.changed.connect(fireTitleChanged);
        this.toDispose.push(Disposable.create(() => this.wrapped.title.changed.disconnect(fireTitleChanged)));

+        if (DescriptionWidget.is(this.wrapped)) {
+            const fireSubHeadingChanged = () => this.onSubHeadingChangedEmitter.fire(undefined);
+            const sub = (this.wrapped as DescriptionWidget)?.onDescriptionChange.event(fireSubHeadingChanged);


Update to a more meaningful name than sub?

Removed as it was only really used for debug anyway

vince-fugnitto · 2021-08-18T19:11:24Z

packages/core/src/browser/style/view-container.css

    flex: 1;
    overflow: hidden;
    white-space: nowrap;
    text-overflow: ellipsis;
+    padding-left: 12px;


We should compute this value I believe rather than hardcoding.
Example:

theia/packages/debug/src/browser/style/index.css

Line 142 in a6422c3

padding-left: var(--theia-ui-padding);

Great suggestion, thanks!

colin-grant-work · 2021-08-23T17:00:59Z

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

+    protected readonly onDidSubHeadingChangedEmitter = new Emitter<void>();
+    readonly onDidSubHeadingChanged = this.onDidSubHeadingChangedEmitter.event;


I think onDidChangeSubHeading(Emitter) would conform more closely to the guidelines on event naming.

No probs, have updated all of these references!

colin-grant-work · 2021-08-23T17:03:19Z

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

+        if (DescriptionWidget.is(this.wrapped)) {
+            const fireSubHeadingChanged = () => this.onDidSubHeadingChangedEmitter.fire(undefined);
+            this.toDispose.push((this.wrapped as DescriptionWidget)?.onDidDescriptionChange.event(fireSubHeadingChanged));


Since you've performed a type check already, you shouldn't need the as cast or the ?. conditional accessor.

Why the change in terminology here from onDidChangeDescription to onDidChangeSubHeading? I think I'd prefer that we use the description-type names throughout and leave the fact that they're rendered as a subheading as an implementation detail.

colin-grant-work · 2021-08-23T17:52:21Z

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

@@ -50,6 +52,8 @@ export class PluginViewWidget extends Panel implements StatefulWidget {
        this.node.style.height = '100%';
    }

+    public onDidDescriptionChange: Emitter<void> = new Emitter<void>();


Suggested change

public onDidDescriptionChange: Emitter<void> = new Emitter<void>();

onDidChangeDescription: Emitter<void> = new Emitter<void>();

Signed-off-by: Aaron Rhodes <aaron.rhodes@arm.com>

aaronrhodes · 2021-08-26T14:48:09Z

thanks @colin-grant-work - have addressed feedback

colin-grant-work

This is working well for me. I was surprised to find that if the ViewContainerPart is the only widget in its ViewContainer, the title bar is not updated:

Only child:

With sibling:

But it looks like that's the same behavior as in VSCode:

Only child:

With sibling:

In other contexts, I've remarked that it would be nice to extend Phosphor's Title to handle additional fields not foreseen by Phosphor (pinning, descriptions). There are also two existing fields that we currently treat as more or less interchangeable: label and caption. Arguably, caption could be used for description, but we'd have to check all existing code to ensure we're not relying on caption and label being interchangeable. Unfortunately, extending the Title wouldn't be trivial, as it's assigned in a somewhat eccentric way in Phosphor. I think this PR achieves its objectives effectively given the means available, but I wanted to remark on some of the problem areas that it touches for future reference.

msujew reviewed Aug 16, 2021

View reviewed changes

packages/core/src/browser/shell/description-widget.ts Outdated Show resolved Hide resolved

packages/core/src/browser/shell/description-widget.ts Outdated Show resolved Hide resolved

vince-fugnitto reviewed Aug 16, 2021

View reviewed changes

vince-fugnitto added tree issues related to the tree (ex: tree widget) vscode issues related to VSCode compatibility labels Aug 16, 2021

aaronrhodes force-pushed the ar/add-vscode-treeview-description branch from bc50b40 to 929d152 Compare August 18, 2021 15:06

aaronrhodes force-pushed the ar/add-vscode-treeview-description branch from 929d152 to 4466fc2 Compare August 18, 2021 16:02

msujew reviewed Aug 18, 2021

View reviewed changes

vince-fugnitto reviewed Aug 18, 2021

View reviewed changes

aaronrhodes changed the title ~~Add support for sub heading in outline view~~ Add support for sub heading in extension views Aug 19, 2021

aaronrhodes force-pushed the ar/add-vscode-treeview-description branch from 4466fc2 to 37ef12f Compare August 19, 2021 14:10

colin-grant-work reviewed Aug 23, 2021

View reviewed changes

Add support for sub heading in view-container-part

d880c0e

Signed-off-by: Aaron Rhodes <aaron.rhodes@arm.com>

aaronrhodes force-pushed the ar/add-vscode-treeview-description branch from 37ef12f to d880c0e Compare August 26, 2021 14:47

colin-grant-work approved these changes Aug 26, 2021

View reviewed changes

paul-marechal added this to the 1.18.0 milestone Aug 26, 2021

westbury merged commit 7442150 into eclipse-theia:master Aug 31, 2021

westbury deleted the ar/add-vscode-treeview-description branch August 31, 2021 08:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for sub heading in extension views #9909

Add support for sub heading in extension views #9909

aaronrhodes commented Aug 16, 2021 •

edited

Loading

msujew left a comment •

edited

Loading

vince-fugnitto left a comment

vince-fugnitto Aug 16, 2021

aaronrhodes Aug 18, 2021

aaronrhodes commented Aug 18, 2021

msujew Aug 18, 2021

aaronrhodes Aug 19, 2021

vince-fugnitto Aug 18, 2021

vince-fugnitto Aug 18, 2021

aaronrhodes Aug 19, 2021

vince-fugnitto Aug 18, 2021

aaronrhodes Aug 19, 2021

vince-fugnitto Aug 18, 2021

aaronrhodes Aug 19, 2021

colin-grant-work Aug 23, 2021

aaronrhodes Aug 26, 2021

colin-grant-work Aug 23, 2021

aaronrhodes Aug 26, 2021

colin-grant-work Aug 23, 2021

aaronrhodes Aug 26, 2021

aaronrhodes commented Aug 26, 2021

colin-grant-work left a comment

	onDescriptionChange: Emitter<void>;
	onDidChangeDescription: Emitter<void>;

		protected readonly onSubHeadingChangedEmitter = new Emitter<void>();
		readonly onSubHeadingChanged = this.onSubHeadingChangedEmitter.event;

		protected readonly onDidSubHeadingChangedEmitter = new Emitter<void>();
		readonly onDidSubHeadingChanged = this.onDidSubHeadingChangedEmitter.event;

	public onDidDescriptionChange: Emitter<void> = new Emitter<void>();
	onDidChangeDescription: Emitter<void> = new Emitter<void>();

Add support for sub heading in extension views #9909

Add support for sub heading in extension views #9909

Conversation

aaronrhodes commented Aug 16, 2021 • edited Loading

What it does

How to test

Review checklist

Reminder for reviewers

msujew left a comment • edited Loading

Choose a reason for hiding this comment

vince-fugnitto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronrhodes commented Aug 18, 2021

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

aaronrhodes commented Aug 26, 2021

colin-grant-work left a comment

Choose a reason for hiding this comment

aaronrhodes commented Aug 16, 2021 •

edited

Loading

msujew left a comment •

edited

Loading