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 programmatically selecting a TreeItem in custom views #30288

Closed
Gama11 opened this issue Jul 8, 2017 · 38 comments
Closed

Allow programmatically selecting a TreeItem in custom views #30288

Gama11 opened this issue Jul 8, 2017 · 38 comments
Assignees
Labels
api api-proposal feature-request Request for new features or functionality tree-views Extension tree view issues verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@Gama11
Copy link
Contributor

Gama11 commented Jul 8, 2017

I've been trying to mimick the files.autoReveal behavior of the regular explorer in the Dependency Explorer of the Haxe extension. I can expand the necessary nodes just fine, but I don't think there's any way to scroll to the correct position in the view + highlight the node programmatically right now.

From what I can tell, the currently planned enhancements won't help with this (#27823). Could a function for selecting a node be added to API? I think a lot of other extensions would benefit from this as well, Code Outline comes to mind.

@vscodebot vscodebot bot added the api label Jul 8, 2017
@sandy081 sandy081 added tree-views Extension tree view issues feature-request Request for new features or functionality labels Jul 11, 2017
@sandy081 sandy081 added this to the Backlog milestone Jul 11, 2017
@sailro
Copy link
Member

sailro commented Oct 21, 2017

This is a huge limitation, it would be so great to have such a basic feature when working with custom tree views

@pltrant
Copy link

pltrant commented Jan 28, 2018

Yes, this API would be very useful for code view/navigation extensions. I have an extension with a custom view and it is frequently requested to highlight the node that represents where they are in the file.

@mike-lischke
Copy link

This FR could be tracked by #27823. I have (multi) selection per code in my feature wishlist there.

@sandy081
Copy link
Member

@Gama11 @pltrant Can I assume is this request is to have an API to reveal an item in the tree? Or do you also need select as a separate API?

@sandy081 sandy081 modified the milestones: Backlog, February 2018 Feb 14, 2018
@sandy081
Copy link
Member

I am working on a proposed API for this which will be available for authors to try out in this iteration and the plan to make it available in main API is in next milestone.

@Gama11
Copy link
Contributor Author

Gama11 commented Feb 14, 2018

If reveal implies select, that seems good enough to me.

@sandy081
Copy link
Member

reveal and select are two independent actions.

reveal - Reveals the item in the tree view.
select - Selects the item in the tree. Select will not show the item if it is not visible.

@Gama11
Copy link
Contributor Author

Gama11 commented Feb 14, 2018

In that case, both are needed to mimic the files.autoReveal behavior, as files are also highlighted in the regular file explorer when using that setting.

@pltrant
Copy link

pltrant commented Feb 14, 2018

Agreed with @Gama11. Will the select API also invoke the click command associated with the node? For my use case, I would not want it to do that, but I imagine others could want that, so I suggest there be a parameter to include/exclude that option.

@sandy081
Copy link
Member

I think having just select without revealing does not make sense. So I thought, have a single api reveal(element, select?: boolean) would be much more appropriate.

@pltrant Yes, select will trigger the command. I will think to make it optional or trigger the command only when user did the selection.

@pltrant
Copy link

pltrant commented Feb 14, 2018

A single api for reveal to optionally include select makes the most sense (I agree with not seeing the benefit of just a select without a reveal).

@eamodio
Copy link
Contributor

eamodio commented Feb 14, 2018

@sandy081 would using reveal(element, true) also focus the newly selected element?

Probably best as another api, but it would be great to be able to just focus the custom view itself (and whatever element was last selected in it)

@mike-lischke
Copy link

mike-lischke commented Feb 14, 2018

Selecting w/o also revealing a node does make sense. Imagine a file tree with some collapsed and some expanded nodes and you want to select all nodes for a given file type. Do you really want to make all collapsed node visible, only because they are in the selection set? That's a usability nightmare (and I'm assuming here, multi selection is already in).

@eamodio
Copy link
Contributor

eamodio commented Feb 14, 2018

@mike-lischke Good point, we can't forget about multi-select, which would completely change the api proposed above

@sandy081
Copy link
Member

@eamodio there are multiple variations for reveal like

  • only reveal
  • reveal and select
  • reveal and focus -> Not sure if this makes sense.
  • reveal and select and focus

focus on a view might be helpful as an API that will focus the view itself and the last selected element as you mentioned. But what is the need for focus API on a element?

@mike-lischke Is there any existing real use case to support select api ?

@mike-lischke
Copy link

Focus is not a state of a tree item, but belongs to the treeview. Hence there is no need for focus for a tree item. If the notion of a single item is required you would use treeview focus + single selection.

