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

Should panel toggle keybindings focus instead of toggle when the panel is not active? #19400

Closed
Tyriar opened this Issue Jan 25, 2017 · 29 comments

Comments

@Tyriar
Member

Tyriar commented Jan 25, 2017

Currently all the panel toggle commands hide the respective panel when it's visible and show it when it's hidden. I find this annoying though and instead use the following custom keybinding:

{
  "key": "ctrl+`",
  "command": "workbench.action.terminal.focus",
  "when": "!terminalFocus"
}

This overrides the setting when the terminal does not have focus to simply focus it, but retain the hide behavior when it is focused. This means you don't have to hit ctrl+` twice in order to get focus to the terminal when it's already displayed but not focused. This does change the hiding the terminal case when it's not focused to need 2 ctrl+`'s but personally I hardly ever do this compared to switching focus from the editor to the terminal.

I frequently tell users about my config and they all react positively so I'm wondering whether we should make this the default behavior for all panels (to keep them consistent).

/discuss

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Jan 25, 2017

Contributor

I believe this should be the default behavior because it aligns with user intent: no matter where I'm at in the editor, if I hit the hotkey it should bring me to the respective panel. If I'm already there I probably want to toggle it.

Contributor

daviwil commented Jan 25, 2017

I believe this should be the default behavior because it aligns with user intent: no matter where I'm at in the editor, if I hit the hotkey it should bring me to the respective panel. If I'm already there I probably want to toggle it.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Jan 26, 2017

Member

I am ok changing this, the sidebar behaves the same (focus in when not focussed and invoking the keybinding). As long as we provide a way to configure the old behaviour.

Member

bpasero commented Jan 26, 2017

I am ok changing this, the sidebar behaves the same (focus in when not focussed and invoking the keybinding). As long as we provide a way to configure the old behaviour.

@bpasero bpasero removed their assignment Jan 26, 2017

@kaiwood

This comment has been minimized.

Show comment
Hide comment
@kaiwood

kaiwood Jan 26, 2017

Contributor

Should be changed, yes. I'm using almost exclusively using workbench.action.focusPanel as soon as the terminal is active because I can't stand the do-2-things-at-once-behaviour of workbench.action.terminal.focus.

Consistency with the sidebar would be a big plus 👍

Contributor

kaiwood commented Jan 26, 2017

Should be changed, yes. I'm using almost exclusively using workbench.action.focusPanel as soon as the terminal is active because I can't stand the do-2-things-at-once-behaviour of workbench.action.terminal.focus.

Consistency with the sidebar would be a big plus 👍

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Jan 26, 2017

Contributor

I am fine with changing this to behave like the sidebar. Especially if the users seem to prefer this. They can always use cmd + J to toggle

Contributor

isidorn commented Jan 26, 2017

I am fine with changing this to behave like the sidebar. Especially if the users seem to prefer this. They can always use cmd + J to toggle

@isidorn isidorn removed their assignment Jan 26, 2017

@sandy081

This comment has been minimized.

Show comment
Hide comment
@sandy081

sandy081 Jan 26, 2017

Member

This was brought up already before - #7540

Member

sandy081 commented Jan 26, 2017

This was brought up already before - #7540

@dbaeumer

This comment has been minimized.

Show comment
Hide comment
@dbaeumer

dbaeumer Jan 26, 2017

Member

+1 for the change.

Member

dbaeumer commented Jan 26, 2017

+1 for the change.

@gandhis1

This comment has been minimized.

Show comment
Hide comment
@gandhis1

gandhis1 Jan 31, 2017

I cannot emphasize how many times I have had to invoke the shortcut twice solely to regain focus on the terminal. Definitely a usability flaw

gandhis1 commented Jan 31, 2017

I cannot emphasize how many times I have had to invoke the shortcut twice solely to regain focus on the terminal. Definitely a usability flaw

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Jan 31, 2017

Member

@isidorn to be clear I'm proposing a conditional focus though so it would not be consistent with the sidebar, just a little more like the sidebar's behavior.

Member

Tyriar commented Jan 31, 2017

@isidorn to be clear I'm proposing a conditional focus though so it would not be consistent with the sidebar, just a little more like the sidebar's behavior.

@sqlaide

This comment has been minimized.

Show comment
Hide comment
@sqlaide

sqlaide Mar 31, 2017

Personally, i would just like to switch cursor between terminal and editor without hiding terminal. This setting works for me.
'{
"key": "ctrl+`",
"command": "workbench.action.terminal.focus",
"when": "!terminalFocus"
},
{
"key": "ctrl+`",
"command": "workbench.action.focusActiveEditorGroup",
"when": "terminalFocus"
}'

sqlaide commented Mar 31, 2017

