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

API for hierarchical document symbol data #34968

Closed
jrieken opened this Issue Sep 25, 2017 · 29 comments

Comments

@jrieken
Member

jrieken commented Sep 25, 2017

In order to support a real outline view (#5605) and a breadcrumb bar (#9418, #31162) we need better document symbol information. Today, the API doesn't allow providers to return tree data and it also doesn't spec what the ranges of symbols should be. Some language extensions make the range be the whole symbol range, some make it the identifier range. It needs a new or refined API to support hierarchies, I see two options

  • allow a tree of SymbolInformation-objects, e.g. add an optional children-property.
  • have two range-objects on SymbolInformation-objects, one being the whole range and one being the identifier range. While this looks counter intuitive and worst than having tree-style-data it makes merging of different data sets from different providers easy.
@jrieken

This comment has been minimized.

Member

jrieken commented Jan 5, 2018

@Leedehai

This comment has been minimized.

Leedehai commented Apr 14, 2018

Indeed needed..

@jrieken jrieken added this to the April 2018 milestone Apr 17, 2018

@jrieken

This comment has been minimized.

Member

jrieken commented Apr 17, 2018

We want to define, maybe propose, the API this milestone and build a UI for this in May.

@jrieken

This comment has been minimized.

Member

jrieken commented Apr 19, 2018

Proposal is to have a new type like DocumentSymbolInformation which has children and a symbol range. A DocumentSymbolProvider is free to return the new type, or old SymbolInformation objects. In the latter case, we cannot render a tree and because of that providers should signal what they return upon registration.

@eamodio

This comment has been minimized.

Contributor

eamodio commented Apr 21, 2018

FYI, this is also related to #11587

@jrieken

This comment has been minimized.

Member

jrieken commented Apr 23, 2018

First cut of the API proposal

export class HierarchicalSymbolInformation {
  name: string;
  kind: SymbolKind;
  location: Location; // use location#range to point to the 'identifier' or 'goto' position
  range: Range; // full symbol range as opposed
  children: HierarchicalSymbolInformation[]; // child items

  constructor(name: string, kind: SymbolKind, location: Location, range: Range);
}

export interface DocumentSymbolProvider {
  provideDocumentSymbols(document: TextDocument, token: CancellationToken): ProviderResult<HierarchicalSymbolInformation | SymbolInformation[]>;
}
@patrys

This comment has been minimized.

patrys commented May 16, 2018

I'm the author of the Code Outline extension. Here's my take on this:

allow a tree of SymbolInformation-objects, e.g. add an optional children-property.

This could work nicely for languages where methods can be defined outside of the class body or where symbols belong to namespaces independent of the order of definition. However...

have two range-objects on SymbolInformation-objects, one being the whole range and one being the identifier range. While this looks counter intuitive and worst than having tree-style-data it makes merging of different data sets from different providers easy.

This approach would work better if you're ever planning to incorporate other sources of regions such as the folding provider (I'm planning to show folding regions as containers once executeFoldingRegionProvider becomes available as a command).

@jrieken

This comment has been minimized.

Member

jrieken commented May 17, 2018

An alternative would be a Hierarchy<T>-type which defines a hierarchy but doesn't interfere with the business object. Something like this:

export class Hierarchy<T> {
	parent: T;
	children: Hierarchy<T>[];
	constructor(element: T);
}

export class SymbolInformation {
	detail: string;
	range: Range;
	//more...
}

export interface DocumentSymbolProvider {
	provideDocumentSymbols(
		document: TextDocument, 
		token: CancellationToken
	): ProviderResult<SymbolInformation[] | Hierarchy<SymbolInformation>[]>;
}

jrieken added a commit that referenced this issue May 17, 2018

@jrieken jrieken modified the milestones: May 2018, June 2018 May 29, 2018

@DanTup

This comment has been minimized.

DanTup commented Jun 12, 2018

