[outline] Use the `DocumentSymbol.detail` not just only the `Document… #3314

vrubezhny · 2018-10-29T20:40:55Z

…Symbol.name` #2894

This fix:

adds the decorator service to the outline
adds the tree node decorator to the monaco editor in order to decorate nodes using DocumentSymbo's detail property

Signed-off-by: Victor Rubezhny vrubezhny@redhat.com

[outline] Outline is added with a possibility to decorate the text of the tree elements.

kittaakos

Nice feature, perhaps we could do this a bit differently. I did not want to add any inline comments but comment on this in general and discuss it before getting into the code.

The colored ranges is a useful addition, but I do not like the way we're extracting the detail meta-information from the name of the tree node by checking the position of the colon (:). Instead;

I would update the OutlineSymbolInformationNode with an optional detail property.
Proposal: instead of having a decorator for the Outline (that we might need later), we can attach the decoration information to the tree node.

https://github.com/theia-ide/theia/blob/8bb854571382333ffe50d2e209e3d6ec7c1bf203/packages/core/src/browser/tree/tree-widget.tsx#L594-L603

https://github.com/theia-ide/theia/blob/8bb854571382333ffe50d2e209e3d6ec7c1bf203/packages/core/src/browser/tree/tree-decorator.ts#L507-L525

If rather stick to your approach, please update the JavaDecorator to use OutlineSymbolInformationNode instead of the TreeNode.
Finally, you could use the caption suffixes to achieve the same UX. See here. No colored ranges are available; still one can use different colors for the label.

kittaakos

I added a few remarks if you are willing to continue with your initial idea.

kittaakos · 2018-10-30T07:21:02Z

packages/core/src/browser/tree/tree-decorator.ts

+                    if (color) {
+                        result.push({ data: caption.substr(i, length), color: color });
+                    } else {
+                        result.push({ data: caption.substr(i, length), highligh: true, color: color });


We have to fix this typo: highligh.

yes, probably there was a typo. I just left it as it was before.

Not sure what the policy is in Theia about drive-by fixes.

kittaakos · 2018-10-30T07:21:46Z

packages/java/src/browser/java-decorator.ts

+        const name = node.name;
+        const colon = name.indexOf(':');
+        return {
+                priority: 200, highlight: {ranges: [{offset: colon, length: name.length - colon,  color: '0x847a4c'}]}


0x847a4c => We need to use pre-defined constants instead.

This color value is actually not used. I just couldn't find a proper way to use here something like var(--theia-word-highlight-color0), so the actual color to be used is specified in browser/style/tree.css like:

.theia-TreeNodeSegment label { color: var(--theia-word-highlight-color0); }

The point is that the highlight property stays undefined and, as such, label element is used to decorate the text instead of mark

If it's unused, we should use a constant that marks it as such, e.g. '0xdeadbeef" or something similar.

kittaakos · 2018-10-30T07:22:15Z

packages/outline-view/src/browser/outline-decorator-service.ts

+export const OutlineTreeDecorator = Symbol('OutlineTreeDecorator');
+
+/**
+ * Decorator service for the navigator.


navigator => copy-paste. (Same above.)

sure. to be fixed

kittaakos · 2018-10-30T07:22:56Z

packages/java/src/browser/java-decorator.ts

+    protected readonly emitter = new Emitter<(tree: Tree) => Map<string, TreeDecoration.Data>>();
+
+    @postConstruct()
+    protected init(): void {


kittaakos · 2018-10-30T07:27:10Z

packages/java/src/browser/java-decorator.ts

+            const { name } = treeNode;
+            const colon = name.indexOf(':');
+
+            if (colon !== -1 && name.length > colon + 1) {


I am a bit puzzled; is name.length > colon + 1 equivalent to !name.endsWith(':')? If so, please simplify. Otherwise, can you please explain to me what use-case you want to handle here?

Yes, exactly.

kittaakos · 2018-10-30T07:28:14Z

packages/core/src/browser/tree/tree-decorator.ts

@@ -383,6 +395,9 @@ export namespace TreeDecoration {

        }

+        export namespace ColoredRange {


it's used instead of Range in CaptionHighltght for ranges property:

export interface CaptionHighlight { /** * The ranges to highlight in the caption. */ readonly ranges: CaptionHighlight.ColoredRange[] ...

But again, I couldn't properly set it when creating ranges in JavaDecorator (so, the color defined in css is used instead)

Sorry, I know, there is a lot of such inconsistencies in this PR, but the intention was to present the solution to the discussion and find a way to improve it and fix it before it can be accepted.

vrubezhny · 2018-10-30T14:02:25Z

I like your proposal with adding detail meta information, but...

I would update the OutlineSymbolInformationNode with an optional detail property.

Proposal: instead of having a decorator for the Outline (that we might need later), we can attach the decoration information to the tree node.

Who'll bring this additional information to the node? Do we still need some kind of decorator to provide such info for the nodes?
Currently, all the TreeNodes are created in https://github.com/theia-ide/theia/blob/master/packages/monaco/src/browser/monaco-outline-contribution.ts#L275, so we can't distinguish here between a css style node and a java node, for example. (In order to decorate the only JavaElement nodes I've added a filter that checks if the current editor is a Java editor, so the other kinds of outline tree nodes (like, for example, in CSS editor: .theia-ExpansionToggle:hover) stay not decorated)

If rather stick to your approach, please update the JavaDecorator to use OutlineSymbolInformationNode instead of the TreeNode.

Just a question: why? I don't use any special properties/methods of OutlineSymbolInformationNode, the generic TreeNode has all I need...

Finally, you could use the caption suffixes to achieve the same UX. See here. No colored ranges are available; still one can use different colors for the label.

Thanks for pointing me to this type hierarchy example, I was looking mostly at git decorations that use caption suffixes and there I didn't spot that I can use them the same way I did in my solution. Shame on me, but well, this is my first try :)

kittaakos · 2018-10-30T14:23:12Z

Who'll bring this additional information to the node?

It is on the DocumentSymbol. We can generalize it. If the detail is there, we decorate it. Otherwise, we do nothing. There's no need to have specific code for the Java Outline.

Just a question: why? I don't use any special properties/methods of OutlineSymbolInformationNode, the generic TreeNode has all I need...

I think it would be better to work with OutlineSymbolInformationNode if you use it for the Outline. And as I said, I think we could do better than finding a non-trailing colon in text and do something based on that.

tsmaeder · 2018-11-05T09:54:36Z

packages/java/src/browser/java-decorator.ts

+        const name = node.name;
+        const colon = name.indexOf(':');
+        return {
+                priority: 200, highlight: {ranges: [{offset: colon, length: name.length - colon,  color: '0x847a4c'}]}


If it's unused, we should use a constant that marks it as such, e.g. '0xdeadbeef" or something similar.

tsmaeder · 2018-11-05T10:45:40Z

packages/core/src/browser/tree/tree-decorator.ts

+                    if (color) {
+                        result.push({ data: caption.substr(i, length), color: color });
+                    } else {
+                        result.push({ data: caption.substr(i, length), highligh: true, color: color });


Not sure what the policy is in Theia about drive-by fixes.

tsmaeder · 2018-11-05T10:52:40Z

packages/java/src/browser/java-decorator.ts

+import { JAVA_LANGUAGE_ID } from '../common';
+
+@injectable()
+export class JavaDecorator implements TreeDecorator {


I agree with @kittaakos that use of "details" should not be Java-specific. See my comment in the PR main page for more.

tsmaeder · 2018-11-05T11:05:18Z

I'm liking the conversation in this PR, here's my 2 cents:

DocumentSymbol.detail is not specific to Java, so our handling of it should not be Java-specific
However, the fact that we show DocumentSymbol instances is specific to the MonacoOutlineContribution. If we extend an interface, we should extend that one.
We do not know what kinds of information decorators need to have: we should expose the "DocumentSymbol" object the tree node represents in MonacoOutlineSymbolInformationNode
We can use property accessors to compute the things we need to implement the TreeNode interface based on the SymbolData, so when we create the tree nodes at https://github.com/theia-ide/theia/blob/master/packages/monaco/src/browser/monaco-outline-contribution.ts#L275 , we should not just return an anonymous JS object, but a class
Tree decorators can add prefixes and suffixes to the tree node caption. Shouldn't we use this instead of futzing with the highlights? Shouldn't 'detail' be a suffix?

vrubezhny · 2018-11-16T17:23:03Z

The PR is updated

tsmaeder · 2018-11-21T10:36:32Z

packages/monaco/src/browser/monaco-outline-decorator.ts

+        return this.emitter.event;
+    }
+
+    protected collectDecorators(tree: Tree): Map<string, TreeDecoration.Data> {


This seems copied from the GitTreeDecorator, but the naming here is off: this is not collecting decorators, this is collecting decorations. The method should be named "collectDecorations"

tsmaeder · 2018-11-21T10:39:09Z

packages/monaco/src/browser/monaco-outline-decorator.ts

+                result.set(treeNode.id, treeNode);
+            }
+        }
+        return new Map(Array.from(result.entries()).map(m => [m[0], this.toDecorator(m[1])] as [string, TreeDecoration.Data]));


I have no idea why we're not producing a map of id->decoration directly. Why go through this dance with creating an array, iterating over it, etc?

agree again.

packages/outline-view/src/browser/outline-view-frontend-module.ts

vrubezhny · 2018-11-21T16:00:48Z

I've rebased and squashed the commits of the PR

kittaakos · 2018-11-22T17:12:24Z

packages/monaco/src/browser/monaco-outline-contribution.ts

@@ -342,6 +347,7 @@ export interface MonacoOutlineSymbolInformationNode extends OutlineSymbolInforma
    uri: URI;
    range: Range;
    fullRange: Range;
+    detail: string;


I have not yet tried your changes in action but I think this should be detail?: string instead. See the DocumentSymbol.detail in the specification.

I thought it should be detail?: string, but then I found its definition in monaco.d.ts in node_modules/@typefox/monaco-editor-core/ which tells that it's not optional:

That's why I've made it to be detail: string. Still not sure if it's correct, but changing it to be optional will change nothing as it will be always initialized from DocumentSymbol.detail which is not an optional.

DocumentSymbol.detail which is not an optional.

@akosyakov, what's the truth? The TS-LS fills the detail with an empty string, but according to the LSP, DocumentSymbol.detail is optional. Thanks!

Since we don't control monace.d.ts we will have to handle the discrepancy: note that null is a valid value for a non-optional field, so we just have to handle the case of "undefined field" in the LSP version of DcocumentSymbol correctly.

Since we don't control monace.d.ts

FYI: @typefox/monaco-editor-core

What I mean is that it doesn't really matter: we either have a value or we don't. If monaco wants a value, we can pass null if we don't have on.

@akosyakov, ping.

what's the truth?

In VS Code and Monaco DocumentSymobl.detail is mandatory, in LSP is optional. They are all different APIs.

The TS-LS fills the detail with an empty string, but according to the LSP, DocumentSymbol.detail is optional.

TS-LS fills detail with empty string to work around a bug in version of vscode-languageserver-node which we use for Monaco language client. It can not distinguish between LSP DocumentSymbol and SymbolInformation otherwise: https://github.com/TypeFox/vscode-languageserver-node/blob/fb80c5e2db4c714032c28d6f482e770642ae9e23/types/src/main.ts#L1701

I still don't see why this makes a difference: if there is no value, let's use an emtpy string and be done with. What is the actual problem here? @akosyakov

work around a bug in version of vscode-languageserver-node which we use for Monaco language client. It can not distinguish between LSP DocumentSymbol and SymbolInformation

Note, the issue seems to be fixed in the original source: https://github.com/Microsoft/vscode-languageserver-node/blob/c80ead2e4162cf3f7b547ea2dfc57e64dbd6d423/types/src/main.ts#L1903-L1911

kittaakos

It works nicely. Great enhancement! I already added a comment; the detail should be optional.

kittaakos

Thanks for the patch!

kittaakos · 2018-11-28T12:50:17Z

I have restarted the Windows build. Presumably, it was failing with an unrelated error.
@vrubezhny, ping me if I should push the "merge" button.

vrubezhny · 2018-11-28T17:35:54Z

@kittaakos I've added a fixup that makes MonacoOutlineSymbolInformationNode.detail to be optional. So, if DocumentSymbol.detail value of a node is undefined - it should be treated correctly and such a node is not to be decorated.
Please, review.

kittaakos · 2018-12-03T13:56:58Z

I am checking this PR again...

kittaakos

It works great! Sorry for not noticing your latest comment on the PR, @vrubezhny.

vrubezhny · 2018-12-03T14:26:52Z

@kittaakos Good! Then I'm going to squash it, rebase and then merge it

kittaakos · 2018-12-03T14:31:15Z

Good! Then I'm going to squash it, rebase and then merge it

Super, thanks!

…Symbol.name` #2894 This fix: - adds the decorator service to the outline - adds the tree node decorator to the monaco editor in order to decorate nodes using DocumentSymbo's detail property Signed-off-by: Victor Rubezhny <vrubezhny@redhat.com>

vrubezhny mentioned this pull request Oct 29, 2018

[outline] Use the DocumentSymbol.detail not just only the DocumentSymbol.name #2894

Closed

kittaakos self-requested a review October 30, 2018 07:00

kittaakos reviewed Oct 30, 2018

View reviewed changes

tsmaeder requested changes Nov 5, 2018

View reviewed changes

tsmaeder mentioned this pull request Nov 12, 2018

Languages Sprint 158 (K-3) eclipse-che/che#11903

Closed

15 tasks

tsmaeder requested changes Nov 21, 2018

View reviewed changes

tsmaeder approved these changes Nov 21, 2018

View reviewed changes

tsmaeder approved these changes Nov 22, 2018

View reviewed changes

kittaakos self-requested a review November 22, 2018 17:08

kittaakos reviewed Nov 22, 2018

View reviewed changes

kittaakos reviewed Nov 23, 2018

View reviewed changes

kittaakos approved these changes Nov 28, 2018

View reviewed changes

kittaakos approved these changes Dec 3, 2018

View reviewed changes

vrubezhny merged commit 5b3d0aa into eclipse-theia:master Dec 4, 2018

tsmaeder mentioned this pull request Dec 6, 2018

Languages Sprint 159 (L-1) eclipse-che/che#12135

Closed

19 tasks

		@@ -383,6 +395,9 @@ export namespace TreeDecoration {

		}

		export namespace ColoredRange {

[outline] Use the DocumentSymbol.detail not just only the `Document… #3314

[outline] Use the DocumentSymbol.detail not just only the `Document… #3314

Conversation

vrubezhny commented Oct 29, 2018 • edited Loading

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos 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

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

vrubezhny commented Oct 30, 2018

kittaakos commented Oct 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsmaeder commented Nov 5, 2018

vrubezhny commented Nov 16, 2018

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

vrubezhny commented Nov 21, 2018

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

tsmaeder Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos left a comment

Choose a reason for hiding this comment

kittaakos commented Nov 28, 2018

vrubezhny commented Nov 28, 2018 • edited Loading

kittaakos commented Dec 3, 2018

kittaakos left a comment

Choose a reason for hiding this comment

vrubezhny commented Dec 3, 2018

kittaakos commented Dec 3, 2018

[outline] Use the `DocumentSymbol.detail` not just only the `Document… #3314

[outline] Use the `DocumentSymbol.detail` not just only the `Document… #3314

vrubezhny commented Oct 29, 2018 •

edited

Loading

tsmaeder Nov 28, 2018 •

edited

Loading

vrubezhny commented Nov 28, 2018 •

edited

Loading