Personally, i would just like to switch cursor between terminal and editor without hiding terminal. This setting works for me.
'{
"key": "ctrl+`",
"command": "workbench.action.terminal.focus",
"when": "!terminalFocus"
},
{
"key": "ctrl+`",
"command": "workbench.action.focusActiveEditorGroup",
"when": "terminalFocus"
}'

@Tyriar Tyriar added feature-request and removed *question labels Apr 21, 2017

@ericis

This comment has been minimized.

Show comment
Hide comment
@ericis

ericis Aug 18, 2017

This keybinding works in v1.15.0 ctrl+Alt+'

However, it's one of the most awkward key combinations ever!

ericis commented Aug 18, 2017

This keybinding works in v1.15.0 ctrl+Alt+'

However, it's one of the most awkward key combinations ever!

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 6, 2017

Member

@dbaeumer @isidorn @sandy081 this is currently blocked on when context clauses for output, problems and tasks which I don't think exist currently https://code.visualstudio.com/docs/getstarted/keybindings#_when-clause-contexts

I'd be happy to make the keybinding changes after the contexts are implemented.

Member

Tyriar commented Sep 6, 2017

@dbaeumer @isidorn @sandy081 this is currently blocked on when context clauses for output, problems and tasks which I don't think exist currently https://code.visualstudio.com/docs/getstarted/keybindings#_when-clause-contexts

I'd be happy to make the keybinding changes after the contexts are implemented.

@isidorn isidorn removed their assignment Oct 27, 2017

@bpasero bpasero added workbench-layout and removed workbench labels Nov 16, 2017

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 13, 2018

Member

@Tyriar for my understanding: you are suggesting to change our various keybindings for panel items to move focus in and out instead of hide & show right? When I look at how our keybindings for the side bar behave, it seems there we do exactly that (focus in and focus out). Given that I think being consistent would be good. Though there is #48545 which asks for changing our viewlet keybindings to actually show & hide.

Member

bpasero commented Sep 13, 2018

@Tyriar for my understanding: you are suggesting to change our various keybindings for panel items to move focus in and out instead of hide & show right? When I look at how our keybindings for the side bar behave, it seems there we do exactly that (focus in and focus out). Given that I think being consistent would be good. Though there is #48545 which asks for changing our viewlet keybindings to actually show & hide.

@dbaeumer dbaeumer removed their assignment Sep 13, 2018

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 13, 2018

Member

@bpasero I'm suggesting not quite how the viewlet behaves:

  • When the panel is open and not focused: It should focus
  • When the panel is open and focused: It should hide
  • When the panel is closed: It should show and focus
Member

Tyriar commented Sep 13, 2018

@bpasero I'm suggesting not quite how the viewlet behaves:

  • When the panel is open and not focused: It should focus
  • When the panel is open and focused: It should hide
  • When the panel is closed: It should show and focus
@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 13, 2018

Member

Ok thanks for clarifying. I think we changed this behaviour we should also change it for the sidebar.

Member

bpasero commented Sep 13, 2018

Ok thanks for clarifying. I think we changed this behaviour we should also change it for the sidebar.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 14, 2018

Member

I think my last statement was not good now that I see where these commands are used: we reference them from the main menu (under the "View" menu) and so potentially clicking there can mean to hide the panel because the panel was already visible.

I think we should at least be consistent and do the same as the sidebar:

  • When the panel is open and not focused: It should focus
  • When the panel is open and focused: It should focus the editor (so you can jump in and out)
  • When the panel is closed: It should show and focus

I am not sure what the history behind these commands that they would hide the panel (@isidorn ?) but the current behaviour is weird and lead us to duplicate certain commands, such as:

image

Thoughts?

Member

bpasero commented Sep 14, 2018

I think my last statement was not good now that I see where these commands are used: we reference them from the main menu (under the "View" menu) and so potentially clicking there can mean to hide the panel because the panel was already visible.

I think we should at least be consistent and do the same as the sidebar:

  • When the panel is open and not focused: It should focus
  • When the panel is open and focused: It should focus the editor (so you can jump in and out)
  • When the panel is closed: It should show and focus

I am not sure what the history behind these commands that they would hide the panel (@isidorn ?) but the current behaviour is weird and lead us to duplicate certain commands, such as:

image

Thoughts?

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 14, 2018

Member

@bpasero I'm not suggesting adding another command, rather adding new default keybindings:

{
  "key": "ctrl+`",
  "command": "workbench.action.terminal.focus",
  "when": "!terminalFocus"
},
{
  "key": "shift+ctrl+m",
  "command": "workbench.action.problems.focus",
  "when": "!problemsFocus"
},
...

When the panel is open and focused: It should focus the editor (so you can jump in and out)

This doesn't really solve the problem though imo, I use ctrl/cmd+1 when I want to switch back to the editor personally. The above keybindings just makes it nicer to work with the terminal such that you don't need to trigger toggle terminal twice if it's already visible. I often recommend people use this setup for toggle terminal and I've only heard positive feedback.