Regarding select: I don't understand your question. Do you need examples to support the idea of having a selection API for treeview items? I assumed it would be so obvious how important that is.

@eamodio
Copy link
Contributor

eamodio commented Feb 20, 2018

While focus does belong to the view not the item, imo allowing the inclusion of focus (optionally) on selection is valuable. But as long as there is an api to focus the view itself, I don't feel it is critical.

If I want a command in GitLens to open a commit in the view, it is very likely that the use want it to be focused not just selected -- so that keyboarding works. If that has to be 2 apis -- select the item(s), and then focus the view (which would keep the last selected item(s) that's fine (just less discoverable to an extension dev imo).

@sandy081 I think you captured the desired actions and agree that reveal and focus doesn't really make sense -- it would likely be reveal/select/focus in that case.

Although once you think about multi-select, it gets a bit more complex. Would you allow multi-reveal? Maybe. Multi-select, definitely. Mutli-focus, doesn't make sense.

@eamodio
Copy link
Contributor

eamodio commented Feb 20, 2018

I would propose the following apis (don't consider the names just the ideas):

viewFocus() // focuses the view with the last selection preserved -- and keyboard focus should be on the last "active" selected item
viewReveal(item: TreeItem) // reveals the item, doesn't change selection or focus -- can change to an array to support multi-reveal if it makes sense
viewSetItemState(item: TreeItem, state: 'expand' | 'collapse', options: {  reveal: boolean = false }) // Expands or collapses an item, and optionally reveals it
viewSetSelection(selection: TreeItem[], options?: { focus: boolean = true, reveal: boolean = true }) // changes the view selection where the first item in the array is the active one and the one that would be focused/revealed

To be honest, I don't find viewReveal all that compelling, but I'm also probably missing a lot of use-cases

Thoughts?

P.S. I didn't include anything about this above, but would these apis only allow for you to affect your own views (so maybe on TreeDataProvider) or more global to affect other views -- I'm hoping for the latter, but then each of those apis above either need to take a view id or have some other api, that gets you access to some view proxy-like object.

@sandy081
Copy link
Member

sandy081 commented Feb 20, 2018

@mike-lischke It is obvious that every API will have many examples, but our requirement to introduce an API is driven by the extension authors use cases. Hence asked for your scenario for multi select api for your extension.

@eamodio Since focus is for the view and not for the item mixing them too might not be a good idea. If I have select api for item, I would not add focus option to it. I would have focus as a separate api.

There will be a TreeViewer proxy for each tree view, which will have methods to access. I want to provide access to the TreeViewer while registering the data provider. So one idea is to return it while registering or propose a new API as follows. Its still in draft and thinking.

namespace window {
		/**
		 * Create and show a new webview.
		 *
		 * @param title Title of the webview.
		 * @param column Editor column to show the new webview in.
		 * @param options Webview content options.
		 */
		export function createTreeViewer<T>(id: string, options: { dataProvider: TreeDataProvider<T> }): TreeViewer<T>;
	}

	export interface TreeViewer<T> extends Disposable {

		focus(): TPromise<void>

		reveal(element: T, select?: boolean): TPromise<boolean>;

	}

I think selecting an element without revealing it is not useful. It makes sense in multi-select but in single select, I would expect to show it.

I see your request for expand and collapse api. I think this need an issue for itself, Can you please open a separate feature request so that to avoid hijacking this issue.

@sandy081
Copy link
Member

@mike-lischke your feedback is very much appreciated.

You can only access views you register and the API will be use case driven. I am sorry that we cannot expose methods thinking that this can be useful in future. Once API is out, it is hard to change, so we want to gather requirements before making it public.

Yes, revealing a node is expanding nodes, but it is an implementation detail. I see that, requested authors wanted to have expand or select methods so that they can reveal it in the view. Hence reveal is a better and useful api for them to implement their functionality. If authors requires a fine grained api to expand and collapse for some functionality, we will provide it (which would be nice to handle as a separate feature request).

Currently TreeItem is stateless and is just a view representation. Would not like to put state into it. Also currently it is a pull model, meaning, Tree model is owned by VS Code core and it asks the extension for the tree data. So TreeDataProvider can be also stateless and extensions might not know that an item is in the model / loaded or not. Providing methods on TreeItem to update would force both to hold state. Updating TreeItem would be a better approach in case of Push model where extension owns the Tree model. reveal API on the TreeView will allow extensions to just call that method by passing the handle to the item they have and Tree model takes care of expanding the hierarchy and revealing the element in the view.

Promise will be returned to signal the completion of the operation.

@eamodio
Copy link
Contributor

eamodio commented Feb 22, 2018

@sandy081 Back to the reveal api for a moment. Without the expand option, what would the behavior be? If you wanted to select a node in the tree -- would it always expand the node if it had children? If so, I'm not sure that is always desirable -- it would match mouse selection behavior, but not keyboard selection behavior. IMO the safer bet would be to never expand, since the user can do that themselves, but at the same time, I'd love the ability to choose.

For GitLens, I am thinking about some possible commands, that allow a user to jump to the GitLens view and select a node (i.e. the current branch, or current file, etc), and in some of the cases the user might want the node to be selected, focused, and expanded, and others maybe not -- just selected and focused. So I would definitely like to see an expand option available.

We should also consider that expanding a node could have performance impact/implications too.

@sandy081
Copy link
Member

@eamodio By default revealed element will not be expanded.

If you have the use case, I am happy to add that option.

@eamodio
Copy link
Contributor

eamodio commented Feb 22, 2018

I think it would be useful. Here is a specific case. Currently, you can show the history of a branch in quick picks, but I would also like to have a command that would just find/open that branch and expand it in the GitLens view. Also from that same quickpick menu mentioned, I have a Show in Results which basically shows the GitLens Results view and adds that branch to it, with its history expanded. I had to do it that way, because there was no way for me to focus/select/expand the correct node in the GitLens view itself. So with this feature, I could basically rename that to instead of showing in results, it would just focus/select/expand the branch inside the branches node of the GitLens view.

I'm assuming calling reveal on a node that is nested will automatically expand all parents so that it will be visible. Correct?

@mike-lischke
Copy link

mike-lischke commented Feb 23, 2018

I am sorry that we cannot expose methods thinking that this can be useful in future. Once API is out, it is hard to change, so we want to gather requirements before making it public.

On the other hand, this is by far not the first Treeview implementation on this planet and over the decades a set of useful APIs have been defined on many of the previous implementation. It's not that we couldn't come up with a list of properties etc. that would certainly be used in one or the other form. And again: creativity comes from possibilities. If you don't provide certain APIs then users will start doing workarounds and it's pretty hard later to get all the code changed then. No offense intended, but if you only consider APIs that are requrested here and in similar FRs, instead of implementing the common set, it appears pretty short sighted.

Yes, revealing a node is expanding nodes, but it is an implementation detail.

Why using a different name then? Expanding a hiearchy is such a common and well known term, it makes no sense to introduce a new one for the same functionality.

I see that, requested authors wanted to have expand or select methods so that they can reveal it in the view. Hence reveal is a better and useful api for them to implement their functionality.

Hmm, I prefer properties, because I learned over the past 2 decades that these are the cleaner solution. Up to you of course which developer you want to please.

If authors requires a fine grained api to expand and collapse for some functionality, we will provide it (which would be nice to handle as a separate feature request).

There is no fine grained API to implement. Either you have a function on the tree view to expand or collapse a node or you have that on the node itself (could also be a function or a property). The work is the same, it's only a matter of good or bad design.

Currently TreeItem is stateless and is just a view representation. Would not like to put state into it.

That's leaves me speechless. Such an item is all about state. It has a position, it has a selection state, it has a label, it has an expand state, it has a checked state, it has a visible state, it has an image and more. How can you say you don't want state for a node?

Wait, maybe it's a misunderstanding here. You probably mean you don't want state for the vscode internal objects and that's totally fine, if the data model is extension provided and keeps this state instead.

Also currently it is a pull model, meaning, Tree model is owned by VS Code core and it asks the extension for the tree data.

Yes, I saw that and I really wondered why it was implemented that way. It's so pretty unusual and conflicts with simple things like app driven updates. Instead it should be an application provided data source. The model contains everything: hieararchy, nodes + their states, events for notifications etc. The treeview only takes such a model and visualizes that.

So TreeDataProvider can be also stateless and extensions might not know that an item is in the model / loaded or not. Providing methods on TreeItem to update would force both to hold state.

And what's so bad about this? Can you explain why you don't want to store state? How do you want to know what to update? If you have no state, you cannot compare what changed and are doomed to update the entire tree for every little change. The impact on performance will be terrible. And don't be suprised: when vscode doesn't provide state then the extensions will implement that. There's no win by rejecting the obvious needs.

Updating TreeItem would be a better approach in case of Push model where extension owns the Tree model.

And that should be how it is implemented.

reveal API on the TreeView will allow extensions to just call that method by passing the handle to the item they have and Tree model takes care of expanding the hierarchy and revealing the element in the view.

Setting TreeItem.expanded = true; allows extensions to just set a property to get a node expanded and the TreeView (not the model!) takes care to expand the hierarchy. Please think of the individual roles of models and views.

Promise will be returned to signal the completion of the operation.

I'd rather prefer to make the data source (model) be an extension provided object that can send out events on various occasions, which are data related. The treeview can do the same for view releated operations (like, when it is done loading the content etc.). And they should coalesce notifications, so that extensions can update hundreds of tree items without refreshing the display a hundred times.

sandy081 added a commit that referenced this issue Feb 23, 2018
@eamodio
Copy link
Contributor

eamodio commented Feb 24, 2018

Here is a specific feature request for GitLens: gitkraken/vscode-gitlens#275

I would like to use the expand option on this API to make this happen. But I would also really like the expand option to be more than just a boolean -- maybe an enum of 'item' | 'descendants'. The idea is to be able to expand just 1 level, or deeper.

So in the previous branch expand example, I gave, I would use item, but to fulfill feature request gitkraken/vscode-gitlens#275 I would use descendants.

Another option would be to keep the expand: boolean, but add an options expandLevel: number (or something like that) where it would control how many levels down would be expanded. Would need to support something like -1 or some magic value to mean go all the way.

Or have expand: boolean | number -- if true it expands itself, if a number it would expand to that level (still would need a magic value for all the way)

@eamodio
Copy link
Contributor

eamodio commented Mar 1, 2018

@sandy081 will this be making the Feb cut? (seems unlikely at this point)

@sandy081 sandy081 modified the milestones: February 2018, March 2018 Mar 2, 2018
@sandy081
Copy link
Member

sandy081 commented Mar 2, 2018

@mike-lischke Sorry for late reply. I was bit busy this week with our release stuff.

Yes, there are many Tree APIs in the market and they might share some common things and differ in other. We have our own set of protocols to follow before coming up with an API.

Naming is always tough. We do multiple rounds of feedback and come up with proper name. In this case the extension author wants to reveal a node to the user which makes it obvious to call it reveal. On the other hand expand does not tell about showing to the user.

properties vs methods and other arguments of yours boils down to the design approach. Our design is pull model that extensions provide to the system, data sources and rendering (like label) providers and system asks for the data and render the view. This is one of the familiar designs for Tree implementation for eg., eclipse JFace Tree view has the similar API: https://www.eclipse.org/articles/Article-TreeViewer/TreeViewerArticle.htm

@sandy081
Copy link
Member

sandy081 commented Mar 2, 2018

@eamodio Re gitkraken/vscode-gitlens#275 If I understand correctly, you want the nodes to be auto expanded when view loads them? If so, Is not the collapsibleState property on TreeItem doing it?
Reveal API is different that it allows you to show a node at any time.

will this be making the Feb cut? (seems unlikely at this point)?

It is part of proposed API for Feb release and becomes public for March release.

Try it out and provide feedback.

@eamodio
Copy link
Contributor

eamodio commented Mar 2, 2018

@sandy081 the view will already be loaded and not expanded. I want to be able to have a command to select and expand the item (sometimes 1 level others all descendants).

@sandy081
Copy link
Member

sandy081 commented Mar 2, 2018

@eamodio Ah ok. So what is the context when this command is invoked in your case?

@eamodio
Copy link
Contributor

eamodio commented Mar 2, 2018

There are multiple scenarios.

  1. A command (from the palette or keyboard shortcut) that just focuses the GitLens/GitLens Results views (so any previous selection would be preserved)
  2. A command (from a quickpick or keyboard shortcut) that would select a branch, and maybe expand it 1 level, or a commit — basically jumping into the GitLens explorer
  3. A command on a node that would expand it and all its children for both the GitLens and GitLens Results views
  4. A command when you execute it on a node in the GitLens explorer, would select and expand a node in the GitLens Results view

And those are just a few off the top of my head. I want to make jumping into and around the GitLens views more seamless.

@sandy081
Copy link
Member

sandy081 commented Mar 19, 2018

Current status: Under discussion about how to expose views in the API - #45994

@sandy081
Copy link
Member

Verify that you can use the new TreeView.reveal API without having to run the extension in proposed API mode.

@sandy081 sandy081 added the verification-needed Verification of issue is requested label Mar 27, 2018
@chrmarti chrmarti added the verified Verification succeeded label Mar 29, 2018
@vscodebot vscodebot bot locked and limited conversation to collaborators May 11, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api api-proposal feature-request Request for new features or functionality tree-views Extension tree view issues verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

8 participants