Sash double clicks #4702

mquandalle · 2016-03-27T22:02:08Z

Start implementing #4660.

The provided code defines a new event that is fired when a “stash” is double-clicked, and two usages of it—the first one on the spitted editor frontiers, and the second one on the bottom panel border. It is easier to review this PR commit per commit.

The capture below demonstrates the feature on the spitted editor:

I may want in implement the “sidebar optimal sizing” that originally motivated #4660, but I would like to have some guidance on the code structure:

Is IViewletView the right place to implement an optional getOptimalWidth method? [edit: I’ve chosen IViewlet instead]
What is the best way to perform the HTML elements width measurement? [edit: I’ve found DOM.getTotalWidth and DOM.getRelativeLeft]
How do I get the current viewlet in vs/workbench/layout.ts? [edit: the viewlet service]
Where and how should I write some tests?

Thank you in advance for considering this contribution :-)

msftclas · 2016-03-27T22:02:11Z

Hi @mquandalle, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. I promise there's no faxing. https://cla.microsoft.com.

TTYL, MSBOT;

msftclas · 2016-03-27T22:05:43Z

@mquandalle, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, MSBOT;

bpasero · 2016-03-29T05:12:55Z

@mquandalle I would just suggest to make the side bar size to its minimal width. Since we never show horizontal scrollbars, every size is good enough to show the contents within. Minimal width is defined in https://github.com/Microsoft/vscode/blob/master/src/vs/workbench/browser/layout.ts#L20

mquandalle · 2016-03-29T10:27:04Z

That would be pretty easy but if you look at Sublime and Atom they both have a smarter implementation that measures the width of the explorer items and uses the larger one as the sidebar width. Do the test! For example if you have an opened sidebar at its “optimal width” and you open some folder that contains longer file names, double clicking on the sidebar border will re-calculate a larger width. As described in #4660 I’d like to have the same behavior on VSCode.

So the idea was to use a static width (like indeed DEFAULT_MIN_PART_WIDTH) on the search and debugger viewlets, and let the file-explorer and the git-commiter viewlets implement a method that returns their optimal width.

bpasero · 2016-03-29T10:35:39Z

@mquandalle I see, there is currently no API to compute the optimal width of the explorer tree to prevent truncating of words, but maybe @joaomoreno could chime in on that topic.

joaomoreno · 2016-03-29T11:00:55Z

Renamed stash to sash in the title.

@bpasero @mquandalle It can be done, but it is non-trivial.

mquandalle · 2016-03-29T15:26:51Z

I got a first version of explorer optimal resizing working in 9da61e8.

bpasero · 2016-03-29T16:40:37Z

@mquandalle cool, I think it would make somewhat more sense that every viewlet can implement a function to provide its optimal width. That way, the double click works with all viewlets. If a viewlet decides to not provide optimal width, it should just fallback to the minimal view width as indicated before.

I think you also need to take into account some other aspects: The working files view to the top also has labels that might get truncated. However, I would not expect that the full path of a working file gets revealed, only its label 👍

mquandalle · 2016-03-29T16:54:06Z

I think it would make somewhat more sense that every viewlet can implement a function to provide its optimal width. That way, the double click works with all viewlets. If a viewlet decides to not provide optimal width, it should just fallback to the minimal view width as indicated before.

Yes, that’s exactly how I implemented it.

I think you also need to take into account some other aspects: The working files view to the top also has labels that might get truncated. However, I would not expect that the full path of a working file gets revealed, only its label

Sure, currently I only take into account the explorer tree width but adding the working view as you say makes total sense.

bpasero · 2016-03-30T05:23:56Z

src/vs/base/browser/ui/sash/sash.css

+}
+
+.horizontal-cursor-container * {
+	cursor: ns-resize !important;


Why is this needed?

bpasero · 2016-03-30T05:26:31Z

I added some comments to the code

mquandalle · 2016-04-04T22:52:44Z

Also note that the current DOM measurement strategy has two important drawbacks:

getOptimalWidth() doesn’t work if the sidebar is closed, ie you can’t double click on the closed sash to open the sidebar to its optimal width, which is expected;
if the explorer list is too long, VS renderer will break the list into multiple “tiles” that won’t be displayed concurrently, thus getOptimalWidth() will only consider visible elements and not the entire list.

For comparison both Atom and Sublime handle (2) correctly and don’t have problem (1) as they don’t allow sidebar opening using the mouse.

bpasero · 2016-04-08T06:43:41Z

@mquandalle thanks, can you resolve the merge conflicts in this PR? I will have a look soon and provide feedback.

bpasero · 2016-04-09T14:57:08Z

@mquandalle actually much easier to review your change without merging master back in. just let it be :)

