Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for [Completion|Symbol]Kind modifiers #23927

Closed
jrieken opened this issue Apr 5, 2017 · 16 comments
Closed

Allow for [Completion|Symbol]Kind modifiers #23927

jrieken opened this issue Apr 5, 2017 · 16 comments
Assignees
Labels
api api-finalization feature-request Request for new features or functionality verified Verification succeeded

Comments

@jrieken
Copy link
Member

jrieken commented Apr 5, 2017

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
Copy link
Member Author

jrieken commented May 9, 2018

cc @kieferrm

@jrieken
Copy link
Member Author

jrieken commented May 9, 2018

More modifier candidates are: deprecated, experimental

@sean-mcmanus
Copy link
Contributor

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

@jrieken
Copy link
Member Author

jrieken commented Aug 21, 2019

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
Copy link
Contributor

mjbvz commented Aug 21, 2019

Yes I prefer option 2 as well.

Here's what some other projects do:

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
Copy link
Contributor

sean-mcmanus commented Aug 21, 2019

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:
image

Symbols has a max of 2 modifiers:
image

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
Copy link
Member Author

jrieken commented Aug 22, 2019

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
Copy link
Member Author

jrieken commented Aug 22, 2019

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
Copy link
Member Author

jrieken commented Aug 22, 2019

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
Copy link
Contributor

mjbvz commented Aug 22, 2019

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
Copy link
Member Author

jrieken commented Aug 23, 2019

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
Copy link
Member Author

jrieken commented Aug 23, 2019

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
Copy link
Contributor

octref commented Aug 23, 2019

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
Copy link
Member Author

jrieken commented Aug 26, 2019

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
Copy link
Member Author

jrieken commented Aug 26, 2019

Moving to September for finalisation

@jrieken
Copy link
Member Author

jrieken commented Sep 30, 2019

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

@aeschli aeschli added verified Verification succeeded and removed verification-needed Verification of issue is requested labels Oct 3, 2019
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-finalization feature-request Request for new features or functionality verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants