After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 645990 - adding anything in "places" to favorites results in bad behavior
adding anything in "places" to favorites results in bad behavior
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Dan Winship
gnome-shell-maint
gnome3-important
Depends on:
Blocks:
 
 
Reported: 2011-03-28 18:16 UTC by Sri Ramkrishna
Modified: 2011-06-21 20:24 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Console output with stack trace (1.98 KB, text/plain)
2011-03-28 22:15 UTC, Ron
  Details
searchDisplay: don't make all SearchResult draggable (2.34 KB, patch)
2011-03-28 23:44 UTC, Maxim Ermilov
rejected Details | Review
search-display: Fix getDragActorSource()/getDragActor() (1.06 KB, patch)
2011-03-29 11:38 UTC, Florian Müllner
reviewed Details | Review
search-display: Fix getDragActorSource()/getDragActor() (1.13 KB, patch)
2011-03-31 14:07 UTC, Florian Müllner
accepted-commit_after_freeze Details | Review
search-display: Fix getDragActorSource()/getDragActor() (1.13 KB, patch)
2011-04-01 16:12 UTC, Florian Müllner
committed Details | Review
app-search-result: Fix launching on another workspace (930 bytes, patch)
2011-04-06 06:27 UTC, Florian Müllner
committed Details | Review
search-display: Try harder to use a correct drag actor source (2.02 KB, patch)
2011-05-20 12:17 UTC, Florian Müllner
committed Details | Review

Description Sri Ramkrishna 2011-03-28 18:16:37 UTC
I have tried this on ubuntu, jhbuild, and fedora 15 and I get the same problem.  If I try to add "home" to the favorites bar, I get some strange behavior.  I have a hard time getting out of overview and things are just not working correctly.

Finally, if you decide to do alt-f2 r, then the whole thing hangs and you have to restart gdm.
Comment 1 Elad Alfassa 2011-03-28 18:19:17 UTC
When I drag a place (eg. home) to the dash, the whole shell just freeze and I can't do anything until I kill my session and log-in again.
Fedora 15 x86_64 gnome 2.91.93
Comment 2 Ron 2011-03-28 22:15:51 UTC
Created attachment 184517 [details]
Console output with stack trace
Comment 3 Maxim Ermilov 2011-03-28 23:44:18 UTC
Created attachment 184520 [details] [review]
searchDisplay: don't make all SearchResult draggable

PlaceSearchProvider/DocSearchProvider should not be draggable
Comment 4 Florian Müllner 2011-03-29 11:37:07 UTC
Review of attachment 184520 [details] [review]:

This is only a workaround for a bug in searchDisplay - while the dash only accepts dropped application results, dropping docs/places on a particular workspace used to work fine; so we should fix the real bug to make it work again.
Comment 5 Florian Müllner 2011-03-29 11:38:06 UTC
Created attachment 184569 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

The 'icon' property in search results' meta info has been replaced
by a 'createIcon' property, adjust to this change.
Comment 6 Florian Müllner 2011-03-29 15:56:57 UTC
I don't think dnd of those items is essential, but the resulting behavior when trying is pretty horrible, so would be nice to see this fixed.
Comment 7 Owen Taylor 2011-03-30 20:38:23 UTC
Review of attachment 184569 [details] [review]:

Looks safe to me and certainly fixes a serious bug.

