GNOME Bugzilla – Bug 659288
Ctrl+Alt+[home end]
Last modified: 2014-04-16 19:47:38 UTC
With Ctrl+Alt+Arrow Keys i can switch through virtual desktops, but it would be nice if i could go to the first/last desktop with Ctrl+Alt+Home/End
Created attachment 244507 [details] [review] [PATCH] windowManager: Add switch to first/last workspace keybindings Not tested yet (it probably doesn't work), I just want to know if I'm in the right direction. I can't find where the schema files for the key-bindings are, so I'll need to know that as well so I can actually add the bindings.
See data/org.gnome.shell.gschema.xml.in.in
(In reply to comment #1) > Not tested yet (it probably doesn't work), I just want to know if I'm in the > right direction. I don't think it makes sense to pop up the switcher popup - conceptually, those keybindings look far closer to 'switch-to-workspace-n' to me than 'switch-to-workspace-up'. Without the switcher, I think implementation is best kept in mutter (except for two additional calls to allowKeybinding() in windowManager.js). Speaking of which, you didn't attach the mutter part - not sure it exists, but the patch won't work in this form without additions to mutter anyway ... > I can't find where the schema files for the key-bindings are, so I'll need to > know that as well so I can actually add the bindings. It depends. Most built-in keybindings are in org.gnome.desktop.wm.keybindings (from gsettings-desktop-schemas) and are shared with metacity (well, they were - metacity is pretty much unmaintained atm) and maybe compiz. Then there are some keybindings in org.gnome.mutter.keybindings and org.gnome.shell.keybindings (depending on where they are defined).
(In reply to comment #0) > With Ctrl+Alt+Arrow Keys i can switch through virtual desktops, but it would be > nice if i could go to the first/last desktop with Ctrl+Alt+Home/End On a side note, we are moving to <super> in system keybindings (ctrl-alt-arrow is still assigned by default, but we recommend <super>PgUp/Down nowadays), so the default values should be <super>Home and <super>End.
Created attachment 244512 [details] [review] [PATCH] Keybindings: add switch to last/first workspace keybindings [1/3] This is for the schema, I'll attach the mutter and the shell patches shortly
Created attachment 244516 [details] [review] [PATCH] Keybindings: add switch to last/first workspace keybindings [2/3] Mutter part of the patchset
Created attachment 244518 [details] [review] Keybindings: add switch to last/first workspace keybindings [3/3] gnome-shell part of the patchset
My suggestion would be to show the workspace switcher OSD in this case. We're not showing intermediate workspaces during transitions that move between several workspaces at once, since the animation would be too severe. Showing the OSD would preserve a sense of location within the overall workspace layout.
Created attachment 244563 [details] [review] WindowManager: Show workspace switcher OSD for switch-to-workspace-n keybindings This change will make it easier to implement the rest of the fix.
Review of attachment 244563 [details] [review]: Ha, I see what you did in the commit message! But git-log will add additional whitespace in front of the message, so you are still exceeding the limit :-) Also the "It's the design" cop-out was a bit of a joke - it's still nice to have some actual thoughts about a change in there. I'll suggest: WindowManager: Show switcher popup for switch-to-workspace-n keybindings Currently we show the workspace popup for relative targets ("up", "down"), but not when targetting a specific workspace directly. There is not really a good reason for that difference, and as we are about to introduce a new shortcut to target the last workspace (which does vary with dynamic workspaces), it makes sense to unify the behavior and always show the switcher. Other than that and the nitpicks below, I noticed a small glitch: (1) go to workspace 2 (2) hit shortcut for workspace 1 => popup shows up arrow (3) hit shortcut again => no switch, but popup changes to down arrow Not sure it's something we need to address though ... ::: js/ui/windowManager.js @@ +740,3 @@ let newWs; + let direction; + Trailing whitespace - linus hates it, which is why git complains loudly :) @@ +746,3 @@ + if (direction != Meta.MotionDirection.UP && + direction != Meta.MotionDirection.DOWN) + return; I'd leave this where it is now, e.g. below the if-else block ("if (foo) set bar and baz to something, else set bar and baz to something else" is cleaner to read) @@ +751,2 @@ + } else { + newWs = screen.get_workspace_by_index(target-1); missing whitespace around operator - also, you are assuming that (target >= 1) (which happens to be the case), but using "else if (target > 0)" instead won't hurt. @@ +752,3 @@ + newWs = screen.get_workspace_by_index(target-1); + + if (screen.get_active_workspace().index() > newWs.index()) We already know that newWs.index() == target - 1, so you could save a function call (and if you replace (target - 1) with --target above, the calculation as well)
Created attachment 244599 [details] [review] WindowManager: Show switcher popup for switch-to-workspace-n keybindings Addressed all issues you mentioned.
Created attachment 244600 [details] [review] Keybindings: add switch to last/first workspace keybindings [1/3] This is the gstettings-desktop-schemas part of the patchset to add switch to last workspace keybinding. It also adds <super> + Home as default for "switch to workspace 1".
Created attachment 244601 [details] [review] Keybindings: add keybindings to switch to last workspace [2/3] The mutter part of the patchset
Review of attachment 244599 [details] [review]: needs-work because you break stuff (see last comment), overall this is looking good. ::: js/ui/windowManager.js @@ +181,3 @@ + Shell.KeyBindingMode.OVERVIEW, + Lang.bind(this, this._showWorkspaceSwitcher)); + this.setCustomKeybindingHandler('switch-to-workspace-12', You should now do the same for all the move-to-workspace-n shortcuts (it wasn't necessary before, as by default built-in keybindings work in normal mode - now we want to treat it as move-workspace-up etc., so we want the custom handler here as well) @@ +746,1 @@ Pointless whitespace @@ +752,3 @@ + direction = Meta.MotionDirection.UP; + else if (screen.get_active_workspace().index() == target) + return; Not sure about that - we do show the switcher when activating switch-workspace-up on the first workspace, it makes sense to me to do the same here ("hey, you're already on workspace 2, dummy!"). My comment was just about the arrow changing direction, I don't think it's that big of an issue. (On a related note: using an if-elseif-else statement, having the middle part do an early return is pretty odd stylistically) @@ +755,3 @@ + else + direction = Meta.MotionDirection.DOWN; + } Here a blank line would make more sense IMO (feel free to ignore) @@ +796,2 @@ global.display.clear_mouse_mode(); toActivate.activate_with_focus (window, global.get_current_time()); s/toActivate/workspace/
Review of attachment 244600 [details] [review]: It might make sense to split the patch (add <super>end, because it makes sense with dynamic workspaces; set a default value for switching to first workspace, because goes along well with the former), but up to you. Other than that, there's a typo in the commit message ("swith")
Review of attachment 244600 [details] [review]: Oh, also: it's a bit odd to add 'switch-to-last-workspace' but not 'move-to-last-workspace' - except of course that the latter would behave a bit oddly with dynamic workspaces (question for the design team i guess)
Review of attachment 244601 [details] [review]: ::: src/core/keybindings.c @@ +4031,3 @@ + META_KEY_BINDING_NONE, + META_KEYBINDING_ACTION_WORKSPACE_LAST, + handle_switch_to_last_workspace, 0); It would be nice to tie in with the existing handle_switch_to_workspace() mechanism, but ok ::: src/meta/prefs.h @@ +247,3 @@ * @META_KEYBINDING_ACTION_MOVE_TO_WORKSPACE_UP: FILLME * @META_KEYBINDING_ACTION_MOVE_TO_WORKSPACE_DOWN: FILLME + * @META_KEYBINDING_ACTION_MOVE_TO_WORKSPACE_LAST: FILLME FILLME? eeeks, we *do* have some ugly stuff
Created attachment 244632 [details] [review] WindowManager: Show switcher popup for switch-to-workspace-n keybindings
Created attachment 244633 [details] [review] Keybindings: add switch to last/first workspace keybindings [1/3]
To make my workflow easier, I'm going to wait for review on the switch-to-workspace-n patch before submitting the last (3/3) patch to add "switch to last workspace"
Review of attachment 244632 [details] [review]: Minor style nitpick, good to go with that fixed. ::: js/ui/windowManager.js @@ +791,1 @@ + } Nitpick: blank line should go after closing bracket, not before.
Review of attachment 244633 [details] [review]: LGTM
(In reply to comment #20) > To make my workflow easier, I'm going to wait for review on the > switch-to-workspace-n patch before submitting the last (3/3) patch to add > "switch to last workspace" 3/4 - I also want a patch to add support for switch-to-workspace-last to gnome-shell :-)
Created attachment 244637 [details] [review] windowManager: add switch-to-workspace-last keybinding [3/3] [3/3] because I consider the three patches to add switch-to-last a seperate patchset than the one to show the OSD on switch-to-n (grouped them by function and not by dependencies)
Review of attachment 244637 [details] [review]: ::: js/ui/windowManager.js @@ +121,3 @@ Shell.KeyBindingMode.OVERVIEW, Lang.bind(this, this._showWorkspaceSwitcher)); + this.setCustomKeybindingHandler('switch-to-workspace-last', move-to-workspace-last! @@ +781,3 @@ let direction; + if (isNaN(target) && target != 'last') { You can keep the condition as-is if you move the target == 'last' block to the top
Created attachment 244644 [details] [review] windowManager: add switch-to-workspace-last keybinding [3/3]
Comment on attachment 244632 [details] [review] WindowManager: Show switcher popup for switch-to-workspace-n keybindings Pushed the first patch to get it off the list
Created attachment 244645 [details] [review] Keybindings: add switch to last/first workspace keybindings [1/3] This also adds move to the schema.
Review of attachment 244644 [details] [review]: I've squashed the whitespace change in the first patch, looks good otherwise - thanks!
(In reply to comment #28) > This also adds move to the schema. Oh, sorry - I forgot that "move-to-last" is a bit special when reviewing the shell patch; what do the designers say? (If it's something we want, the patch looks good; might want to consider using <shift><super>... as default values for move-to-first/last btw)
Created attachment 244647 [details] [review] workspacesView: Adapt to changes in windowManager The actionMoveWorkspace() function was changed in commit 8727661c1, adapt its use in workspacesView as well. Whoops, actionMoveWorkspace() is actually used elsewhere as well - sorry I didn't notice that earlier in review!
*** Bug 700810 has been marked as a duplicate of this bug. ***
Review of attachment 244647 [details] [review]: OK.
Comment on attachment 244647 [details] [review] workspacesView: Adapt to changes in windowManager Attachment 244647 [details] pushed as e0a6a62 - workspacesView: Adapt to changes in windowManager
can we close this bug?
There are still uncommitted patches here, so I guess not - Elad? (In reply to comment #30) > Oh, sorry - I forgot that "move-to-last" is a bit special when reviewing the > shell patch; what do the designers say? I guess that's what we are blocking on. In case the comment above is not clear, "move-to-last" moves a window to the last workspace, which will then result in a new empty workspace being created at the end - which of course means that the window has not been moved to the last workspace. But some months later I don't think it's too confusing - the last (empty) workspace is kind of special anyway ...
Review of attachment 244645 [details] [review]: LGTM
(In reply to comment #37) > Review of attachment 244645 [details] [review]: > > LGTM I won't be home until Thursday, can you please push those patches for me?
(In reply to comment #36) > There are still uncommitted patches here, so I guess not - Elad? > > > (In reply to comment #30) > > Oh, sorry - I forgot that "move-to-last" is a bit special when reviewing the > > shell patch; what do the designers say? > > I guess that's what we are blocking on. In case the comment above is not clear, > "move-to-last" moves a window to the last workspace, which will then result in > a new empty workspace being created at the end - which of course means that the > window has not been moved to the last workspace. > But some months later I don't think it's too confusing - the last (empty) > workspace is kind of special anyway ... Ups I saw it wrong. I thought the other ones was obsoletes since the commited one was the last. Sorry, obviously it isn't.