GNOME Bugzilla – Bug 607821
Slide the linear overview when dragging a window
Last modified: 2010-09-24 16:24:05 UTC
In the new linear workspace overview, it's hard to drag and drop an open window between workspaces. On IRC, Jon proposed to slide (flip) the workspace when you drag a window to the left or right edge of the screen. The left edge might be a problem because the dash is there, but on the other hand there would be no other reason to dnd a window onto it. Compiz seems to do more or less the same, with the cube.
Note that this might conflict with bug 606260.
(In reply to comment #1) > Note that this might conflict with bug 606260. Forget it - bug 606260 refers to the normal working area, this is about the overview ... /me slaps himself
Created attachment 152381 [details] [review] Slide the linear overview when dragging a window Change SingleView._scroll.adjustment.value, when dragging windows.
The patch works quite well and the animation seems more or less natural. Perhaps a bit TOO snappy / fast when the worskapces begin to move _immediately_ after grabbing an item and zipping around. I think this patch ought to snap to the dropped-to workspace: Letting a miniature window go on another workspace, more often than not, leaves the overview scrolled in between two workspaces, sometimes with the moved item out of view. Thanks a lot for this, Maxim.
Review of attachment 152381 [details] [review]: A few comments ::: js/ui/dnd.js @@ +215,3 @@ if (this._dragActor) { // If we are dragging, update the position + this.emit('drag-motion', stageX - this._dragStartX, stageY - this._dragStartY); This should be a separate patch I think, like: [dnd] Add a drag-motion signal ::: js/ui/workspacesView.js @@ +630,3 @@ + this._inDrag = true; + this._lastDx = 0; + workspace.connect('window-drag-begin', Lang.bind(this, function (w) { Why are we incrementing the adjustment here? @@ +638,3 @@ + this._inDrag = true; + this._lastDx = 0; + workspace.connect('window-drag-begin', Lang.bind(this, function (w) { Hmm, so we're trying to map motion to how many workspaces to move? This is going to be weird - if I happen to move 200 pixels between events I'll move two workspaces? I think this would work better if we got the geometry (get_transformed_position()) of the workspace, and if we're on say the left 1/3 we move one left, if we're on the right 1/3 we move right.
Created attachment 153604 [details] [review] Slide the linear overview when dragging a window add two dnd target in SingleView. if WindowClone drop to it, than Window will move to right/left workspace.
Not a bad idea. Though I sorta wonder if we can't just scroll over to the next workspace a little bit. Just enough to drop the window. If we can't do that then this is probably OK. However, I think I'd prefer the drop target to be half as wide as it is here. Also we can use the arrow in https://bugzilla.gnome.org/show_bug.cgi?id=609187 . I'd also be interested to see if we can do something similar to create a new workspace when there isn't one already there. So, if I drag a window over to the right and there isn't a workspace there, it might be nice to have a similar drop target and have a "+" sign on it or something.
Created attachment 154098 [details] [review] Slide the linear overview when dragging a window Corrected patch.
Oh nice you were able to do the + thing. A few random thoughts about the behavior - and then at the end some recommendations. * The drop targets are still a bit too "bold". Perhaps they should stay a bit more transparent until a droppable object is over them. * It seems a bit hard to tell when the mouse and dragged window is over the drop target. We don't change the cursor or anything and the window is still fully opaque so it is hard to target correctly. Perhaps we can try doing some prelight on the drop target and make the dragged window slightly translucent? Kinda like nautilus does when dragging files I guess. * It is a little odd that if I drag a window and move all the way to the right edge of the screen and release it doesn't drop onto the target. I see why this is the case - because the workspace doesn't extend to the edge but we should probably still accept the drop in this case. Or perhaps better - just immediately perform the action when you bang into edge of the screen. * It seems like if I go into grid view, create a workspace, and then remove the workspace that the drop targets don't appear. This is probably because we are still in grid view even though there is only one workspace. I guess this is a separate issue. * We'll want to get a new "+" icon. * Even if we do the scroll automatically thing, I suppose the arrow overlay is still interesting because it makes it pretty clear what the options are. * I'm wondering if longer term we want to require actively engaging the reorder/position mode - say click and hold to engage reposition mode. This may allow us to do swipe scrolling more easily. I think this may be ok since repositioning and "management" is expected to be much less frequent than switching workspaces or looking for windows. OK, to summarize: * Change cursor when dragging * Make window say 50% opaque when dragging (just picked a value here) * Have the drop targets + icons more transparent and with no stroke when not active * Make drop targets look basically like they are now when an object is over them * Automatically perform the action when the object hovers over the target for a second or so (again just picked a value) * Automatically peform action when the left or right side of the screen is touched with the dragged object * When automatically performing the action, continue the drag operation. In other words move the workspace view with the object. * When the object is dropped on the target, don't change the workspace view (like it is now) Thoughts?
Created attachment 154300 [details] [review] Implement opacity for StWidget
Created attachment 154301 [details] [review] Add a drag-motion signal
Created attachment 154302 [details] [review] add ability to move window in SingleView (need apply patch from https://bugzilla.gnome.org/show_bug.cgi?id=610575) > * Automatically peform action when the left or right side of the screen is > touched with the dragged object Some object can be very big. May be automatically perform action when the left or right side of the screen is touched with mouse cursor?
> Some object can be very big. May be automatically perform action when the left > or right side of the screen is touched with mouse cursor? Oops, yeah that's what I meant to say.
Created attachment 154361 [details] [review] Add a drag-motion signal
Created attachment 154362 [details] [review] add ability to move window in SingleView automatically perform action when the left or right side of the screen is touched with mouse cursor. (need apply patch from https://bugzilla.gnome.org/show_bug.cgi?id=610575)
Review of attachment 154300 [details] [review]: This would be cool to have, but I'm not sure it actually works - as you've implemented it here, there will be weird ordering dependencies depending on whether the app sets the opacity and then the style is computed, or the style is computed and then the app sets the opacity. Ideally, opacity set in CSS would multiply clutter_actor_set_opacity(). Don't see how to do this without Clutter changes. (The obvious ugly technique of getting the opacity at the start of st_widget_paint(), modifying the opacity, then restoring the original opacity at the end doesn't work since clutter_actor_set_opacity() calls clutter_actor_queue_redraw())
Trying this out, it looks really great visually, but the interaction feels odd. In particular: * Until I release the mouse button, the drop should not end. * The two distinct behaviors with: - The big transparent drop zones - When you hit the edge of the screen are going to make it very hard for people to figure out. My suggestion for UI would be more along the lines of: - When you start dragging, small arrows show at the sides of the screen. I'm thinking semi-transparent arrows centered vertically at the edges of the screen and maybe 60 pixels in size. They would only appear on sides where there are more workspaces. - Dragging to the very edge of the screen flips between workspaces but does not drop the window, allowing you to move a window across multiple workspaces, or back to the original one. - When you drag to the right of the last workspace, a [+] drop box appears, and if drop there, your window goes on a new workspace) [ A possibility would be that instead of having no -> on the right edge of the last workspace, you have a + to hint the drop-to-add-workspace behavior, but I think it's better to let people discover that serendipitously ]
Review of attachment 154361 [details] [review]: I'm OK with the drag-motion addition, but the dropActor method is not a good idea from a user interface perspective. As long as the mouse button is down, you must not drop.
Review of attachment 154362 [details] [review]: Haven't looked at the code yet, but marking as needs-work, because I think the UI needs some more evolution, and I dont' think it's worth digging into the details of the code until that happens. (To reiterate, I think the UI is really cool looking and slickly done, I'm just not sold on the details of the interaction.)
(In reply to comment #16) > Review of attachment 154300 [details] [review]: > > This would be cool to have, but I'm not sure it actually works - as you've > implemented it here, there will be weird ordering dependencies depending on > whether the app sets the opacity and then the style is computed, or the style > is computed and then the app sets the opacity. If style have 'opacity' property, we can ignore app request.
(In reply to comment #17) Yeah that is basically what I was trying to convey. In addition to the other behaviors listed above, the dragged window should probably be more a fixed size. Let's use the same window size that we have in the alt-tab window box now. Adding to what Owen said: - When you start dragging, small arrows fade in at the sides of the screen. I'm thinking semi-transparent arrows (or plus sign) centered vertically at the edges of the screen and maybe 60 pixels in size. They would only appear on sides where there are more workspaces (or when one may be added). - When hovering over the drop target area the arrow or plus icon should prelight. The transition from normal to prelight should be smooth. - When hovering in the drop target area we should add a prelight gradient to the edge of the workspace. - Dragging to the very edge of the screen flips between workspaces but does not drop the window, allowing you to move a window across multiple workspaces, or back to the original one. - Hovering over the drop target area should activate the action after a short time. - As long as the pointer remains over a drop target, continue to traverse workspaces after each hover timeout expires. This allows one to move a single window across several workspaces easily. - When dragging a window it should smoothly zoom to a smaller size window. Similiar in size to the Application Switcher's window list. - The window should be 50% opaque while being dragged. - Dragging a window (in single view) should begin only after a 500ms click and hold. This is in order to optimize selecting windows and switching workspaces. - (for now) We should only add one additional workspace per drag operation. In other words, after the first "+" action is encountered we don't offer another one. - As an affordance for dropping the dragged window we should add an empty position to array of windows on the visible workspace. - When you drag to the right of the last workspace, a [+] drop box appears, and if drop there, your window goes on a new workspace) (Will play with the visuals and come back with more...)
Created attachment 154734 [details] [review] add ability to move window in SingleView (need apply patch from https://bugzilla.gnome.org/show_bug.cgi?id=610575) > - (for now) We should only add one additional workspace per drag operation. > In other words, after the first "+" action is encountered we don't offer > another one. In next drag? If yes, I think It related with bug "don't add more then one empty workspace".
(In reply to comment #22) > Created an attachment (id=154734) [details] [review] > add ability to move window in SingleView > > (need apply patch from https://bugzilla.gnome.org/show_bug.cgi?id=610575) > > > - (for now) We should only add one additional workspace per drag operation. > > In other words, after the first "+" action is encountered we don't offer > > another one. > > In next drag? If yes, I think It related with bug "don't add more then one > empty workspace". The left arrow is on the left screen edge rather than on top of the workspace actor which looks very odd (i.e it is hovering over the dash instead of the workspace edge).
Created attachment 154959 [details] Mockup for adding workspace
Created attachment 154960 [details] Mockup for moving window to workspace
Created attachment 155012 [details] [review] add ability to move window in SingleView Mockup implementation
Wow that was fast :) Overall looks pretty nice. The zoom out seems to work pretty well. Comments: * The stroke on the + target box seems to have a non-uniform width. The left side is different from the top and bottom. * The stroke color on the + target box is (255,255,255,204) in the mockup. * The + icon is pretty hefty compared to the one in the mockup. * The stroke for the + box should fade out into the background (-> transparent) * We should hide the window caption boxes when a window is dragging * When hovering over one of the targets with the dragged window we should activate the action after .5 second or so. * I think should only start dragging after clicking and holding for a bit (.5 second?) - maybe this is a separate bug * The stroke color on the + target box should match the fill color for the + icon. * The stroke color on the + target box should change on hover/prelight as in the mockups * A workspace that appears on the left or right side as a drop target should fade out into the background (->transparent) away from the current workspace (for both prelight and normal states) * There seems to be a bug when I drop a window on another workspace. Sometimes when I drop the window on the other workspace drop target and then switch using the controls under the workspace area the window that was just moved seems too large and doesn't fit in the grid. * The prelight on the other workspace drop target doesn't really match the mockup. It seems to wash out the color instead of adding saturation and brightness. It also doesn't look like a linear gradient to me.
(In reply to comment #27) > * When hovering over one of the targets with the dragged window we should > activate the action after .5 second or so. Does 'activate the action' mean scroll right/left? If no, It conflict with comment 17. >> * Until I release the mouse button, the drop should not end.
Created attachment 155142 [details] [review] add ability to move window in SingleView
Nice! A few comments: * Hovering for say 500ms over the drop zones should have the same effect as hitting the screen edge. * The plus icon has a funny overlap at the center. I'll try to export an icon for you. * The stroke on the + drop area still looks a bit strange on the left vs. top/bottom. * The prelight gradient on the + drop zone seems to have a lot of banding on my system. * When a workspace has only one window, and I start to drag it, it becomes really large just as it becomes translucent. * Sometimes, but not always, when I drop a window on one of the drop zones and then switch to that other workspace manually, the window doesn't fit into the "grid" and is too large.
(In reply to comment #29) > Created an attachment (id=155142) [details] [review] > add ability to move window in SingleView It require to apply patch from https://bugzilla.gnome.org/show_bug.cgi?id=610575
Review of attachment 155142 [details] [review]: Would prefer to see the changes to dnd.js as a separate patch. I'm just doing a partial review of that. Can you explain in that new patch exactly what you're trying to do there? ::: js/ui/dnd.js @@ +167,3 @@ + y = stageY - this._dragActor.height / 2; + x = stageX - this._dragActor.width / 2; + let x, y, w, h; Since this is a new variable, please set this._dragActorOrigin = null in startDrag() probably. @@ +185,3 @@ } else { + this._dragOffsetY = this._dragActor.y - this._dragStartY; + this._dragOffsetX = this._dragActor.x - this._dragStartX; This is weird....let me attach a patch which cleans this part up in the original code.
Created attachment 155356 [details] [review] Clean up definition of dragOffsetX, dragOffsetY These variables were computed strangely; through cancellation they'd end up being negative 1/2 the width/height of the actor, so just use that directly.
Review of attachment 155356 [details] [review]: It's only centered on the actor in the case where we can't do better - where the drag start position is outside the bounds of the dragged object - normally the drag offset depends on where you click.
Created attachment 155369 [details] [review] [dnd] Use mouse position as argument to handleDragOver
Created attachment 155370 [details] [review] [dnd] If dragActor has different size, show resize animation && use its size for calculate dragOffset
Created attachment 155371 [details] [review] Resize window preview to altTab.THUMBNAIL_SIZE when dragging
Created attachment 155504 [details] [review] add ability to move window in SingleView Difference with previous patch: 1. Hovering for say 500ms over the drop zones have the same effect as hitting the screen edge. 2. DND part is separate part. It still has bad '+' icon.
Created attachment 155509 [details] [review] add ability to move window in SingleView Merge with HEAD
Review of attachment 155369 [details] [review]: This looks right to me. Can I suggest this for the commit message: [dnd] Look at current mouse position for dropping Previously we used the top-left corner which was not intuitive, particularly when dragging partially opaque actors.
As mentioned on IRC yesterday: * Dropping on the edge of the screen over the + target doesn't work * The prelight on a dark background isn't visible Otherwise I think it looks good.
Created attachment 155767 [details] [review] [dnd] Ensure our drag actor is positioned over the cursor Previously we were looking at the source size, which is not necessarily the same as the drag actor size.
Created attachment 155768 [details] [review] Resize window preview to altTab.THUMBNAIL_SIZE when dragging
Created attachment 155775 [details] [review] [workspaces] Shrink windows to small previews when dragging Hide the original actor during a grab, and create a new clone. This is easier than trying to ensure we maintain the state of the original window clone.
Created attachment 155782 [details] [review] [workspaces] Shrink windows to small previews when dragging Hide the original actor during a grab, and create a new clone. This is easier than trying to ensure we maintain the state of the original window clone.
Review of attachment 155509 [details] [review]: Comments follow... ::: js/ui/dnd.js @@ +265,2 @@ this._dragActor.show(); dropX, dropY); Hmm this is a pretty fundamental change; why can't you just make the drop targets for the workspaces reactive? ::: js/ui/workspace.js @@ +623,3 @@ _init : function(workspaceNum, parentActor) { Workspace.prototype = { + this._reservedSlot = null; Can you add a comment here what this is used for? Like: "When dragging a window, we use this slot for the original position." @@ +1024,3 @@ + + let buttonOuterWidth = 0; + let buttonOuterHeight, captionHeight; I don't quite understand the original code here, but this change is OK since it's just an extension from the original. @@ +1041,3 @@ }, + if (clone && this.containsMetaWindow(clone.metaWindow)) { + setReservedSlot: function(clone) { Shouldn't this be if (!clone) ? ::: js/ui/workspacesView.js @@ +35,3 @@ const WORKSPACES_VIEW_KEY = 'overview/workspaces_view'; + +const SIZE_WHEN_DRAG = 0.85; Maybe WORKSPACE_DRAGGING_SCALE? @@ +476,3 @@ +} + this._init(); +function NewWorkspaceArea() { I don't understand the magic 0.001 constants here... @@ +486,3 @@ +} + this._init(); +function NewWorkspaceArea() { Why are these two separate? Can't you combine them? It should work to have both a border and a gradient together. @@ +495,3 @@ + if (isHover) + str = 'hover'; + this._child1.set_style_pseudo_class(str); This might be a little cleaner as: this._child1.set_style(isHover ? 'hover' : null) @@ +511,3 @@ + this._leftShadow = new St.Bin({ style_class: 'left-workspaces-shadow', + this._newWorkspaceArea = new NewWorkspaceArea(); + let s = (1 - SIZE_WHEN_DRAG) / 2; More magic 0.001 here... @@ +539,3 @@ + this._dropGroup._delegate = this; + this._dropGroup = new Clutter.Group({ x: 0, y: 0, width: global.screen_width, height: global.screen_height }); + Is there an existing actor in say overview.js that might work for this? We probably only want the drop group to be sized to the overview width, not the whole screen too. @@ +575,2 @@ + let _width = this._workspaces[0].actor.width * scale; + this._newWorkspaceArea.scale = scale; This line is duplicated 3 times. @@ +589,1 @@ Seems like you could make this a common function like: _scaleWorkspaceActor(actor, workspaceWidth, scale) { actor.scale = scale actor.gridX = this._x + (this._width - width) ... actor.gridY = ... ]
Created attachment 155964 [details] [review] add ability to move window in SingleView > Why are these two separate? Can't you combine them? It should work to have > both a border and a gradient together. We need gradient over border. > Seems like you could make this a common function like: > _scaleWorkspaceActor(actor, workspaceWidth, scale) { Formulas for gridX are different. > Shouldn't this be if (!clone)? No. This check need for ensure that window DONT belongs to this workspace.
Created attachment 155965 [details] [review] Add ability for set a cursor on the Clutter stage window. Also set GDK_CURSOR_FLEUR when dragging window in SingleView.
Review of attachment 155964 [details] [review]: ::: js/ui/workspacesView.js @@ +35,3 @@ +const WORKSPACE_DRAGGING_SCALE = 0.85; +const WORKSPACE_SHADOW_WIDTH = (1 - WORKSPACE_DRAGGING_SCALE) / 2 + 0.001; This isn't a _WIDTH, it's another _SCALE, right? So call it WORKSPACE_DRAGGING_SCALE. Also instead of .001, can you use Math.ceil() if that's the desired effect? @@ +476,3 @@ +NewWorkspaceArea.prototype = { + _init: function() { + this.actor = new Clutter.Group({ width: global.screen_width * WORKSPACE_SHADOW_WIDTH, So this would be: width: Math.ceil(global.screen_width * WORKSPACE_SHADOW_SCALE)
Created attachment 156221 [details] [review] add ability to move window in SingleView
Review of attachment 155965 [details] [review]: In the big picture I think we should do cursor handling in dnd.js. This means having a negotiation of some sort; like a "canAcceptDrop" method. During motion we call canAcceptDrop, and change the cursor accordingly. ::: src/shell-global.c @@ +747,3 @@ + * @type: A #GdkCursorType + * + * Restart the current process. Only intended for development purposes. Copy & paste error; this needs to be a real description. "Set the cursor on the stage window." @@ +755,3 @@ +{ + ClutterStage *stage = CLUTTER_STAGE (mutter_plugin_get_stage (global->plugin)); + GdkWindow* stage_window = gdk_window_foreign_new (clutter_x11_get_stage_window (stage)); Two things: * First, I think we don't want the _FLEUR cursor for the whole stage, just for the overview area. I'm not sure right now if we have an X11 window that matches the primary monitor area. Does anyone else know? * Probably should cache the GdkWindow
Review of attachment 156221 [details] [review]: Just one small comment otherwise let's commit this. ::: js/ui/panel.js @@ +227,3 @@ let iconWidth = childBox.x2 - childBox.x1; + [minWidth, minHeight, naturalWidth, naturalHeight] = this._label.actor.get_preferred_size(); What is this changing? Just whitespace? If so can you omit it from the patch please?
Comment on attachment 155767 [details] [review] [dnd] Ensure our drag actor is positioned over the cursor getDragActorSource() was intended for the case when you call makeDraggable() on an actor, but you want dragging from it to only drag (a copy of) part of it. Eg, when dragging from an app well button, you only end up dragging the icon, not the whole button. that said, I guess this patch doesn't break that case, so... you might need to modify _cancelDrag too to make it snap back to the right place? > let [sourceWidth, sourceHeight] = this._dragActorSource.get_transformed_size(); this line is unnecessary now
(In reply to comment #53) > (From update of attachment 155767 [details] [review]) > getDragActorSource() was intended for the case when you call makeDraggable() on > an actor, but you want dragging from it to only drag (a copy of) part of it. > Eg, when dragging from an app well button, you only end up dragging the icon, > not the whole button. > > that said, I guess this patch doesn't break that case, so... > > you might need to modify _cancelDrag too to make it snap back to the right > place? In this case, it looks like since the source isn't visible, we'll snap back to [this._dragStartX + this._dragOffsetX, this._dragStartY + this._dragOffsetY;] which works.
Comment on attachment 155782 [details] [review] [workspaces] Shrink windows to small previews when dragging ok, except >+ let scale = Math.min(altTab.THUMBNAIL_SIZE / this.realWindow.width, altTab.THUMBNAIL_SIZE / this.realWindow.height); I think for the most part it's assumed that a file's constants are private to that file even if they're not underscore-prefixed. (In particular, the pending alt-tab changes rename THUMBNAIL_SIZE to DEFAULT_THUMBNAIL_SIZE, and it would never have occurred to me to check if any other file was also using the old name.) So... move this to Main, or a new file, or just create your own local constant.
(In reply to comment #51) > * First, I think we don't want the _FLEUR cursor for the whole stage, just for > the overview area. I'm not sure right now if we have an X11 window that > matches the primary monitor area. Does anyone else know? There's just one X11 window for the entire reactive area of the stage; the cursor won't apply to areas outside the input shape - I think that setting the cursor for that is fine, especially if it's just during a grab operation. Eventually, we may want the ability to set the cursor scoped to a particular actor, but we'd have to do the cursor switching on our end and not count on X to do it for us.
Created attachment 156314 [details] [review] Fix dropping window on the indicators and at the screen edge for "+"
Review of attachment 156314 [details] [review]: Looks good, and works fine here. (The subject is a bit long but well ...)
Created attachment 157360 [details] [review] Add ability for set a cursor on the Clutter stage window. set GDK_CURSOR_FLEUR when dragging. handleDragOver should return boolean. if it return true, set GDK_PLUS.
Review of attachment 157360 [details] [review]: I suggest splitting this patch into: - Adding the ShellGlobal method - Modifying the DND interface The acceptDrop method in appDisplay.js doesn't have a corresponding handleDragOver method here. ::: js/ui/dnd.js @@ +155,3 @@ this._dragInProgress = true; + Main.setCursor(Gdk.CursorType.FLEUR); I think you should use the named cursors: dnd-none dnd-copy dnd-move As appropriate. See gdk_cursor_new_from_name(). For the copy vs. move distinction you might need to make handleDragOver return an "enum" Dnd.DropAction.NONE/COPY/MOVE ::: js/ui/main.js @@ +162,3 @@ } +function setCursor(type) { This is unnecessary with my suggestion for global.unset_cursor() below ::: js/ui/workspacesView.js @@ +1009,3 @@ + + if (x < this._x || y < this._y || y > this._y + this._height) { + this._dropGroup.lower_bottom(); This is OK *for now* but _dropGroup has to go. Filed: https://bugzilla.gnome.org/show_bug.cgi?id=620504 @@ +1227,2 @@ actor._delegate = {}; + actor._delegate.handleDragOver = Lang.bind(this, function(source, actor, x, y) { Write this as actor._delegate = { handleDragOver: Lang.bind(...) { }, ... }; @@ +1402,3 @@ + this._addButton._delegate.handleDragOver = Lang.bind(this, + function(source, actor, x, y) { + return true; Is it really true that this accepts *everything*? I considered that this should be the behavior implemented in dnd.js if you had a acceptDrop but not a handleDragOver, but thought that accepting everything was unlikely to ever be right. ::: src/shell-global.c @@ +752,3 @@ +shell_global_set_cursor (ShellGlobal *global, + gboolean use_parent_cursor, + GdkCursorType type) I think it's better to have set_cursor() and unset_cursor() rather than the user_parent_cursor boolean @@ +754,3 @@ + GdkCursorType type) +{ + static GdkWindow* stage_window = NULL; Use a member of ShellGlobal not a static variable for this. (Since ShellGlobal is a singleton, *any* member of ShellGlobal could be done as a static instead. But that would be messy.) @@ +765,3 @@ + if (use_parent_cursor) + { + GdkCursor* cursor = gdk_cursor_new (type); Style: 'GdkCursor *cursor' not 'GdkCursor* cursor'
Maxim - How do you want to handle the interaction between this and Bug 620504 - Get rid of SingleView._dropGroup? This patch and some of the changes I suggested above will be simpler if we commit 620504 first. However, you have first rights to commit something. This bug is older. :-) Do you want to finish this first, or should we proceed with bug 620504?
(In reply to comment #61) > Do you want to finish this first, or should we proceed with bug 620504? Patch from bug 620504 already ready, so lets commit it first.
Created attachment 165781 [details] [review] add shell_global_[un]set_cursor
Created attachment 165782 [details] [review] [DND] change cursor when draging for now, handleDragOver should return DND.DragMotionResult. We choose cursor based on returns value of handleDragOver's and dragMonitors.dragMotion. > I think you should use the named cursors: > dnd-none, dnd-copy, dnd-move they are not standard cursors. (some themes not include them)
(In reply to comment #64) > Created an attachment (id=165782) [details] [review] > [DND] change cursor when draging > > for now, handleDragOver should return DND.DragMotionResult. > We choose cursor based on returns value of handleDragOver's and > dragMonitors.dragMotion. > > > I think you should use the named cursors: > > dnd-none, dnd-copy, dnd-move > > they are not standard cursors. (some themes not include them) There's no real standard for DND cursor names. The names are defined only by custom. These however are the names that are used by GTK+ these days, and to me, that makes them the standard names. If these names aren't present, GTK+ falls back to internal RGBA images in the sources and ignores the cursor theme. Since I don't want to cut-and-paste gtkdndcursors.h into the gnome-shell sources, if you want to handle cursor themes that don't have these names: - Add 'gboolean shell_global_set_cursor_name()' - If that returns false, fall back to some approximation like you are using currently
Created attachment 165840 [details] [review] add shell_global_[un]set_cursor
Created attachment 165841 [details] [review] [DND] change cursor when draging
Review of attachment 165840 [details] [review]: Looks good to commit with two small changes ::: src/shell-global.c @@ +431,3 @@ + g_return_if_reached (); + } + cursor = gdk_cursor_new_from_name (gdk_display_get_default (), name); I'd use gdk_cursor_new(GdkCursorType) instead of _from_name() here to make it clear that these are defined-to-be-present cursors @@ +454,3 @@ +shell_global_unset_cursor (ShellGlobal *global) +{ + if (!global->stage_window) /* cursor doesn't set */ Change comment to /* cursor has never been set */
Review of attachment 165841 [details] [review]: Typo in the subject 'dragging' not 'draging' > for now, handleDragOver should return DND.DragMotionResult. It was already returning DragMotionResult before this patch, right? The patch looks pretty good to me, though I haven't checked the details of the feedback it gives situation by situation and line by line. ::: js/ui/appDisplay.js @@ +1042,3 @@ + handleDragOver : function(source, actor, x, y, time) { + let app = null; + if (source instanceof AppWellIcon) { Our style is not to add {} for one line if statements. (If, however, one branch of an 'if .... else' has {}, all branches should {}) ::: js/ui/dnd.js @@ +22,3 @@ + NO_DROP: Shell.Cursor.UNSUPPORTED_TARGET, + COPY_DROP: Shell.Cursor.COPY, + MOVE_DROP: Shell.Cursor.MOVE, This is confusing to read. Add a separate DRAG_CURSOR_MAP = { DragMotionResult.NO_DROP: Shell.Cursor.UNSUPPORTED_TARGET, [...] } and do a look up in that before setting the cursor @@ +357,2 @@ } + global.set_cursor(Shell.Cursor.IN_DRAG); This will result in flickering - you need to set the cursor only once @@ +359,1 @@ while (target) { So write while (target && !found_destination) ::: js/ui/workspacesView.js @@ +280,3 @@ + if (source instanceof Workspace.WindowClone) { + return DND.DragMotionResult.MOVE_DROP; + } if (source.shellWorkspaceLaunch) { Something wrong here - seems like you wanted 'else if' @@ +1208,3 @@ this._dragOverLastX = leftEdge; + return DND.DragMotionResult.CONTINUE; If you are returning CONTINUE when you actually are going to accept a drop, then you need to comment it. For example: /* We actually accept the drop here, but it looks funny if we change the * cursor so we return CONTINUE */ (That may not be an accurate comment)
Created attachment 169856 [details] [review] [DND] change cursor when dragging > If you are returning CONTINUE when you actually are going to accept a drop we don't support drop on screen edge > It was already returning DragMotionResult before this patch, right? we does not have any handleDragOver function. Result of handleDragOver does not used, before this patch.
Review of attachment 169856 [details] [review]: Editorial rewrite of commit message body. Return a DND.DragMotionResult constant from delegate _handleDragMotion methods as well as the existing return value from the drag monitor method dragMotion. Use the result to update the drag cursor. OK to commit with that and the following changes: ::: js/ui/dnd.js @@ +23,3 @@ COPY_DROP: 1, MOVE_DROP: 2, + CONTINUE: 0xff Leave this as 3, don't change to 0xff @@ +26,3 @@ +}; + +const DragCursorMap = { Should be DRAG_CURSOR_MAP - unlike DragDropResult or DragMotionResult this isn't a "type" - it's a data structure @@ +354,1 @@ case DragMotionResult.NO_DROP: I think the switch obscures the logic - it's really "anything but continue" if (result != DragMotionResult.CONTINUE) { global.set_cursor(DRAG_CURSOR_MAP[result]); return true; } is not only shorter, it's more readable. (In both places where you have this switch)
Is there any work left on this bug? Looks to me like the bug should have been closed when landing the cursor stuff ...