bpasero · 2016-04-09T16:06:12Z

@mquandalle getting there, works very nice now. Some more feedback:

You can manage to move getOptimalWidth() into IViewlet by casting to IViewlet in the sidebarPart.ts#getActiveViewlet() method: return <IViewlet>this.getActiveComposite();
This should fix all the compile errors and you can do the move out of IComposite into IViewlet and related implementations. The default implementation (returning null) can then go into the abstract Viewlet class.
Be careful in explorerViewlet#getOptimalWidth(). you expect the explorer view to be there, but if you don't have a folder open, explorer view is of kind EmptyView and does not have any optimal width.
the implementation of getOptimalWidth() seems very duplicate between explorerView and workingFilesView. I suggest to create a new method in DOM where you pass in the class name of the container you want to measure.

mquandalle · 2016-04-09T16:18:18Z

Thank you for the feedback, I should be able to amend the PR soon. Do you have any comment on #4702 (comment)?

bpasero · 2016-04-10T08:19:56Z

@mquandalle sure:

I wonder if double click on the hidden sidebar should actually be a no-op? otherwise I am also fine to leave it as is, though I wonder if this is a use case in common scenarios.
that is true and due to the fact that the tree is fully virtual and the DOM only contains what you see. I think that this is OK as is.

We had to implement a slight change in the sash drag "hack", by removing the full cover transparent overlay (that was preventing clicks events from being fired) and replacing it by a CSS `cursor` rule on the document.body. The ideal solution for a clean dragging events implementation would be to use the `Element.setCapture` API that is unfortunately not available in Chrome.

The calculus was plain wrong as showed in winjs/winjs#1621 This was probably unotified since this utility function wasn't used in the code base.

Fixes microsoft#4660

mquandalle · 2016-04-14T13:53:32Z