Member

Tyriar commented Sep 14, 2018

@bpasero I'm not suggesting adding another command, rather adding new default keybindings:

{
  "key": "ctrl+`",
  "command": "workbench.action.terminal.focus",
  "when": "!terminalFocus"
},
{
  "key": "shift+ctrl+m",
  "command": "workbench.action.problems.focus",
  "when": "!problemsFocus"
},
...

When the panel is open and focused: It should focus the editor (so you can jump in and out)

This doesn't really solve the problem though imo, I use ctrl/cmd+1 when I want to switch back to the editor personally. The above keybindings just makes it nicer to work with the terminal such that you don't need to trigger toggle terminal twice if it's already visible. I often recommend people use this setup for toggle terminal and I've only heard positive feedback.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 14, 2018

Member

@Tyriar I still dislike the fact that viewlets behave differently from panels

Member

bpasero commented Sep 14, 2018

@Tyriar I still dislike the fact that viewlets behave differently from panels

bpasero added a commit that referenced this issue Sep 17, 2018

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 17, 2018

Member

@Tyriar I have a change ready in c7b740f that would change every panel toggle action to behave as outlined in #19400 (comment). I do not think we can just fix this via changing our default keybindings, because what about people that have changed the keybindings manually?

Member

bpasero commented Sep 17, 2018

@Tyriar I have a change ready in c7b740f that would change every panel toggle action to behave as outlined in #19400 (comment). I do not think we can just fix this via changing our default keybindings, because what about people that have changed the keybindings manually?

@Tyriar

This comment has been minimized.

Show comment
Hide comment
@Tyriar

Tyriar Sep 17, 2018

Member

@bpasero looks ok, one thing to keep in mind is that doing it this way makes you unable to hide the panels via the command palette. You can still use "toggle panel", but "toggle terminal" will actually focus the terminal since the command palette was focused.

Member

Tyriar commented Sep 17, 2018

@bpasero looks ok, one thing to keep in mind is that doing it this way makes you unable to hide the panels via the command palette. You can still use "toggle panel", but "toggle terminal" will actually focus the terminal since the command palette was focused.

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 18, 2018

Member

Good point, I suggest we talk about this in the UX call.

Member

bpasero commented Sep 18, 2018

Good point, I suggest we talk about this in the UX call.

bpasero added a commit that referenced this issue Sep 20, 2018

@bpasero bpasero self-assigned this Sep 20, 2018

@bpasero bpasero added this to the September 2018 milestone Sep 20, 2018

@bpasero bpasero closed this Sep 20, 2018

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 20, 2018

Member

The behaviour of the commands for toggling panels has changed as outlined in #19400 (comment). To get back the previous behaviour, simply configure a keybinding to close the panel when the related panel is active, e.g. for output:

{
	"key": "cmd+shift+u",
	"command": "workbench.action.closePanel",
	"when": "activePanel==workbench.panel.output"
}

The list of panel identifiers is:

  • workbench.panel.terminal
  • workbench.panel.markers
  • workbench.panel.output
  • workbench.panel.repl
Member

bpasero commented Sep 20, 2018

The behaviour of the commands for toggling panels has changed as outlined in #19400 (comment). To get back the previous behaviour, simply configure a keybinding to close the panel when the related panel is active, e.g. for output:

{
	"key": "cmd+shift+u",
	"command": "workbench.action.closePanel",
	"when": "activePanel==workbench.panel.output"
}

The list of panel identifiers is:

  • workbench.panel.terminal
  • workbench.panel.markers
  • workbench.panel.output
  • workbench.panel.repl
@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 24, 2018

Member

Verification: see #19400 (comment)

Member

bpasero commented Sep 24, 2018

Verification: see #19400 (comment)

@isidorn isidorn added the verified label Sep 26, 2018

@isidorn

This comment has been minimized.

Show comment
Hide comment
@isidorn

isidorn Sep 26, 2018

Contributor

@bpasero I think you took @Tyriar comment a bit too literarly. I verified it works great, however there is one corner case which is a regression, thus reopening

  1. View > terminal -> terminal gets nicely opened
  2. View > output -> panel gets closed. Instead the output should be shown
Contributor

isidorn commented Sep 26, 2018

@bpasero I think you took @Tyriar comment a bit too literarly. I verified it works great, however there is one corner case which is a regression, thus reopening

  1. View > terminal -> terminal gets nicely opened
  2. View > output -> panel gets closed. Instead the output should be shown

@isidorn isidorn reopened this Sep 26, 2018

@bpasero

This comment has been minimized.

Show comment
Hide comment
@bpasero

bpasero Sep 26, 2018

Member

Wow, not sure how I did not test that. Fixed, thanks!

Member

bpasero commented Sep 26, 2018

Wow, not sure how I did not test that. Fixed, thanks!

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