@patrys I think you've misunderstood what I said. I'm not talking about grouping things like imports; imports shouldn't even appear in the outline tree. My comment is about providing data for rendering in the outline tree that is not shown in the flat document symbol list (because when not rendered in a tree it's less useful).

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 12, 2018

Maybe not directly related, but wouldn't it make sense that we specify a way to provide additional symbol information (like arguments of a function type symbol), so they can be rendered in the outline tree later on?

@mofux Yeah, an earlier version had a detail-property and I am open for adding it back. Not having it ensures a slick/clean UI which is something we always strive for. Tho we could implement the UI so that we only show the detail when an item is selected

@DanTup

This comment has been minimized.

DanTup commented Jun 12, 2018

Not having it ensures a slick/clean UI which is something we always strive for.

I think not having it could encourage people to just add it to the name.. I'd already done that in Dart because it seemed useful. If it's a separate field at least it could be rendered more subtle.

@mofux

This comment has been minimized.

mofux commented Jun 12, 2018

@jrieken Thanks for your thoughts. What shape would this detail property have? As said before, if there was a reliable way to get the hover message of the DocumentSymbol (maybe by its gotoRange) that would be even better as we can keep the DocumentSymbol clean and simple.

Another thought:
From a design perspective, it would IMHO make more sense to have the DocumentSymbol carry a defined set of extra information (return type, arguments etc...), and then have the HoverProvider re-use that information to create the hover label out of that.

@eamodio

This comment has been minimized.

Contributor

eamodio commented Jun 12, 2018

Given that the DocumentSymbol has a children property, maybe the parameters of a function/method could be added as children of it with a new SymbolKind.Parameter or something like that?

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 13, 2018

I have added a simple detail: string property (like CompletionItem#detail) and we will render it next to the names of symbol, likely only when selected. I have stayed away from spec'ing things more precisely because we usually try to avoid assigning semantics, e.g. this is a param, this is var-args, etc, but we like to design the API according to our UX needs.

Wrt hovers: We are thinking to simply invoke the existing HoverProviders and to reuse that information. We need for special API - all information needed for that request (document & position) is already there.

@mofux

This comment has been minimized.

mofux commented Jun 13, 2018

Wrt hovers: We are thinking to simply invoke the existing HoverProviders and to reuse that information. We need for special API - all information needed for that request (document & position) is already there.

I'm afraid it isn't that simple, consider these cases that show the hover positions required to get the correct function signature:

// have to hover "funcA"
function funcA(a, b) {}

// have to hover "funcB"
const funcB = (a, b) => {}

// have to hover "function" keyword
this.funcC = function(a, b) {}

// no hover position can reveal the function signature
this.funcD = (a, b) => {}
@jrieken

This comment has been minimized.

Member

jrieken commented Jun 13, 2018

I guess it would use the gotoRange so giving the extension control over this. Not sure about the last two samples and how often you use this to refer to the global but as soon as you have inside a function or class it works correctly.

@mattacosta

This comment has been minimized.

Contributor

mattacosta commented Jun 13, 2018

Maybe not directly related, but wouldn't it make sense that we specify a way to provide additional symbol information (like arguments of a function type symbol), so they can be rendered in the outline tree later on?

Something else that comes to mind was an editor that I used in the past which had green/yellow/red icons (with static variants) next to public/protected/private methods and properties. It was a quick way to check member ordering without having to run something like tslint.

@KamasamaK

This comment has been minimized.

KamasamaK commented Jun 13, 2018

The ability to have modifiers on kind is proposed in #23927.

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 14, 2018

After discussing this today we wanna make two ranges fullRange becomes range and gotoRange will become selectionRange.

@pltrant

This comment has been minimized.

pltrant commented Jun 15, 2018

Is it going to be possible for extensions to contribute configurationDefaults for the Outline (to show and to set follow cursor and sort type)? It would seem logical since we can do that for things like the minimap. It's beneficial for creating a curated experience with certain specialized extensions (and users can always override said defaults).

@jrieken

This comment has been minimized.

Member

jrieken commented Jun 18, 2018

@pltrant This issue is about the underlying API, not configuration

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.