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

Move panel.location from settings into local storage #37351

Closed
mjbvz opened this Issue Oct 31, 2017 · 16 comments

Comments

Projects
None yet
7 participants
@mjbvz
Contributor

mjbvz commented Oct 31, 2017

Testing #36571

Repo steps

  1. In a multiroot workspace
  2. In your user settings have: "workbench.panel.location": "bottom"
  3. In the workspace settings have "workbench.panel.location": "right"
  4. The panel should appear to the right.
  5. Now try using commands or the panel button to change the panel position to the bottom

Bug
Position cannot be changed

@mjbvz mjbvz changed the title from Cannot change panel position to Cannot change panel position when using both workspace and user settings Oct 31, 2017

@isidorn isidorn added this to the November 2017 milestone Oct 31, 2017

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 8, 2017

Contributor

@mjbvz this is expected behavior. Though I understand it is an ugly user experience.
You can have the same for many other things in our UX e.g status bar hidden, sidebar location and absolutely all the ux settings.

@sandy081 @bpasero why do we allow these settings to be written in the workspace settings in the first place. They should only be user settings. Do we already have an issue for that so I close this as a duplicate.
Bottom line: workspace settings should be only the ones which are allowed as resource settings.

Other suggestions on how to improve this are welcome

Contributor

isidorn commented Nov 8, 2017

@mjbvz this is expected behavior. Though I understand it is an ugly user experience.
You can have the same for many other things in our UX e.g status bar hidden, sidebar location and absolutely all the ux settings.

@sandy081 @bpasero why do we allow these settings to be written in the workspace settings in the first place. They should only be user settings. Do we already have an issue for that so I close this as a duplicate.
Bottom line: workspace settings should be only the ones which are allowed as resource settings.

Other suggestions on how to improve this are welcome

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 8, 2017

Member

IMO we are already too late to take such decision and this will be a breaking change.

One suggestion here is let the commands update the value at the right place instead of hard coding to user settings. You can do that by not providing any target while writing using IConfigurationService.updateValue and the service writes it smartly.

Member

sandy081 commented Nov 8, 2017

IMO we are already too late to take such decision and this will be a breaking change.

One suggestion here is let the commands update the value at the right place instead of hard coding to user settings. You can do that by not providing any target while writing using IConfigurationService.updateValue and the service writes it smartly.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 9, 2017

Member

@sandy081 @bpasero why do we allow these settings to be written in the workspace settings in the first place. They should only be user settings. Do we already have an issue for that so I close this as a duplicate.
Bottom line: workspace settings should be only the ones which are allowed as resource settings.

I do not understand that statement. We have workspace settings since day 1 and the panel position is not a resource setting. It is perfectly fine that a user configures the panel in one workspace to be vertical and horizontal in the other, no?

Member

bpasero commented Nov 9, 2017

@sandy081 @bpasero why do we allow these settings to be written in the workspace settings in the first place. They should only be user settings. Do we already have an issue for that so I close this as a duplicate.
Bottom line: workspace settings should be only the ones which are allowed as resource settings.

I do not understand that statement. We have workspace settings since day 1 and the panel position is not a resource setting. It is perfectly fine that a user configures the panel in one workspace to be vertical and horizontal in the other, no?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 9, 2017

Contributor

@sandy081 thanks for the suggestion. So this would write it to the most specific location? We should then change all our UX write settings to use this. @bpasero do you object if I change this for statusbar, sidebar and others

Sure it is perfectly fine but I personally do not buy that use case since day one :) I do not think users use workspace settings as a UX persistance mechanism.

Contributor

isidorn commented Nov 9, 2017

@sandy081 thanks for the suggestion. So this would write it to the most specific location? We should then change all our UX write settings to use this. @bpasero do you object if I change this for statusbar, sidebar and others

Sure it is perfectly fine but I personally do not buy that use case since day one :) I do not think users use workspace settings as a UX persistance mechanism.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 9, 2017

Member

@isidorn Yes, there is a plan to adopt to all such settings. Please go ahead and adopt them. 👍

Member

sandy081 commented Nov 9, 2017

@isidorn Yes, there is a plan to adopt to all such settings. Please go ahead and adopt them. 👍

@octref

This comment has been minimized.

Show comment
Hide comment
@octref

octref Nov 13, 2017

Member

StatusBar and Sidebar are different use cases than Vertical Panels.
I have global setting for StatusBar and Sidebar and don't foresee myself changing them ever (or have workspace setting for them). But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window using either a Quick Command or the Button on the Panel.

Member

octref commented Nov 13, 2017

StatusBar and Sidebar are different use cases than Vertical Panels.
I have global setting for StatusBar and Sidebar and don't foresee myself changing them ever (or have workspace setting for them). But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window using either a Quick Command or the Button on the Panel.

@isidorn isidorn added the debt label Nov 13, 2017

@isidorn isidorn changed the title from Cannot change panel position when using both workspace and user settings to UX settings should be written to the most specific location Nov 13, 2017

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Nov 14, 2017

Member

But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window

@octref makes a really good point, a user with a vertical and a horizontal monitor will be one of the common cases for wanting differing settings. Setting ti at a workspace settings level is clunky as you don't want to check this setting in.

