Allow for [Completion|Symbol]Kind modifiers

[jrieken]

extracted from #2628

In completions and for symbols we allow to provide a kind which is mostly use to derive an icon. For instance vscode.CompletionItemKind.Method. This issue is about allowing to modify these kinds to expressing that something is a "private member" or an "abstract protected method"

The idea is that we don't add more and more kinds, e.g no CompletionItemKind.PrivateMethod but a second type, a kind-modifier, that works in combination with a kind.

Potential candidates are: public, protected, internal, package-private, friend, private, static, final, const, abstract, volatile, transient, ... A "n languages have this"-rule should be applied when picking candidates.

[jrieken]

cc @kieferrm

[jrieken]

More modifier candidates are: deprecated, experimental

[sean-mcmanus]

For C/C++, we would like "declaration", "private", and "protected".

[jrieken]

Different options

1. Instead of adding a new enum, add special members to the existing CompletionItemKind enum, e.g ModifierReadonly, ModifierStatic etc. Then use everything as bit mask, e.g. kind: CompletionItemKind.Function | CompletionItemKind.ModifierDeprecated | ModifierReadonly`
2. Add a new enum, e.g. CompletionItemKindModifier, and redefine the kind-property to be kind?: CompletionItemKind | { base: CompletionItemKind, modifier?: CompletionItemKindModifier }. That ensures a modifier never occurs without base kind.

I am in favour of option 2 but I am unsure if we should have two XYZKindModifier enums CompletionItemKindModifier and SymbolKindModifier or one. I don't like the former but I feel it's the correct way of doing it...

Adding @mjbvz for feedback

[mjbvz]

Yes I prefer option 2 as well.

Here's what some other projects do:

• Typescript — Has a kind and then a list of kindModifiers (as a space separated string)
• Rosyln — I believe it just uses a flat list of kinds: http://source.roslyn.io/#Microsoft.CodeAnalysis.Workspaces/Tags/WellKnownTags.cs,65cd20bb5138f97e

From an api point of view, I like the TypeScript approach that uses an array of modifiers:

kind?: CompletionItemKind | { base: CompletionItemKind, modifiers?: readonly CompletionItemKindModifier[] }

That way an extension could mark a completion as readonly and static or as optional and external for example. However it would make the ux design tricky as there would be even more combinations kinds and kind modifiers

[sean-mcmanus]

The result our C/C++ extension wants is just the ability to have our completion/symbol icons match VS:
Completion has a max of 1 modifier:

Symbols has a max of 2 modifiers:

Looking at C#, they have an additional "internal" icon modifier too (the "protected internal" and "private protected" cases are not given unique icons). Only 1 modifier max.

From a UI design/implementation perspective, it seems like the max icon modifiers would be around 3, with the base icon image rendered in the top left (bigger than the modifiers), and potential modifiers rendered on the bottom left, bottom right, and top right. Unless modifiers could change rendering of the base icon in other ways, such as color modulation or a transparent overlay over the entire base icon (e.g. "strikeout").

Should the API reflect the intended UI effect?

An AccessModifierKind that renders in the bottom right and a DefinitionModifierKind that renders in the bottom left would work for us. An OtherModifierKind could go in the top right -- not sure what languages would use that though. Maybe an OverlayModifierKind for a strikeout effect and a ColorModifierKind for making icons partially transparent?

Maybe an API that just gave more control over the icon rendering to extensions would make more people happy instead of having the API be so abstract with just "kinds" that are detached from the actual results.

[jrieken]

I like the TypeScript approach that uses an array of modifiers:

Hm, I was thinking about a bitmask but an array might be JavaScript-like. This also compares to DiagnosticsTag-arrays.

An AccessModifierKind that renders in the bottom right and a DefinitionModifierKind that renders in the bottom left would work for us. An OtherModifierKind could go in the top right

@sean-mcmanus I don't think we wanna be the fine grained. Generally, we have to walk the fine line of finding a good enough abstraction that describes the our UX and that still fits most programming languages.

[jrieken]

Current proposal (due to TypeScript gymnastics the name is kind2 in the implementation)

	export enum CompletionItemKindModifier {
		Deprecated = 1
	}

	export interface CompletionItem {

		/**
		 *
		 */
		kind?: CompletionItemKind | { base: CompletionItemKind, modifier: ReadonlyArray<CompletionItemKindModifier> };
	}

[jrieken]

Actually, instead of "modifier" we could use "tags". That term is less overloaded when it comes to programming lingo (and it also matches DiagnosticsTag). So, have CompletionItemKindTag and SymbolItemKindTag

@mjbvz Preferences? Likes?

[mjbvz]

I like “tags”. It’s more open ended.

If we call it “tags” does it still make sense to put this as a property on “kind”? Or should it be it’s own field?

[jrieken]

If we call it “tags” does it still make sense to put this as a property on “kind”? Or should it be it’s own field?

Good question... I have choosen the current approach so that we don't have tags without (base) kinds. E.g an item could only be tagged as readonly... Internally the API is two field, but also because kind isn't optional. It might be OK as we have a default kind but it's also a little weird...

[jrieken]

It's not like this

export enum CompletionItemKindTag {
  Deprecated = 1
}

export interface CompletionItem {
  kind?: CompletionItemKind | { kind: CompletionItemKind, tags: ReadonlyArray<CompletionItemKindTag> };
}

The alternative, with tags as it own field would be this

export enum CompletionItemTag {
  Deprecated = 1
}

export interface CompletionItem {
  kind?: CompletionItemKind;
  tags?: ReadonlyArray<CompletionItemTag>
}

[octref]

Prefer the alternative, kind.kind would be weird. The tags name is a bit unspecific, but we have Diagnostic#tags as well so we might as well stay consistent with it.

Good question... I have choosen the current approach so that we don't have tags without (base) kinds. E.g an item could only be tagged as readonly... Internally the API is two field, but also because kind isn't optional. It might be OK as we have a default kind but it's also a little weird...

I feel even if you can't makeup your mind to provide a kind you should be able to mark an item as deprecated.

[jrieken]

Went with the kind approach and I actually quite like it. I thinks its flexible and expressive enough. These are all the proposed changes to cover suggestion, outline, and workspace symbols

	export enum SymbolTag {
		Deprecated = 1
	}

	export interface SymbolInformation {
		/**
		 *
		 */
		tags?: ReadonlyArray<SymbolTag>;
	}

	export interface DocumentSymbol {
		/**
		 *
		 */
		tags?: ReadonlyArray<SymbolTag>;
	}

	export enum CompletionItemTag {
		Deprecated = 1
	}

	export interface CompletionItem {

		/**
		 *
		 */
		tags?: ReadonlyArray<CompletionItemTag>;
	}

[jrieken]

Moving to September for finalisation

[jrieken]

Verifier: use the vscode-dts-utility to use the latest version of vscode.d.ts and/or vscode.proposed.d.ts