GNOME Bugzilla – Bug 609218
drag and drop from places in the activities overview
Last modified: 2010-03-18 17:05:26 UTC
Hi all... I don't know if this feature is in designers/developers' mind or it should be working by now and I am experiencing a bug or not even planned. I think it would be great if it were possible to drag and drop from places/devices section, to have opened in a specific workspace the directory you want. It would also be great being able to do the same from the search result of applications or documents. When using the "find box" in overlay mode, you cannot drag and drop the results to any workspace. Is it working as expected? This enhancement would unify the behaviour in left panel in overlay mode, since it is possible to drag and drop from applications and recent items... but not from places/devices or searching results... Thank you and great job!! Ramon.
(In reply to comment #0) > I think it would be great if it were possible to drag and drop from > places/devices section, to have opened in a specific workspace the directory > you want. That's actually a regression introduced by some random idiot (read: me) a while ago, so yeah, it's definitively a bug. > It would also be great being able to do the same from the search result of > applications or documents. When using the "find box" in overlay mode, you > cannot drag and drop the results to any workspace. I am not sure whether that's desired behavior or just nobody bothered implementing it. We'll have to check that, but IMHO it would make sense to allow DND for consistency.
Created attachment 153219 [details] [review] [placeDisplay] Fix dnd regression DND from the places section broke with 1c4c3afb when St.Label was replaced with St.Button. To fix, make the pointer grab in St.Button optional, to allow dnd of buttons.
Created attachment 153220 [details] [review] [dash] Enable DND of search results
Yep, consistency between the search results and the presentation and behavior of the rest of the dash sounds like a good thing.
I wish there would be more "random idiots" working as you do and letting users access to great and free enviroments like this one. Thank you for your time!! Great job, I mean it! Ramon.
Review of attachment 153219 [details] [review]: I think the StButton patch should be separate; I'd like to keep functional changes to ST as independent changes and not have them mixed together with shell patches that use them. I think there is a problem here - with this patch, if a drag is started the button won't get the release event so is_pressed will get stuck as TRUE, and the button will misbehave next time the pointer enters the button. (It may not be obvious if the button isn't styled to look different when pressed, but it's definitely a bug.) There will also be problems with the the pointer moving directly from a child of the StButton to a parent of the StButton while the button is pressed - without a grab, the StButton won't get a leave event in that case. (One thing I'm confused about is I don't quite offhand see how your patch works - how does the box actor get press events when the user clicks on the StButton? So maybe I'm missing something about this.) This is definitely messy. Two approaches occur to me: - Move the iconBox into the St.Button - have the St.Button be the entire icon and text combo. Then add support in St.Button for triggering a drag. Something like st_button_set_draggable() and a ::start-drag signal. You'd use the C equivalent of Gtk.Settings.get_default().gtk_dnd_drag_threshold() right from the St.Button code. (The fancier version of this is to move dnd.js into the ST layer. But that would require a quite a bit of API design and work.) - Don't use St.Button at all, and handle everything from JS code. Maybe someone else has a simpler solution. I've fooled around a bit in my head with approaches involving synthesizing faked events, but they don't seem satisfactory.
(In reply to comment #6) > (The fancier version of this is to move dnd.js into the ST layer. But that > would require a quite a bit of API design and work.) > > - Don't use St.Button at all, and handle everything from JS code. not especially related, but bug 606979 suggests having an St.Button where the clickable area extends outside the decorated area. But if we're going to be redesigning StButton...
(In reply to comment #7) > (In reply to comment #6) > > (The fancier version of this is to move dnd.js into the ST layer. But that > > would require a quite a bit of API design and work.) > > > > - Don't use St.Button at all, and handle everything from JS code. > > not especially related, but bug 606979 suggests having an St.Button where the > clickable area extends outside the decorated area. But if we're going to be > redesigning StButton... Couldn't you do this by styling .my-button .my-visible-button { } .my-button:hover .my-visible-button { } ?
would that get all the hover/pressed/etc interaction correctly between the two different buttons? ah, i guess if the inner button was non-reactive and i just set its pseudo-class by hand to match the outer button...
(In reply to comment #9) > would that get all the hover/pressed/etc interaction correctly between the two > different buttons? ah, i guess if the inner button was non-reactive and i just > set its pseudo-class by hand to match the outer button... The inner button wouldn't be a button at all - my class naming was confusing - you just drive the styling off of the pseudo-class on the outer element.
*** Bug 609494 has been marked as a duplicate of this bug. ***
Review of attachment 153220 [details] [review]: I'd like to see a body to the commit message - the subject is pretty self explanatory, but something like: Allow dragging search results to a specific workspace to launch them on that workspace; the drag icon is the icon from the search result. Does add information to someone reading the log. There's virtual always always a sentence or two that can be said usefully :-) Otherwise, looks good. ::: js/ui/dash.js @@ +366,3 @@ this.actor.connect('clicked', Lang.bind(this, this._onResultClicked)); + + let draggable = DND.makeDraggable(this.actor); Hmm, assigning it without using the variable is a bit weird, but it's what we do elsewhere; I guess it makes it clear that it returns an object that could be connected to, etc, if needed. @@ +387,3 @@ + + getDragActor: function(stageX, stageY) { + return new Clutter.Clone({ source: this.metaInfo['icon'] }); There's a corner case with using a Clone - if the search results change during the drag and the source actor gets destroyed things will go haywire. But this is OK for now.... not worth a lot of complexity to handle that corner case.
Comment on attachment 153220 [details] [review] [dash] Enable DND of search results Attachment 153220 [details] pushed as 67e70df - [dash] Enable DND of search results
*** Bug 606159 has been marked as a duplicate of this bug. ***
Created attachment 155593 [details] [review] [placeDisplay] Fix dnd regression It would probably be nicer to move dnd.js into StWidget, but a much less intrusive fix is to replace StButton with StClickable and handle dnd just like AppWellIcon.
Review of attachment 155593 [details] [review]: Basically looks right to me, few minor comments. ::: data/theme/gnome-shell.css @@ +504,3 @@ + +.places-actions { + spacing: 2px; I don't really follow what the changes are here - why was the spacing changed for .places-actions - this is a vertical box, right? May just need some explanation in the commit message. ::: js/ui/placeDisplay.js @@ +447,3 @@ + let [stageX, stageY] = event.get_coords(); + this._dragStartX = stageX; + this._dragStartY = stageY; You should check the event button and only trigger drags on button 1, otherwise a multiple-button button press can get things confused. I think you should unset dragStartX/Y on button release. It looks like currently: - the release will trigger a launch - the launch will trigger leaving the overview - that will cause a hover change - which will unset dragStartX/Y But that's not very clean.
Created attachment 155687 [details] [review] [placeDisplay] Fix dnd regression (In reply to comment #16) > +.places-actions { > + spacing: 2px; > > I don't really follow what the changes are here - why was the spacing changed > for .places-actions - this is a vertical box, right? May just need some > explanation in the commit message. This is actually a left-over from a test - to verify that the hover state is updated correctly, I added a border to each item, which was highlighted on hover. With the original spacing the borders touched the content which looked so crappy that I quickly split it into spacing and padding ... the net result is the same, so I didn't change it back when doing the patch. Should I do that?
Hmm, what's the actual regression with this patch? DnD from places seems to work OK now.
(In reply to comment #18) > Hmm, what's the actual regression with this patch? DnD from places seems to > work OK now. Are you sure? It still does not work at all here (pointer grab in St.Button) ...
Review of attachment 155687 [details] [review]: Overall we clearly need to clean up the DnD bits; I think pushing DnD down into St (like mx-{draggable,droppable}.[ch]), and having StDraggableButton might be a big help. But I don't want to block this patch on that. One minor comment. ::: js/ui/placeDisplay.js @@ +449,3 @@ + return; + if (event.get_button() != 1) + _onButtonPress: function(actor, event) { Would prefer if this was an explicit "return false;" we don't do this at all consistently in the rest of the code, but it makes things a lot more clear that the return value IS being used. @@ +453,3 @@ + return; + if (event.get_button() != 1) + _onButtonPress: function(actor, event) { And here too.
Created attachment 156455 [details] [review] [placeDisplay] Fix dnd regression Fixed return values.
Review of attachment 156455 [details] [review]: LGTM
Attachment 156455 [details] pushed as ddfe944 - [placeDisplay] Fix dnd regression