GNOME Bugzilla – Bug 674104
Show the workspace switcher when moving a window
Last modified: 2012-06-27 18:08:53 UTC
Currently, when moving a window to a different workspace with <Ctrl><Alt><Shift>Arrow, the actual moving is animated but we don't show the workspace switcher. This can be confusing, because if the original window is fullscreen, you have no idea where you come from and where you ended up. Compare this to Unity/Compiz, they instead show the switcher for all keyboard initiated movements. Patches coming (many of them, because some minor mutter changes are needed)
Created attachment 212045 [details] [review] Make meta_workspace_get_neighbor() public There is no need for this function to be private, and it can greatly simplify gnome-shell code handling workspace switch.
Created attachment 212046 [details] [review] Make it possible to reimplement move-to-workspace keybindings from plugins Export the necessary functions so that a plugin that wishes to do so can reimplement those keybindings without loss of functionality.
Created attachment 212047 [details] [review] WorkspaceSwitcher: simplify code for handling keybindings Most of code implementing workspace switches was repeated with minor differences on each direction. Instead, consolidate it and use the new meta_workspace_get_neighbor.
Created attachment 212048 [details] [review] WindowManager: handle move-to-workspace keybindings Install a custom handler for move-to-workspace-* keybindings that shows the workspace switcher, which gives the user a sense of direction when navigating with the keyboard.
Review of attachment 212046 [details] [review]: Uh, we can already move windows to certain workspaces from the Shell. That should be all that's needed here, right? I agree that change_workspace is a lot easier, but I don't understand the stuff about mouse_mode.
Hah, this is quite similar to bug #660839. Of course, the patches in there were never updated for the new keybindings work, but I think they're still worth a look.
Review of attachment 212045 [details] [review]: Sure.
Review of attachment 212047 [details] [review]: Wow, much better.
Review of attachment 212048 [details] [review]: So, what I did before was something like: let [action, to, workspace, direction] = binding.split('-'); const MOTION_DIRECTIONS = {'up': Meta.MotionDirection.UP, 'down': Meta.MotionDirection.DOWN}; let motionDirection = MOTION_DIRECTIONS[direction]; if (action === 'switch') this._switchWorkspace(display, window, motionDirection); else if (action === 'move') this._moveWorkspace(display, window, motionDirection); I think that would be cleaner than something like this.
(In reply to comment #5) > Review of attachment 212046 [details] [review]: > > Uh, we can already move windows to certain workspaces from the Shell. That > should be all that's needed here, right? I agree that change_workspace is a lot > easier, but I don't understand the stuff about mouse_mode. I'm not sure either, I just wanted to make sure that gnome-shell code did exactly the same as metacity original code, to avoid weird regressions. Anyway, as I understand it, mutter can sometimes decide to ignore enter/leave events in sloppy/mouse focus modes (for popups?), but then it needs to reset that, once the user has made its choice with the keyboard.
So, what was the outcome of this bug / what needs to change before merging? (No hurry, just cleaning up the queue)
I don't quite understand the mouse mode stuff, so I'd like to have Owen take a look at it. Otherwise, I think rebasing the patches in my bug that I linked above is fine. You can commit the ACN patches.
Comment on attachment 212045 [details] [review] Make meta_workspace_get_neighbor() public Attachment 212045 [details] pushed as e31f55e - Make meta_workspace_get_neighbor() public
(In reply to comment #12) > I don't quite understand the mouse mode stuff, so I'd like to have Owen take a > look at it. > > Otherwise, I think rebasing the patches in my bug that I linked above is fine. > You can commit the ACN patches. It seems that patch #1 there is above some unreferenced work to handle move-to-worskspace-* keybindings, so I'm rebasing it on top of patch #4 here.
Review of attachment 212048 [details] [review]: ::: js/ui/windowManager.js @@ +595,3 @@ + _handleMoveWindow: function(display, screen, window, binding) { + if (screen.n_workspaces == 1) + return; This won't ever happen. @@ +598,3 @@ + + if (this._workspaceSwitcherPopup == null) + this._workspaceSwitcherPopup = new WorkspaceSwitcherPopup.WorkspaceSwitcherPopup(); Why are we doing this here? @@ +601,3 @@ + + if (binding.get_name() == 'move-to-workspace-up') + this.actionMoveWindow(window, Meta.MotionDirection.UP); Again, I'd like to see a bit more genericism: let [action, to, workspace, direction] = binding.get_name().split('-'); direction = Meta.MotionDirection[direction.toUpperCase()]; if (action == 'move') // ... else if (action == 'switch') // ... @@ +616,3 @@ + if (activeWorkspace != toActivate) { + /* This won't have any effect for "always sticky" windows + (like desktop windows or docks) */ We use // style comments in the shell.
(In reply to comment #15) > Review of attachment 212048 [details] [review]: > > ::: js/ui/windowManager.js > @@ +595,3 @@ > + _handleMoveWindow: function(display, screen, window, binding) { > + if (screen.n_workspaces == 1) > + return; > > This won't ever happen. Unless you enable static workspaces.
(In reply to comment #15) > Review of attachment 212048 [details] [review]: > > @@ +598,3 @@ > + > + if (this._workspaceSwitcherPopup == null) > + this._workspaceSwitcherPopup = new > WorkspaceSwitcherPopup.WorkspaceSwitcherPopup(); > > Why are we doing this here? Uhm... I just copied what the keybindings for switch-workspace do (and showing the switcher is the whole point of this bug)
(In reply to comment #17) > Uhm... I just copied what the keybindings for switch-workspace do (and showing > the switcher is the whole point of this bug) Just curious why it was there instead of in _actionMoveWorkspace, is all. (In reply to comment #16) > Unless you enable static workspaces. get_neighbor should return the same workspace anyway, so I don't think it matters.
Review of attachment 212048 [details] [review]: Although maybe the genericism is a bit too much and too early. Hm...
Created attachment 217433 [details] [review] WindowManager: handle move-to-workspace keybindings Install a custom handler for move-to-workspace-* keybindings that shows the workspace switcher, which gives the user a sense of direction when navigating with the keyboard.
Review of attachment 217433 [details] [review]: Yep. This works for me.
(In reply to comment #21) > Review of attachment 217433 [details] [review]: > > Yep. This works for me. Wait... what about the mouse_mode stuff?
See how-to-get-focus-right.txt and bug #167545 . I don't know why it happens only with a flip in the regular keybinding handler, rather than all the time.
(In reply to comment #23) > See how-to-get-focus-right.txt and bug #167545 . I don't know why it happens > only with a flip in the regular keybinding handler, rather than all the time. Ok, I read this as "mouse_mode must be cleared when handling the keybinding".
Comment on attachment 212046 [details] [review] Make it possible to reimplement move-to-workspace keybindings from plugins Attachment 212046 [details] pushed as f65b7c5 - Make it possible to reimplement move-to-workspace keybindings from plugins
Attachment 212047 [details] pushed as de72065 - WorkspaceSwitcher: simplify code for handling keybindings Attachment 217433 [details] pushed as 04dbf15 - WindowManager: handle move-to-workspace keybindings