Dont scroll to revealed files option. #96890

phuein · 2020-05-04T00:54:15Z

Adds a new option in settings to not scroll to revealed files in the file explorer view.

This PR fixes #23902

Quick demo: https://drive.google.com/file/d/1k_RL401kYfRgdfC1nVHkyPI65BSL-oXL/view?usp=sharing

Adds a new option in settings to not scroll to revealed files in the file explorer view.

msftclas · 2020-05-04T00:54:27Z

All CLA requirements met.

isidorn · 2020-05-04T12:34:59Z

src/vs/workbench/contrib/files/browser/files.contribution.ts

 		},
+		'explorer.autoRevealNoScroll': {
+			'type': 'boolean',
+			'description': nls.localize('autoRevealNoScroll', "Controls whether the explorer should not automatically scroll to revealed files."),


Please chagne to "should automatically" the word "not" makes the sentence more confusing

isidorn · 2020-05-04T12:35:11Z

src/vs/workbench/contrib/files/browser/files.contribution.ts

 			'description': nls.localize('autoReveal', "Controls whether the explorer should automatically reveal and select files when opening them."),
 			'default': true
 		},
+		'explorer.autoRevealNoScroll': {


explorer.autoRevealScroll is a better name

What about explorer.autoScrollOnReveal ?

As an end user that tells me exactly what's happening without even having to read the description.

autoScrollOnReveal also highlights the dependency on autoReveal which you referenced here @phuein #96890 (comment)

isidorn · 2020-05-04T12:39:21Z

@phuein thank you for the PR.
I left some minor comments in your code.
Some things to think about: do users request for other auto reveal behavior and should this setting be a boolean or an open ended string. I suggest you search for all issues labeld with file-explorer and search for reveal. And see if there is more things which we could tackle as part of this setting.

However I am not 100% convinced we need this. The deafult behavior imho is fine

If the file is visible do not scroll when auto revealing
If explicitly explorer is focused do scroll

What is the case that does not work for you.

autoRevealNoScroll -> autoRevealScroll with a matching description and default value to true, the editor's default behavior.

phuein · 2020-05-04T22:17:28Z

Hi @isidorn, thanks for reviewing my PR.

I've modified the name and description to fit your suggestions. I agree it's less verbose and confusing this way.

I ran a search for is:open label:file-explorer reveal and I'm only seeing a repeat of this request / issue, so it doesn't seem like anything more needs to be added. If more requests come in relating to this issue, I'd be happy to review them, so feel free to tag me.

The case that bothers users is two-fold, and made us disable explorer.autoReveal entirely, which loses having the current file highlighted.

It may be unwanted for the explorer view to scroll away when closing a file tab. This requires the user to scroll back; repeatedly, when cleaning up tabs.
When moving between tabs often, the scrolling / changed project view may become a visual distraction.

isidorn · 2020-05-05T10:18:16Z

Thanks for updating the PR and for claryfing.
However let's go deeper into 1 and 2.

So you have a.txt and b.txt open as tabs. Both are visible in the explorer. b.txt is focused, you close it and now a.txt is open. Explorer does not scroll for me. It only scroll if a.txt was not visible. Which is expected. Can you please clarify this?
Same as 1. when changing focus from a.txt to b.txt if both are visible no scrolling will happen.

clounie · 2020-05-05T20:31:57Z

@isidorn have you read the original description? It is pretty detailed, this feels like a repeat discussion.

#23902 (comment)

We just want to see the active file highlighted like normal in the sidebar, without the sidebar scrolling on us when our active file changes.

clounie · 2020-05-05T20:49:24Z

src/vs/workbench/contrib/files/browser/files.contribution.ts

 		},
