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

Additions to Tree view API #52300

Closed
wants to merge 6 commits into from
Closed

Additions to Tree view API #52300

wants to merge 6 commits into from

Conversation

sandy081
Copy link
Member

@sandy081 sandy081 commented Jun 19, 2018

@sandy081 sandy081 requested a review from jrieken June 19, 2018 09:40
@sandy081 sandy081 added api tree-views Extension tree view issues labels Jun 19, 2018
@sandy081 sandy081 added this to the June 2018 milestone Jun 19, 2018
/**
* The [command](#Command) that should run when the user double clicks on the tree item.
*/
doubleClickCommand?: Command;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually don't mention user-gestures like click, doubleClick but we use terms that describe their meaning... Something like alternativeCommand etc. Unsure about a better name... What is this being used for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I am aware that we do not mention user-gestures. But here I did not find a better name. Usage is pretty simple and from its name, this command gets executed when user double clicks on a node.

Just to note, even though it is not in API, we talk about single click / double click behaviour in one of our settings workbench.settings.openMode. Thought this will align with that.

Copy link
Member

@jrieken jrieken Jun 20, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, settings name are the wild-wild-west ;-) What I was asking is why we are adding this and how is this being used? I would find a it rather weird that double vs single clicking an item is different. Since you brought it up, openMode is now being ignored for contributed trees, right? Is that something we want?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was asking is why we are adding this and how is this being used?

One of the use cases is that in a script view, run script on double click and not on single click. Another use case is to pin a document after opening it by double clicking from tree view.

openMode is now being ignored for contributed trees, right? Is that something we want?

I do not know but looks like it is being respected. But I do not want that behaviour in custom trees.

In general, Single click selects or highlights an element and whereas double click executes the function associated to that element. Currently command property provides a function that should be executed on select. We need a placeholder for a property that provides a function associated to the given element. command looks to me the right word for that but it is already taken.

It seems Single clicking is usually a primary action of the mouse we can call double click command as secondaryCommand ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I do not want that behaviour in custom trees.

That's something for the UX call I'd say.. Personally, I like when they look and feel alike.

In general, Single click selects or highlights an element and whereas double click executes the function associated to that element.

Not sure so about that... Clicking an element in the explorer always opens it, double clicking it also opens it but pins it as well. That reminds me of the alternative menu item which are meant to be a similar but slightly different action. So, maybe call it altCommand or alternativeCommand.

What would really confuse me as a user would be a tree that doesn't do anything on single click but that does do something on double click. So, maybe we should gear the API towards that not being possible. something like { command: Command | { cmd: Command, alt: Command }}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's something for the UX call I'd say.. Personally, I like when they look and feel alike.

Custom trees cannot inherit generic behaviour because each node behaves differently. For example, if a node has both command and altCommand and user has openMode configured to doubleClick then what is expected behaviour? I would say in this case commands take the precedence over setting and execute command on select (single click) and altCommand on doubleClick is expected.

So, maybe we should gear the API towards that not being possible. something like { command: Command | { cmd: Command, alt: Command }}

I like it 👍

/**
* `true` if the [tree view](#TreeView) is visible otherwise `false`.
*/
readonly visible: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only means visible and not focused, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

/**
* Selected elements.
*/
selection: T[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add readonly and ReadonlyArray<T>?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was there in last milestone. Hope the change will not break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just readonly should be enough and non breaking

@sandy081
Copy link
Member Author

@jrieken Updated the PR with requested changes and removed double click command

@sandy081 sandy081 closed this Jun 25, 2018
@sandy081 sandy081 deleted the sandy081/treeView branch July 12, 2018 07:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api tree-views Extension tree view issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants