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

Dont scroll to revealed files option. #96890

Merged
merged 5 commits into from May 13, 2020
Merged

Dont scroll to revealed files option. #96890

merged 5 commits into from May 13, 2020

Conversation

phuein
Copy link
Contributor

@phuein phuein commented May 4, 2020

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
Copy link

msftclas commented May 4, 2020

CLA assistant check
All CLA requirements met.

@@ -375,6 +375,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('autoReveal', "Controls whether the explorer should automatically reveal and select files when opening them."),
'default': true
},
'explorer.autoRevealNoScroll': {
'type': 'boolean',
'description': nls.localize('autoRevealNoScroll', "Controls whether the explorer should not automatically scroll to revealed files."),
Copy link
Contributor

@isidorn isidorn May 4, 2020

Choose a reason for hiding this comment

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

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

@@ -375,6 +375,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('autoReveal', "Controls whether the explorer should automatically reveal and select files when opening them."),
'default': true
},
'explorer.autoRevealNoScroll': {
Copy link
Contributor

@isidorn isidorn May 4, 2020

Choose a reason for hiding this comment

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

explorer.autoRevealScroll is a better name

Copy link

@clounie clounie May 5, 2020

Choose a reason for hiding this comment

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

What about explorer.autoScrollOnReveal ?

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

Copy link

@clounie clounie May 6, 2020

Choose a reason for hiding this comment

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

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

@isidorn
Copy link
Contributor

isidorn commented May 4, 2020

@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
Copy link
Contributor Author

phuein commented May 4, 2020

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.

  1. 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.

  2. When moving between tabs often, the scrolling / changed project view may become a visual distraction.

@isidorn
Copy link
Contributor

isidorn commented May 5, 2020

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

  1. 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?
  2. Same as 1. when changing focus from a.txt to b.txt if both are visible no scrolling will happen.

@clounie
Copy link

clounie commented May 5, 2020

@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.

@@ -375,6 +375,11 @@ configurationRegistry.registerConfiguration({
'description': nls.localize('autoReveal', "Controls whether the explorer should automatically reveal and select files when opening them."),
'default': true
},
'explorer.autoRevealScroll': {
'type': 'boolean',
'description': nls.localize('autoRevealScroll', "Controls whether the explorer should automatically scroll to revealed files."),
Copy link

@clounie clounie May 5, 2020

Choose a reason for hiding this comment

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

Perhaps -

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

Copy link
Contributor Author

@phuein phuein May 6, 2020

Choose a reason for hiding this comment

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

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

@phuein
Copy link
Contributor Author

phuein commented May 6, 2020

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
Copy link
Contributor

isidorn commented May 6, 2020

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

@clounie
Copy link

clounie commented May 6, 2020

@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
Copy link

clounie commented May 6, 2020

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

Usecase 1

  1. I'm in a Node.js project looking at a folder with 6 different files
  2. I Cmd+Click to explore one of my dependencies in node_modules
  3. VS Code proceeds to open that node module in explorer, opening a thousand different hierachies in the node_modules folder
  4. 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
Copy link
Contributor

isidorn commented May 7, 2020

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
Copy link
Contributor Author

phuein commented May 9, 2020

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
Copy link
Contributor

isidorn commented May 11, 2020

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:

  1. You left a console.log in the code
  2. 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.

Screenshot 2020-05-11 at 11 28 48

Also fix types for setting variable type error.
@phuein
Copy link
Contributor Author

phuein commented May 12, 2020

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
Copy link
Contributor

isidorn commented May 12, 2020

Thanks. However looking at the suggested values this is not good. It should suggest true false or the "highlightNoScroll"
Screenshot 2020-05-12 at 10 22 58

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

@isidorn isidorn May 12, 2020

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@phuein phuein May 12, 2020

Choose a reason for hiding this comment

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

Nice catch, I missed that line. Committed again.

@isidorn
Copy link
Contributor

isidorn commented May 13, 2020

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

@isidorn isidorn merged commit 67bc3d7 into microsoft:master May 13, 2020
1 of 2 checks passed
@isidorn isidorn added this to the May 2020 milestone May 13, 2020
@isidorn
Copy link
Contributor

isidorn commented May 13, 2020

Pushed a bit of polish via a06c753

@phuein
Copy link
Contributor Author

phuein commented May 15, 2020

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

@isidorn
Copy link
Contributor

isidorn commented May 15, 2020

Thanks for contributing!

@github-actions github-actions bot locked and limited conversation to collaborators Jun 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to keep file selected in sidebar without forcing a scroll
4 participants