+		'explorer.autoRevealScroll': {
+			'type': 'boolean',
+			'description': nls.localize('autoRevealScroll', "Controls whether the explorer should automatically scroll to revealed files."),


Perhaps -

Controls whether the explorer should automatically scroll when a different file is selected

autoRevealScroll depends on autoReveal being active, so I feel the current description fits better.

phuein · 2020-05-06T01:09:29Z

For both 1 and 2, this issue only refers to when the file is not visible in the explorer view, and so causing an unwanted scroll. With this in mind, my previous post explains scenarios where this scrolling may be unwanted.

Video example of scrolling scenario:
https://drive.google.com/file/d/1doXAwszkVNQtj2NUGDw0Fj8kwOkSz_tM/view?usp=sharing

isidorn · 2020-05-06T07:21:11Z

@phuein so why don't you disable autoReveal? Because you want autoReval only when the item is in the viewport?

clounie · 2020-05-06T20:12:26Z

@phuein so why don't you disable autoReveal? Because you want autoReval only when the item is in the viewport?

Because if you disable autoReveal, then Explorer does not correctly highlight the active file. It permanently keeps the highlight on the last file the user selected in explorer. That is, until the user explicitly clicks on a different file in Explorer.

clounie · 2020-05-06T20:18:03Z

@isidorn here are two usecases where the current behavior is very frustrating:

Usecase 1

I'm in a Node.js project looking at a folder with 6 different files
I Cmd+Click to explore one of my dependencies in node_modules
VS Code proceeds to open that node module in explorer, opening a thousand different hierachies in the node_modules folder
Because of the autoReveal scroll, I now have to scroll through hundreds if not thousands of folders before I can close the node_modules folder. Until I do that, I can't see the rest of my project hierarchy (both above and below node_modules)

That is incredibly annoying as a user. It takes you completely out of flow.

However, I still want to see the active file highlighted (just not scrolled to) because that behavior is very useful.

Usecase 2

Another usecase would be exploring down the sidebar and wanting to switch between viewing files you're exploring in the sidebar, as well as already-open tabs -- without VS Code forcing you to lose your place because the files aren't in the same part of the folder hierarchy.

Ref #23902 (comment)

isidorn · 2020-05-07T07:50:54Z

Ok thanks for the explanation.
So what does not work for me in this PR is the setting name:
"autoRevealScroll"
If the setting is not scrolled into view it is not revealed, those are synonyms. Also it is funky that the autoRevealScroll depends on another setting autoReveal.

Idealy we would change the autoReveal setting to support both boolean values as before and string values.
So I can be true | false | "on" | "off" | "highlightNoScroll".
That way all is under one setting and this feature would be more discoverable.

autoRevealScroll removed, AutoReveal class added with string options, and maintained backwards boolean value compatibility.

phuein · 2020-05-09T05:54:57Z

I'm glad the issue and user needs are clarified. I also agree that the previous added setting was funky. I went with it as a simple concept. Modifying autoReveal added complexity. Especially with being boolean backwards compatible. It's now changed and implemented with the new commit.

Thanks for reviewing my work.

isidorn · 2020-05-11T09:32:18Z

Thanks for the changes.
I tried it out and looks good overall.

However there is one problem, now when users who have explorer.autoReveal: true or false will see a warning when they open their settings.json
Unfortunetly we can not ship like this, since thousands of users might see this.
So we need to use a JSON Schema OR type. On the file.contribution line 374 change.

Apart from that:

You left a console.log in the code
I do not see the benefit of introducing the AutoScroll enum. That way you have to change the code in all the clients. Why not handle it locally in the explorerService so you expect true / false / string. That way changes will be local to that file.

Also fix types for setting variable type error.

phuein · 2020-05-12T01:04:51Z

Alright, this commit should simplify it. It's either true / false / [string]. Less files and lines are affected. Also fixed the type error for the setting variable. And removed the excess console.log().

isidorn · 2020-05-12T08:24:00Z

Thanks. However looking at the suggested values this is not good. It should suggest true false or the "highlightNoScroll"

isidorn · 2020-05-12T08:24:32Z

src/vs/workbench/contrib/files/browser/files.contribution.ts

+			'type': ['boolean', 'string'],
+			'enum': [true, false, 'highlightNoScroll'],
+			'default': 'autoReveal.on',
+			'enumDescriptions': [


I think this enum descriptions and the default are wrong. Default should be true.

Nice catch, I missed that line. Committed again.

isidorn · 2020-05-13T13:44:39Z

Good job, thanks for this PR. Merging in ☀️ 🍺

isidorn · 2020-05-13T13:51:45Z

Pushed a bit of polish via a06c753

phuein · 2020-05-15T03:10:21Z

Awesome! Thanks for helping out throughout the PR. Everything should be checked now, and I can't wait to enjoy this on release.

isidorn · 2020-05-15T08:08:51Z

Thanks for contributing!

Dont scroll to revealed files option.

737032c

Adds a new option in settings to not scroll to revealed files in the file explorer view.

alexr00 assigned isidorn May 4, 2020

isidorn requested changes May 4, 2020

View reviewed changes

Nicer name and description after review

ba5cac4

autoRevealNoScroll -> autoRevealScroll with a matching description and default value to true, the editor's default behavior.

clounie reviewed May 5, 2020

View reviewed changes

autoReveal changed into class with added options

a3be328

autoRevealScroll removed, AutoReveal class added with string options, and maintained backwards boolean value compatibility.

Remove autoReveal class and simplify selection

c2d3bf9

Also fix types for setting variable type error.

isidorn requested changes May 12, 2020

View reviewed changes

Fixed autoReveal default value

d5a0fe0

isidorn merged commit 67bc3d7 into microsoft:master May 13, 2020

isidorn added this to the May 2020 milestone May 13, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Dont scroll to revealed files option. #96890

Dont scroll to revealed files option. #96890

Uh oh!

phuein commented May 4, 2020

msftclas commented May 4, 2020 •

edited

Loading

isidorn May 4, 2020

isidorn May 4, 2020

clounie May 5, 2020

clounie May 6, 2020

isidorn commented May 4, 2020

phuein commented May 4, 2020

isidorn commented May 5, 2020

clounie commented May 5, 2020 •

edited

Loading

clounie May 5, 2020

phuein May 6, 2020

phuein commented May 6, 2020

isidorn commented May 6, 2020

clounie commented May 6, 2020 •

edited

Loading

clounie commented May 6, 2020 •

edited

Loading

isidorn commented May 7, 2020

phuein commented May 9, 2020

isidorn commented May 11, 2020

phuein commented May 12, 2020

isidorn commented May 12, 2020

isidorn May 12, 2020

phuein May 12, 2020

isidorn commented May 13, 2020

isidorn commented May 13, 2020

phuein commented May 15, 2020

isidorn commented May 15, 2020

Labels

4 participants

Dont scroll to revealed files option. #96890

Dont scroll to revealed files option. #96890

Uh oh!

Conversation

phuein commented May 4, 2020

msftclas commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented May 4, 2020

phuein commented May 4, 2020

isidorn commented May 5, 2020

clounie commented May 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phuein commented May 6, 2020

isidorn commented May 6, 2020

clounie commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

clounie commented May 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usecase 1

Usecase 2

isidorn commented May 7, 2020

phuein commented May 9, 2020

isidorn commented May 11, 2020

phuein commented May 12, 2020

isidorn commented May 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isidorn commented May 13, 2020

isidorn commented May 13, 2020

phuein commented May 15, 2020

isidorn commented May 15, 2020

Labels

4 participants

msftclas commented May 4, 2020 •

edited

Loading

clounie commented May 5, 2020 •

edited

Loading

clounie commented May 6, 2020 •

edited

Loading

clounie commented May 6, 2020 •

edited

Loading