I rebased my branch on master` to solves the (trivial) merge conflict. I also added a commit to address your #4702 (comment) points.

Points 1 and 3 are fixed, but I have a doubt about 2. You say that I’m not handling the case when explorerViewlet#getExplorerView() returns EmptyView but the return type of this function is ExplorerView and not ExplorerView | EmptyView. Should I change that?

bpasero · 2016-04-15T06:10:46Z

@mquandalle changes look good! you are right, getExplorerView() will return only the explorer view, but it can return null in case you did not open a folder (just try to File > Close Folder and doubleclick on the explorer sash).

mquandalle · 2016-04-15T07:13:21Z

Ok, it's fixed.

This commit also moves the `getOptimalWidth` method from the `Composite` to the `Viewlet` component. This partially addresses microsoft#4702 (comment)

bpasero · 2016-04-15T09:31:05Z

Thanks so much, merged to master!

* Support double click on sashes We had to implement a slight change in the sash drag "hack", by removing the full cover transparent overlay (that was preventing clicks events from being fired) and replacing it by a CSS `cursor` rule on the document.body. The ideal solution for a clean dragging events implementation would be to use the `Element.setCapture` API that is unfortunately not available in Chrome. * Center the split editor frontier by double-clicking on it * Reset the bottom panel size by double-clicking on it * Implement sash reset on diff editors * Fix a bug with DOM measurement The calculus was plain wrong as showed in winjs/winjs#1621 This was probably unotified since this utility function wasn't used in the code base. * Implement sidebar sash optimal resizing Fixes #4660 * Abstract `getLargestChildWidth` into a DOM method This commit also moves the `getOptimalWidth` method from the `Composite` to the `Viewlet` component. This partially addresses #4702 (comment) * Calculate the sidebar optimal width correctly in case no folder is open

Tyriar · 2016-04-15T18:11:18Z

Thanks @mquandalle, I've been wanting this for a while! 🎆

msftclas added the cla-required label Mar 27, 2016

msftclas added cla-signed and removed cla-required labels Mar 27, 2016

Tyriar assigned bpasero Mar 28, 2016

joaomoreno changed the title ~~Stash double clicks~~ Sash double clicks Mar 29, 2016

mquandalle changed the title ~~Sash double clicks~~ [WIP] Sash double clicks Mar 29, 2016

mquandalle force-pushed the stash-dblclick branch from 56a86a7 to 9da61e8 Compare March 29, 2016 15:24

bpasero reviewed Mar 30, 2016
View reviewed changes

src/vs/base/browser/ui/sash/sash.css

}

.horizontal-cursor-container * {

cursor: ns-resize !important;

Copy link

Member

bpasero Mar 30, 2016

Choose a reason for hiding this comment

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

Why is this needed?

bpasero added this to the April 2016 milestone Mar 30, 2016

mquandalle force-pushed the stash-dblclick branch 6 times, most recently from a4fa580 to 76a8ce0 Compare April 1, 2016 16:42

mquandalle added 6 commits April 14, 2016 15:05

Center the split editor frontier by double-clicking on it

451730a

Implement sash reset on diff editors

78acad8

Reset the bottom panel size by double-clicking on it

c6a47a9

Fix a bug with DOM measurement

868e988

The calculus was plain wrong as showed in winjs/winjs#1621 This was probably unotified since this utility function wasn't used in the code base.

Implement sidebar sash optimal resizing

af43674

Fixes microsoft#4660

mquandalle force-pushed the stash-dblclick branch from 28cd1ad to 0fb5a80 Compare April 14, 2016 13:49

mquandalle force-pushed the stash-dblclick branch from 0fb5a80 to 6b207f2 Compare April 15, 2016 07:10

mquandalle added 2 commits April 15, 2016 09:42

Abstract getLargestChildWidth into a DOM method

b6036c7

This commit also moves the `getOptimalWidth` method from the `Composite` to the `Viewlet` component. This partially addresses microsoft#4702 (comment)

Calculate the sidebar optimal width correctly in case no folder is open

bfe45ae

mquandalle force-pushed the stash-dblclick branch from 6b207f2 to bfe45ae Compare April 15, 2016 07:42

bpasero merged commit 65bdaf3 into microsoft:master Apr 15, 2016

mquandalle mentioned this pull request Apr 15, 2016

Improve sidebar optimal resizing #5320

Closed

2 tasks

mquandalle mentioned this pull request May 3, 2016

April Iteration Plan #4888

Closed

68 tasks

mquandalle mentioned this pull request Mar 2, 2017

Pointer capture when moving sashes or scrollbars #21675

Closed

github-actions bot locked and limited conversation to collaborators Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sash double clicks #4702

Sash double clicks #4702

mquandalle commented Mar 27, 2016

msftclas commented Mar 27, 2016

msftclas commented Mar 27, 2016

bpasero commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero commented Mar 29, 2016

joaomoreno commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero Mar 30, 2016

bpasero commented Mar 30, 2016

mquandalle commented Apr 4, 2016

bpasero commented Apr 8, 2016

bpasero commented Apr 9, 2016

bpasero commented Apr 9, 2016

mquandalle commented Apr 9, 2016

bpasero commented Apr 10, 2016

mquandalle commented Apr 14, 2016 •

edited

Loading

bpasero commented Apr 15, 2016

mquandalle commented Apr 15, 2016

bpasero commented Apr 15, 2016

Tyriar commented Apr 15, 2016

Sash double clicks #4702

Sash double clicks #4702

Conversation

mquandalle commented Mar 27, 2016

msftclas commented Mar 27, 2016

msftclas commented Mar 27, 2016

bpasero commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero commented Mar 29, 2016

joaomoreno commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero commented Mar 29, 2016

mquandalle commented Mar 29, 2016

bpasero Mar 30, 2016

Choose a reason for hiding this comment

bpasero commented Mar 30, 2016

mquandalle commented Apr 4, 2016

bpasero commented Apr 8, 2016

bpasero commented Apr 9, 2016

bpasero commented Apr 9, 2016

mquandalle commented Apr 9, 2016

bpasero commented Apr 10, 2016

mquandalle commented Apr 14, 2016 • edited Loading

bpasero commented Apr 15, 2016

mquandalle commented Apr 15, 2016

bpasero commented Apr 15, 2016

Tyriar commented Apr 15, 2016

mquandalle commented Apr 14, 2016 •

edited

Loading