GNOME Bugzilla – Bug 643319
Workspace thumbnail active area should extend to edge of monitor
Last modified: 2012-03-19 22:48:56 UTC
Currently the area where a mouse click would change the desktop ends some pixel away from the right border. Therefore it is quite easy to click to the right of it instead of on it (since you tend to just move the mouse to the far right). I think this should be changed so that clicking right of the actual preview also switches the desktop, or by moving the preview. The later however might have unwanted aesthetic implications ;).
*** Bug 643094 has been marked as a duplicate of this bug. ***
*** Bug 643676 has been marked as a duplicate of this bug. ***
Correct. The Dash extends to the left edge, it's weird that the workspace thumbnails don't extend to the right one.
*** Bug 650536 has been marked as a duplicate of this bug. ***
Created attachment 201723 [details] [review] fittsify the workspace selector
Created attachment 201977 [details] [review] workspaceThumbnail: extend active area to the edges of the pager Now the thumbnail is reactive for all the pager width. Also added some comments.
Review of attachment 201977 [details] [review]: ::: js/ui/workspaceThumbnail.js @@ +862,3 @@ + // For this padding see comment in this._allocate() + let padding = themeNode.get_padding(St.Side.TOP); It seems like you should be adding the left and right padding here, not the top padding twice.
Created attachment 202768 [details] [review] workspaceThumbnail: extend active area to the edges of the selector We want to catch clicks and drops also on the edge of the monitor
(In reply to comment #8) > Review of attachment 201977 [details] [review]: > > ::: js/ui/workspaceThumbnail.js > @@ +862,3 @@ > > + // For this padding see comment in this._allocate() > + let padding = themeNode.get_padding(St.Side.TOP); > > It seems like you should be adding the left and right padding here, not the top > padding twice. Done. Now it fully respects the theme. Also, I changed the workspace thumbnail actor from StGroup to ShellGenericContainer because now a custom allocation is required for its child (actually the previous patch worked, but it generated Clutter warnings).
Review of attachment 202768 [details] [review]: ::: js/ui/workspaceThumbnail.js @@ +919,2 @@ let thumbnailHeight = portholeHeight * this._scale; + let thumbnailWidth = Math.round(portholeWidth * this._scale + leftPadding + rightPadding); Instead of doing leftPadding + rightPadding everywhere just use themeNode.get_horizontal_padding()
(In reply to comment #11) > Review of attachment 202768 [details] [review]: > > ::: js/ui/workspaceThumbnail.js > @@ +919,2 @@ > let thumbnailHeight = portholeHeight * this._scale; > + let thumbnailWidth = Math.round(portholeWidth * this._scale + > leftPadding + rightPadding); > > Instead of doing leftPadding + rightPadding everywhere just use > themeNode.get_horizontal_padding() As discussed in IRC conversation: <stfacc> drago01: I'm reading your review for bug 643319 <bebot> Bug http://bugzilla.gnome.org/show_bug.cgi?id=643319 normal, Normal, ---, gnome-shell-maint, UNCONFIRMED, Workspace thumbnail active area should extend to edge of monitor <stfacc> actually I considered get_horizontal_padding() <stfacc> but in one place I use leftPadding alone <stfacc> so it just felt more "symmetrical" to go this way <drago01> stfacc: ok good point
Review of attachment 202768 [details] [review]: It appears to me that this likely hasn't been tested with multi-monitor - it removes all the calls to setPorthole and also changes the allocation of the thumbnail to include the horizontal padding. Since we're counting on clip_to_allocation to clip to a single monitor this will cause drawing artifacts. I can think of three approaches: 1. Add an extra layer of nesting - have a container (which handles events and DND) around a separate actor that is responsible for displaying and clipping the workspace contents. The downside of this is that the allocation handling is probably at least as messy as in your patch. 2. Add an extra actor that is beneath the workspace actor whose only responsibility is to handle drops and clicks but isn't nested with the workspace actor. This would avoid most of the layout complexity of this patch but does mean that there will be some awkward code to forward events and dnd around.. 3. Play around with how the workspace actor "picks" - make it reactive outside of its allocation. I'll attach a patch that has the backend implementation for this - you should be able to figure out how to use it without much problem. Although this adds some dependency on clutter details, I think it's best because it has the minimal effect on the already complex code workspaceThumbnail.js. If other shell maintainers cry foul on clutter hackery, my preference would be 2 :-)
Created attachment 203114 [details] [review] Add shell_util_set_pick_borders() Add a (slightly hacky) function that expands the pick area of an actor actor.
Created attachment 203115 [details] [review] Add shell_util_set_pick_borders() With fixed-up comments.
Review of attachment 202768 [details] [review]: I'm much more of a fan of 2., obviously, but if you want to write the code for 3., you can maintain it for life.
(In reply to comment #13) > Review of attachment 202768 [details] [review]: > [...] > 2. Add an extra actor that is beneath the workspace actor whose only > responsibility is to handle drops and clicks but isn't nested with the > workspace actor. This would avoid most of the layout complexity of this patch > but does mean that there will be some awkward code to forward events and dnd > around.. As a variation of this approach, what about using ThumbnailsBox as the only event handler? The current split between WorkspaceThumbnail and ThumbnailsBox (which handles the creation of new workspaces) forces, for example, to double-check if the pointer is in the workspace "cut", which in turn causes some issues because the coordinates passed to handleDragOver are actor-relative (see https://bugzilla.gnome.org/show_bug.cgi?id=664625) In summary, is it a good idea to move all the "event" logic to ThumbnailsBox, and keep WorkspaceThumbnail only for displaying?
Created attachment 204026 [details] [review] workspaceThumbnail: extend active area to the edges of the selector We add an extra background to each workspace thumbnail, with the only responsability to catch clicks and drops outside the visible area
Created attachment 204900 [details] [review] workspaceThumbnail: extend thumbnail pick area to the edges of the selector
(In reply to comment #17) > (In reply to comment #13) > > Review of attachment 202768 [details] [review] [details]: > > > [...] > > > 2. Add an extra actor that is beneath the workspace actor whose only > > responsibility is to handle drops and clicks but isn't nested with the > > workspace actor. This would avoid most of the layout complexity of this patch > > but does mean that there will be some awkward code to forward events and dnd > > around.. > > As a variation of this approach, what about using ThumbnailsBox as the only > event handler? The current split between WorkspaceThumbnail and ThumbnailsBox > (which handles the creation of new workspaces) forces, for example, to > double-check if the pointer is in the workspace "cut", which in turn causes > some issues because the coordinates passed to handleDragOver are actor-relative > (see https://bugzilla.gnome.org/show_bug.cgi?id=664625) > > In summary, is it a good idea to move all the "event" logic to ThumbnailsBox, > and keep WorkspaceThumbnail only for displaying? When I first sw this comment I didn't much like the idea - it seemed like duplicating the clutter layout system a second time. But the workspace cut logic is pretty gross and if it makes things simpler to do a bit of manual layout calculation, I'd be in favor of it.
Review of attachment 204026 [details] [review]: doesn't seem this handles drops
Review of attachment 203115 [details] [review]: If the goal was to just get clickable invisible borders on an otherwise normal actor, I'd be fine with this, but considering the existing complexity where this is used, I think it likely just confuses things.
Review of attachment 204900 [details] [review]: Rejected my patch, so rejecting this as a consequence
(In reply to comment #21) > Review of attachment 204026 [details] [review]: > > doesn't seem this handles drops mmhh, it works for me. Setting the delegate of the back actor should be enough. Anyway, a patch follows implementing what is proposed in comment 17. It depends on patch at https://bugzilla.gnome.org/show_bug.cgi?id=669887 for correct xdnd. This also fix https://bugzilla.gnome.org/show_bug.cgi?id=664625
Created attachment 207342 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation.
Created attachment 207484 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation. minor corrections, mostly style
Created attachment 209480 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation. Rebased.
Review of attachment 209480 [details] [review]: ::: js/ui/workspaceThumbnail.js @@ +565,3 @@ + let thumbnail = this._thumbnails[i] + let [w, h] = thumbnail.actor.get_transformed_size(); + if (y >= thumbnail.actor.y && y <= thumbnail.actor.y + h) { You should just pick instead.
Review of attachment 209480 [details] [review]: Didn't really have time to do a full review, but this approach seems like a reasonable one to me, and we should try to get this landed before the code freeze on Monday (I'm off for the weekend, will be back Monday) Couple of random comments: ::: js/ui/workspaceThumbnail.js @@ +541,3 @@ + function(actor, event) { + return true; + })); this.actor.connect('button-press-event', function() { return true; } ) is sufficient @@ +565,3 @@ + let thumbnail = this._thumbnails[i] + let [w, h] = thumbnail.actor.get_transformed_size(); + if (y >= thumbnail.actor.y && y <= thumbnail.actor.y + h) { Responding to dragoo1 - don't think you can pick - the actors aren't reactive. @@ +682,3 @@ return false; + if (!source.realWindow && !source.shellWorkspaceLaunch && source != Main.xdndHandler && this._dropWorkspace != -1) the structure of this function is quite confusing. Suggest redoing it to if (this._dropWorkspace != 1) { } else if (this._dropPlaceholderPos != 1) { } else { return false; }
(In reply to comment #29) > Review of attachment 209480 [details] [review]: > > Didn't really have time to do a full review, but this approach seems like a > reasonable one to me, and we should try to get this landed before the code > freeze on Monday (I'm off for the weekend, will be back Monday) > > Couple of random comments: > > ::: js/ui/workspaceThumbnail.js > @@ +541,3 @@ > + function(actor, event) { > + return true; > + })); > > this.actor.connect('button-press-event', function() { return true; } ) > > is sufficient > > @@ +565,3 @@ > + let thumbnail = this._thumbnails[i] > + let [w, h] = thumbnail.actor.get_transformed_size(); > + if (y >= thumbnail.actor.y && y <= thumbnail.actor.y + h) { > > Responding to dragoo1 - don't think you can pick - the actors aren't reactive. Being reactive isn't prerequisite for picking; picks can just be optionally limited to reactive actors. i.e could just do: let foo = global.stage.get_actor_at_pos(Clutter.PickMode.ALL, x, y); while let foo = global.stage.get_actor_at_pos(Clutter.PickMode.REACTIVE, x, y); would limit to reactive actors.
Follow up from the IRC discussion: Picking doesn't solve the problem here as the x coordinate is supposed to be ignored. So this part should be fine as is.
Created attachment 209942 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation. -- rebased and amended
bump :) anyone willing to give a look to this thing? Tomorrow I'll be away and I'll be back only after code freeze, so if the maintainers find anything worth here, or anything in the need of being amended, please do what you think is just ;)
Review of attachment 209942 [details] [review]: Generally looking good, still just a few things ::: js/ui/workspaceThumbnail.js @@ +618,3 @@ // Draggable target interface handleDragOver : function(source, actor, x, y, time) { + if (!source.realWindow && !source.shellWorkspaceLaunch && source != Main.xdndHandler) Won't this addition by itself make the XDND drop do inter-workspace placesholders, which is wrong? @@ +668,3 @@ + return this._thumbnails[this._dropWorkspace].handleDragOverInternal(source, time); + + if (placeholderPos == -1) This needs restructuring the same way as the code in accept drop - if (this._dropSpace != -1) { } else if (this._dropPlaceholdePos != -1) { } else { } @@ +677,3 @@ + if (this._dropWorkspace != -1) { + if (!source.realWindow && !source.shellWorkspaceLaunch && source != Main.xdndHandler) + return false; I don't see the point of this if statement - acceptDropInternal should be handling itself what it want or doesn't work. @@ +721,3 @@ + return true; + } else + return false; This should have braces - we don't use braces on single line if statements, but if one branch of an if statement has braces all branches have braces
(In reply to comment #34) > Review of attachment 209942 [details] [review]: > > Generally looking good, still just a few things > > ::: js/ui/workspaceThumbnail.js > @@ +618,3 @@ > // Draggable target interface > handleDragOver : function(source, actor, x, y, time) { > + if (!source.realWindow && !source.shellWorkspaceLaunch && source != > Main.xdndHandler) > > Won't this addition by itself make the XDND drop do inter-workspace > placesholders, which is wrong? No it won't because before adding a placeholder I check again that source != Main.xdndHandler This line is here to return early it the source is uninteresting in all cases. > > @@ +668,3 @@ > + return > this._thumbnails[this._dropWorkspace].handleDragOverInternal(source, time); > + > + if (placeholderPos == -1) > > This needs restructuring the same way as the code in accept drop - > > if (this._dropSpace != -1) { > } else if (this._dropPlaceholdePos != -1) { > } else { > } > Ok. > @@ +677,3 @@ > + if (this._dropWorkspace != -1) { > + if (!source.realWindow && !source.shellWorkspaceLaunch && source > != Main.xdndHandler) > + return false; > > I don't see the point of this if statement - acceptDropInternal should be > handling itself what it want or doesn't work. Right, it is a leftover from previous iteration. > > @@ +721,3 @@ > + return true; > + } else > + return false; > > This should have braces - we don't use braces on single line if statements, but > if one branch of an if statement has braces all branches have braces Ok.
Created attachment 210116 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation.
Created attachment 210117 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Currently, click and drop events are handled by each WorkspaceThumbnail instance. With the introduction of the workspace cut and the request to extend the reactive area of the workspace selector to the edge of the monitor, it becomes more convenient to do all the event handling inside ThumbnailsBox, even if this requires some manual layout computation. -- ops, I put braces in a place they shouldn't be
Review of attachment 210117 [details] [review]: Looks good! Thanks for working on this long-standing issue!
Excellent work, thanks Stefano!
Comment on attachment 210117 [details] [review] workspaceThumbnail: move event handling to ThumbnailsBox Attachment 210117 [details] pushed as 369c1b0 - workspaceThumbnail: move event handling to ThumbnailsBox
btw, can someone change the status to RESOLVED/FIXED? ;)