Why don't we use the setting as the default, but when it's set by the user it's saved in the workspace's local storage? Perhaps there should be 2 sets of commands to compliment this as well, one to change the default and one to change the current window.

Member

Tyriar commented Nov 14, 2017

But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window

@octref makes a really good point, a user with a vertical and a horizontal monitor will be one of the common cases for wanting differing settings. Setting ti at a workspace settings level is clunky as you don't want to check this setting in.

Why don't we use the setting as the default, but when it's set by the user it's saved in the workspace's local storage? Perhaps there should be 2 sets of commands to compliment this as well, one to change the default and one to change the current window.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 14, 2017

Member

Yeah, the panel location might be one of those where it is hard to decide if it should be a setting or a local storage thing. For example, we also put the sidebar visibility into local storage but not into settings because we think it changes often.

Member

bpasero commented Nov 14, 2017

Yeah, the panel location might be one of those where it is hard to decide if it should be a setting or a local storage thing. For example, we also put the sidebar visibility into local storage but not into settings because we think it changes often.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 14, 2017

Member

One advantage of making something available as a setting is that you can copy and synchronise your set up across machines, for example, using Settings Sync Extension. In that perspective, panel location as a setting is good.

But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window

I see this is asking for supporting non-sharable workspace settings, for now we are advocating to use new Workspace as a workaround solution. If you do not want to pollute your settings under vscode\settings.json with your personal ones, you can create a workspace out of it and have your personal workspace settings in it. BUT, It has the limitation of restricting this solution only to folder workspaces.

Member

sandy081 commented Nov 14, 2017

One advantage of making something available as a setting is that you can copy and synchronise your set up across machines, for example, using Settings Sync Extension. In that perspective, panel location as a setting is good.

But for vertical panel, it's really when I move a window to a widescreen monitor I wish I could toggle the layout specific for that window

I see this is asking for supporting non-sharable workspace settings, for now we are advocating to use new Workspace as a workaround solution. If you do not want to pollute your settings under vscode\settings.json with your personal ones, you can create a workspace out of it and have your personal workspace settings in it. BUT, It has the limitation of restricting this solution only to folder workspaces.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 14, 2017

Member

Yeah, but I think for a user this is not discoverable after all because you invoke the action and it changes the panel in all windows. I think that users would not even be aware of this thing being written to settings.

Member

bpasero commented Nov 14, 2017

Yeah, but I think for a user this is not discoverable after all because you invoke the action and it changes the panel in all windows. I think that users would not even be aware of this thing being written to settings.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 14, 2017

Member

True, but as a setting it gives advantage to those users who want to retain their set ups.

Member

sandy081 commented Nov 14, 2017

True, but as a setting it gives advantage to those users who want to retain their set ups.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 15, 2017

Contributor

After trying this out and processing user feedback I have decided to write the panel position into workspace storage and to deprecate the setting.
Users switch this all the time, and it just does not feel like a real setting as already nicely explaind in comments above.

I have decided to leave the other ux settings untouched since it seems that every setting based on the feeling either needs to go to local storage or to our settings.
Though I would argue that most of them should just go to local storage, but that is for another discussion.

Contributor

isidorn commented Nov 15, 2017

After trying this out and processing user feedback I have decided to write the panel position into workspace storage and to deprecate the setting.
Users switch this all the time, and it just does not feel like a real setting as already nicely explaind in comments above.

I have decided to leave the other ux settings untouched since it seems that every setting based on the feeling either needs to go to local storage or to our settings.
Though I would argue that most of them should just go to local storage, but that is for another discussion.

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Nov 16, 2017

Member

@isidorn Good. Would like to know what is our solution if users wants to have vertical panel as default?

Member

sandy081 commented Nov 16, 2017

@isidorn Good. Would like to know what is our solution if users wants to have vertical panel as default?

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 16, 2017

Contributor

@sandy081 not supported since the downside is that they need to click one button per workspace

Contributor

isidorn commented Nov 16, 2017

@sandy081 not supported since the downside is that they need to click one button per workspace

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Nov 16, 2017

Member

How about seeding the default value actually from settings? I would maybe then also rename the setting to make clear that it would be the default value. We could apply this pattern also to the side bar visibility thing so that users can define a default initial state until the state is changed.

Member

bpasero commented Nov 16, 2017

How about seeding the default value actually from settings? I would maybe then also rename the setting to make clear that it would be the default value. We could apply this pattern also to the side bar visibility thing so that users can define a default initial state until the state is changed.

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Nov 16, 2017

Contributor

@bpasero this sounds like it would align nice. However I am not convinced this would actually be used by people.
Waiting for a user feature request / complain before I actually jump on this.
Not a fan of just adding settings for everything.

Contributor

isidorn commented Nov 16, 2017

@bpasero this sounds like it would align nice. However I am not convinced this would actually be used by people.
Waiting for a user feature request / complain before I actually jump on this.
Not a fan of just adding settings for everything.

@isidorn isidorn changed the title from UX settings should be written to the most specific location to Move panel.location from settings into local storage Nov 20, 2017

@isidorn isidorn added feature-request and removed debt labels Nov 20, 2017

@kieferrm kieferrm added the verified label Dec 5, 2017

@vscodebot vscodebot bot locked and limited conversation to collaborators Dec 30, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.