GNOME Bugzilla – Bug 656333
Dragging a non-favorited dash item back to its original position should not favorite the app
Last modified: 2012-03-03 16:45:40 UTC
Steps: 1. Grab a non-favorited app in the dash 2. Pull it out to the right. 3. Drop it over its original target. Expected: Nothing significant happens. Actual: App becomes favorited and moves inline with the other favorites. We should probably just have the placeholder stop after non-favorites, and put a separator between the two -- you have to drag into the favorites section to do anything.
Created attachment 206158 [details] [review] Don't favorite dash item when the item isn't draged in the favorites With this patch the drag place holder is removed when the draged item is outside the favorites. On release, the item is added to the favorites only if the place holder exists
Thanks for the patch, Jean-Phillippe! A few comments: * I don't think the app should get favourited if it is moved from its original position - it shouldn't matter where in the dash it gets dragged to. * Since it is not being added to the dash, the pointer cursor should not have a + on it when the launcher is being held over the dash. * You have some trailing whitespace on line 20. :)
Created attachment 206612 [details] [review] Fix cursor appearance & trailing space Not sure why you say that the item gets favorited when it is moved to its original position. In my case it doesn't. Can you explain a little more ? I fixed the two other points Thank you
(In reply to comment #3) > Created an attachment (id=206612) [details] [review] > Fix cursor appearance & trailing space Thanks for the updated patch! It's definitely a step in the right direction. The cursor is now a simple grab hand (dnd-none.png in the theme) when dragging the launcher over the dash. It needs to be a hand with an arrow when hovered over the dash to indicate that the launcher will be dropped (that's dnd-move.png). > Not sure why you say that the item gets favorited when it is moved to its > original position. In my case it doesn't. Can you explain a little more ? Steps to reproduce: 1. Click and hold a non-favourited launcher in the dash 2. Drag it to a different location in the dash Expected behaviour - the app gets moved and nothing else. Result - the app is moved and is added to the favourites.
(In reply to comment #4) > 1. Click and hold a non-favourited launcher in the dash > > 2. Drag it to a different location in the dash > > Expected behaviour - the app gets moved and nothing else. That is a completely new requirement (which means it'll require quite intrusive code changes) - limiting reordering to favorites was explicitly okayed by Jon. If we really want to change the design here, we'll have to (re)answer some questions first: - how persistent should the position of non-favorites be? e.g. if I move "app" around and close it, should its icon appear at the old position the next time it's opened? - if not, why do you want to change the position of a transient item? And doesn't the same apply to summary items in the message tray? - either way, what are the consequences on the position of favorites? right now, the position is fixed, e.g. does not depend on the set of running apps (we used to have "most used apps" in the dash, but the approach was abandoned, not least because the frequently changing positions were problematic) - what is the preferred way of adding items to the dash then? right click? application view? (yeah, I'm bikeshedding here - moving an icon up into the "favorites area" is my option of choice to add favorites ;-)
(In reply to comment #5) > (In reply to comment #4) > > 1. Click and hold a non-favourited launcher in the dash > > > > 2. Drag it to a different location in the dash > > > > Expected behaviour - the app gets moved and nothing else. > > That is a completely new requirement (which means it'll require quite intrusive > code changes) - limiting reordering to favorites was explicitly okayed by Jon. If that's the case let's just try and land the existing patch for now.
Review of attachment 206612 [details] [review]: Looks mostly reasonable. The commit message is missing a body, and the subject needs some serious trimming (I'd suggest "dash: Don't favorite items dropped at their original position", which is safely within the 70-75 limit). ::: js/ui/dash.js @@ +846,3 @@ + // No drag placeholder means we don't wan't to favorite the app + // and we are dragging it to its original position + let addFavorite = (this._dragPlaceholder); I don't understand why you set up a MetaLater function to do nothing - why not just if (!this._dragPlaceholder) return true;
Created attachment 208901 [details] [review] Better commit message & code improvement
Review of attachment 208901 [details] [review]: Looks good except for a small typo in the commit message (draged -> dragged) - I think a more active form would sound better ("The drag placeholder is removed ..." => "Remove the drag placeholder ..."), but I guess that's just me :-) In any case, the commit message can be adjusted when pushing, no need to attach another iteration here. We are in UI freeze now, so you (we) might need to request a freeze break (technically this is definitively a UI change, but it's so minor that I suspect we can get away with just pushing it).
Would be great, thanks !
Given that the patch does not really affect documentation in any way (e.g. the behavior changed is not documented in that much detail, screenshots are not affected), I've pushed the patch without freeze break request after a quick IRC discussion. Thanks for your contribution!