GNOME Bugzilla – Bug 568753
Allow windows to be dragged to other workspaces
Last modified: 2009-01-23 19:23:59 UTC
Here's an initial attempt at dragging windows between workspaces. Some things on my mind with this patch: - There's no attempt to handle the the window title overlay and they get left all over the place. Probably the right thing to do is to hide it when beginning the drag. - When you drop a window, it doesn't animate from the spot where you drop it to its final posiiton in the new workspace (suprisingly hard to notice) - In adding ad-hoc properties to the clone actor, I took a different approach from the existing code and _prefixed them. I think adding the ad-hoc properties at all is likely wrong, since it could lead to really strange stuff going on if they clash with real ClutterActor properties/methods. if we do add them, they probably need to be _shell_<blah> We might want a window "peer" object for the actor instead, corresponding what we have for Workspace. (In combination with MetaWindow and MutterWindow, this gets pretty confusing.) Also relates to whether we want to stop using clones eventually. - I access the private workspace property _workspaceNum in one place from outside the object (just kill the _, I think) - Rather than special casing moving from the overlay, we probably should notice windows being added/removed to workspaces more generally. Not completely sure how that works. (animation from drop-location also comes into play here) - addWorkspace and addWorkspaceClone are too similar names for methods that do very different things. (Had to refactor out the common code for adding the workspace clone/peer since I was adding a signal connection)
Created attachment 127034 [details] [review] Allow windows to be dragged to other workspaces - Hook up mouse-dragging for window actors - Add a ::window-dragged signal to Workspace - Connect to ::window-dragged for each workspace, and compute the new workspace. - Add Workspace.addWindow()/removeWindow() to add and remove windows from the workspace on the fly. (Probably a temporary workaround for not handling this more generally, though we may need to be able to add a window giving an initial position/scale for the window.)
(In reply to comment #0) > - There's no attempt to handle the the window title overlay and they get > left all over the place. Probably the right thing to do is to hide it when > beginning the drag. Yeah > - When you drop a window, it doesn't animate from the spot where you drop > it to its final posiiton in the new workspace (suprisingly hard to notice) Hm... you just need to make the new clone get created with the correct x and y. So Workspaces._onWindowDragged would pass in the translated/scaled coordinates to Workspace.addWindow, which could then either (a) actually re-position the window there before cloning it (such that when you left the overlay the window would be on the spot on the screen where you'd dropped it), or (b) pass optional args to _addWindowClone to create the clone somewhere other than where the window is. Related to this, we need the zip-back animation when you drop somewhere illegal. > - In adding ad-hoc properties to the clone actor, I took a different approach > from the existing code and _prefixed them. I think adding the ad-hoc > properties at all is likely wrong, since it could lead to really strange > stuff going on if they clash with real ClutterActor properties/methods. > > if we do add them, they probably need to be _shell_<blah> Clutter shouldn't be adding any properties whose names start with "_" though, and since the objects are private to workspaces.js, we shouldn't have to worry about any other parts of the code adding conflictingly-named properties either... > We might want a window "peer" object for the actor instead Yeah, probably. We've already got to keep track of window, clone, and title... > - I access the private workspace property _workspaceNum in one place from > outside the object (just kill the _, I think) Yup > - addWorkspace and addWorkspaceClone are too similar names for methods > that do very different things. addWorkspace could become workspacedAdded? + // Reposition all windows in their zoomed-to-overlay position. We really need a lawyer-friendly synonym for "Exposé™"... + let tweenProperties = { ... + if (workspaceZooming) + tweenProperties['workspace_relative'] = this; If you added "if (!workspace) return [];" to the start of _workspace_relative_modifier(), then you could just do: Tweener.addTween(... workspace_relative: workspaceZooming ? this : null, ...); (or s/null/false/?) + let i = 0; + + for (; i < this._windows.length; i++) { Ew. Is this just because otherwise js2-mode incorrectly highlights the second "for (let i" as an error?
(In reply to comment #2) > (In reply to comment #0) > > - When you drop a window, it doesn't animate from the spot where you drop > > it to its final posiiton in the new workspace (suprisingly hard to notice) > > Hm... you just need to make the new clone get created with the correct x and y. > So Workspaces._onWindowDragged would pass in the translated/scaled coordinates > to Workspace.addWindow, which could then either (a) actually re-position the > window there before cloning it (such that when you left the overlay the window > would be on the spot on the screen where you'd dropped it), or (b) pass > optional args to _addWindowClone to create the clone somewhere other than where > the window is. > > Related to this, we need the zip-back animation when you drop somewhere > illegal. This is going to be *so* much easier than it was for GtkDND :-) > > - In adding ad-hoc properties to the clone actor, I took a different approach > > from the existing code and _prefixed them. I think adding the ad-hoc > > properties at all is likely wrong, since it could lead to really strange > > stuff going on if they clash with real ClutterActor properties/methods. > > > > if we do add them, they probably need to be _shell_<blah> > > Clutter shouldn't be adding any properties whose names start with "_" though, > and since the objects are private to workspaces.js, we shouldn't have to worry > about any other parts of the code adding conflictingly-named properties > either... I think somewhere in GTK+ I used a -prefixed property for something private, but certainly it isn't normal. My main concern is more that ad-hoc adding of _properties is fine for native objects but bad for JS implemented objects, so its pretty inconsistent. > > We might want a window "peer" object for the actor instead > > Yeah, probably. We've already got to keep track of window, clone, and title... > > > - I access the private workspace property _workspaceNum in one place from > > outside the object (just kill the _, I think) > > Yup > > > - addWorkspace and addWorkspaceClone are too similar names for methods > > that do very different things. > > addWorkspace could become workspacedAdded? Isn't addWorkspace incrementWorkspaceCount? workspaceAdded sounds like > + // Reposition all windows in their zoomed-to-overlay position. > > We really need a lawyer-friendly synonym for "Exposé™"... Yeah. (Just to be clear, in case any lawyers are listening in, what we do issimilar to Exposé in general effect but in no means identical :-) > + let tweenProperties = { > ... > + if (workspaceZooming) > + tweenProperties['workspace_relative'] = this; > > If you added "if (!workspace) return [];" to the start of > _workspace_relative_modifier(), then you could just do: > > Tweener.addTween(... > workspace_relative: workspaceZooming ? this : null, > ...); > > (or s/null/false/?) Hmm, true. I had some hestitation because we are actually workspace relative, it's simply that the workspace-relative code can't handle the workspace-not-moving case. > + let i = 0; > + > + for (; i < this._windows.length; i++) { > > Ew. Is this just because otherwise js2-mode incorrectly highlights the second > "for (let i" as an error? No, I actually wanted a loop control variable that wasn't scoped to the the loop. But since that wasn't obvious, I guess I should rewrite it to be less clever :-)
Created attachment 127050 [details] [review] Allow windows to be dragged to other workspaces - Made the window animate into place from the dragged location properly - Added snap-back - Made the clone title handling cleaner by putting the "should it be showing" logic in one place. (There's a bug you may notice with the snap-back - the title may temporarily reappear until you move the mouse - this is because clutter doesn't send enter/leaves when actors move, only when the mouse moves.) - Cleanup and some more comments - Made _removeWindow() looping more straight-forward
ok, visually everything looks great now. Maybe SNAP_BACK_ANIMATION_TIME could be a bit faster? And maybe when rearranging windows after a drag, they should move faster than Overlay.ANIMATION_TIME? Codewise: > > Ew. Is this just because otherwise js2-mode incorrectly highlights the second > > "for (let i" as an error? > > No, I actually wanted a loop control variable that wasn't scoped to the the > loop. But since that wasn't obvious, I guess I should rewrite it to be less > clever :-) Ah, I probably would have gotten that if you'd done: let i; for (i = 0; ... lifting the i=0 out as well made it look weird and then I didn't even look carefully enough to see what you were actually doing. _updateCloneTitle's completingTweenCount param makes me cry. If the suggestion you have commented there won't work, another slightly-less-awful-than-now possibility would be to store the value of Tweener.getTweenCount() at onStart time and compare at onComplete, rather than hardcoding it into onCompleteParams. (Which makes me notice that you could actually have _positionWindows figure out workspaceZooming itself by checking Tweener.getTweenCount(this.actor). If you wanted to. For that matter, _workspace_relative_modifier could check that...) If the other clone title bug is annoying to fix I'd say just ignore it for now since we'll probably end up rewriting all of it again soon anyway. :) + // display for changes to the window state that have already been made + // x/y/scale are used to give an initial position for the window (if the (Need a period at the end of the first line) + metaWindow.change_workspace_by_index(targetWorkspace._workspaceNum, You were going to de-underscore _workspaceNum.
- Fixed _workspaceNum - Switched to using a manually maintained clone._animationCount; certainly much cleaner that way. - Added missing periods - Tweaked down the snapback to 0.25 (didn't change the settling time for a drag, but mostly because of laziness... I'd have to pass it in to positionWindows). The overlay probably comes in a bit too slow anyways. There's good refactoring that could be done here by adding a WorkspaceWindow - we are passing around the clone a whole lot in the workspace code because we have all these Workspace methods that are really window methods. Committed.