::: js/ui/searchDisplay.js
@@ +79,2 @@
     getDragActorSource: function() {
+        return this._content;

This might not be strictly right. Shouldn't it be content.child not content? It doesn't really matter much because we rescale and we switch pages immediately so it's basically impossible to tell if the alignment was exactly right or not.
Comment 8 Owen Taylor 2011-03-30 20:48:19 UTC
Review of attachment 184569 [details] [review]:

::: js/ui/searchDisplay.js
@@ +79,2 @@
     getDragActorSource: function() {
+        return this._content;

Actually, this isn't right either - it needs to be 

 this._icon.icon

noting that this._icon doesn't currently exist, and the code that creates the icon doesn't always run, if provider.createResultActor exists and is used instead. So, maybe this is the best approach with a:

 // not exactly right, but aligment problems are hard to notice

comment.
Comment 9 Owen Taylor 2011-03-30 20:49:10 UTC
Assigning to danw for a second review.
Comment 10 Dan Winship 2011-03-31 12:40:07 UTC
Comment on attachment 184569 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

yeah, I agree with Owen's review. To fix it completely we'd need the metaInfo to have a getDragActorSource method.
Comment 11 Florian Müllner 2011-03-31 14:07:11 UTC
Created attachment 184782 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

(In reply to comment #10)
> (From update of attachment 184569 [details] [review])
> yeah, I agree with Owen's review. To fix it completely we'd need the metaInfo
> to have a getDragActorSource method.

Right. Alternatively we could remove getDragActorSource altogether and not get the snapback animation ...
Comment 12 Florian Müllner 2011-04-01 15:40:16 UTC
(In reply to comment #10)
> (From update of attachment 184569 [details] [review])
> yeah, I agree with Owen's review. To fix it completely we'd need the metaInfo
> to have a getDragActorSource method.

Can I get another review on this, or should I update the patch with a proper fix now? I'd rather wait with that until freeze break ends ...
Comment 13 Dan Winship 2011-04-01 16:09:15 UTC
Comment on attachment 184782 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

>+        // not exactly right, but aligment problems are hard to notice

so are typos

(marking commit-after-freeze for now since it doesn't yet have release-team approval).
Comment 14 Florian Müllner 2011-04-01 16:12:39 UTC
Created attachment 184878 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

(In reply to comment #13)
> (From update of attachment 184782 [details] [review])
> >+        // not exactly right, but aligment problems are hard to notice
> 
> so are typos

Gah - shouldn't rely on copy+paste :(
Comment 15 Owen Taylor 2011-04-01 16:13:31 UTC
(In reply to comment #12)
> (In reply to comment #10)
> > (From update of attachment 184569 [details] [review] [details])
> > yeah, I agree with Owen's review. To fix it completely we'd need the metaInfo
> > to have a getDragActorSource method.
> 
> Can I get another review on this, or should I update the patch with a proper
> fix now? I'd rather wait with that until freeze break ends ...

I think that danw and I are in agreement that your current patch makes the most sense in the short term. I'll send this out in my batch of freeze break emails today.
Comment 16 Florian Müllner 2011-04-01 16:15:07 UTC
(In reply to comment #15)
> I'll send this out in my batch of freeze break emails
> today.

Ah, OK - I was about to do it myself, but I guess batching is more r-t friendly ...
Comment 17 Florian Müllner 2011-04-02 09:37:23 UTC
Comment on attachment 184878 [details] [review]
search-display: Fix getDragActorSource()/getDragActor()

Attachment 184878 [details] pushed as 429f809 - search-display: Fix getDragActorSource()/getDragActor()

Pushed after r-t approval; leaving bug open for a proper fix after freeze end.
Comment 18 MT 2011-04-06 03:14:30 UTC
Does this also fix dragging items from the Settings category? I tried that now, and I am unable to drop a search result within Settings onto a workspace and launch it (the drag event is not finished after releasing the mouse button, I have to press Esc to cancel it).
Comment 19 Florian Müllner 2011-04-06 06:27:44 UTC
Created attachment 185261 [details] [review]
app-search-result: Fix launching on another workspace

(In reply to comment #18)
> Does this also fix dragging items from the Settings category?

No. I'm attaching a fix.
Comment 20 Dan Winship 2011-04-06 12:03:51 UTC
Comment on attachment 185261 [details] [review]
app-search-result: Fix launching on another workspace

indeed
Comment 21 Florian Müllner 2011-04-07 11:52:11 UTC
Comment on attachment 185261 [details] [review]
app-search-result: Fix launching on another workspace

Attachment 185261 [details] pushed as 09607f6 - app-search-result: Fix launching on another workspace

Leaving bug open as of comment 17.
Comment 22 Florian Müllner 2011-05-20 12:17:01 UTC
Created attachment 188219 [details] [review]
search-display: Try harder to use a correct drag actor source

Commit 429f809b7 fixed an exception in getDragActorSource(), but
the returned actor is only an approximation (e.g. in contrast to
the actual drag actor, it includes the label).
Try a bit harder to return the correct actor and only fall back to
the approximation when necessary.

OK, I admit that I had pretty much forgotten about this bug and the promise to write a proper fix - sorry about that.
Comment 23 Dan Winship 2011-06-21 20:20:21 UTC
Comment on attachment 188219 [details] [review]
search-display: Try harder to use a correct drag actor source

oops, just noticed this was assigned to me
Comment 24 Florian Müllner 2011-06-21 20:24:38 UTC
Attachment 188219 [details] pushed as b9456ca - search-display: Try harder to use a correct drag actor source