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 621197 - De-duplicate "actor contains actor" code
De-duplicate "actor contains actor" code
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2010-06-10 12:33 UTC by Dan Winship
Modified: 2010-06-11 14:07 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
De-duplicate "actor contains actor" code (10.69 KB, patch)
2010-06-10 12:33 UTC, Dan Winship 		committed Details | Review

Description Dan Winship 2010-06-10 12:33:16 UTC 
(see also http://bugzilla.openedhand.com/show_bug.cgi?id=2162)
Comment 1 Dan Winship 2010-06-10 12:33:18 UTC 
Created attachment 163285 [details] [review]
De-duplicate "actor contains actor" code

Add _st_actor_contains() in st-private for use within St, and
monkey-patch in a Clutter.Actor.contains() for use by javascript, and
then replace all the duplicate implementations with one or the other
of those.
Comment 2 Florian Müllner 2010-06-10 13:20:47 UTC 
Review of attachment 163285 [details] [review]:

Oh yes! I'm sure you considered adding the method to StWidget instead and decided against it (chrome.js?) ...
Comment 3 Dan Winship 2010-06-10 13:55:18 UTC 
(In reply to comment #2)
> Oh yes! I'm sure you considered adding the method to StWidget instead and
> decided against it (chrome.js?) ...

yeah, there are definitely places where we want to be able to test against arbitrary actors, not just StWidgets
Comment 4 drago01 2010-06-10 20:50:28 UTC 
(In reply to comment #1)
> Created an attachment (id=163285) [details] [review]
> De-duplicate "actor contains actor" code
> 
> Add _st_actor_contains() in st-private for use within St, and
> monkey-patch in a Clutter.Actor.contains() for use by javascript, and
> then replace all the duplicate implementations with one or the other
> of those.

Is there any reason for the "rush" ... i.e can't we wait for the clutter patch to be reviewed instead of having our own implementation and remove it shortly afterwards?

OTOH the code is already written ...
Comment 5 Dan Winship 2010-06-10 21:02:22 UTC 
it's not clear that they'd add it to the stable branch anyway
Comment 6 Florian Müllner 2010-06-10 21:07:09 UTC 
(In reply to comment #5)
> it's not clear that they'd add it to the stable branch anyway

Also: the code is already in gnome-shell - and removing the duplicates now will make it a tiny bit easier to replace it with the clutter version (should it land).
Comment 7 drago01 2010-06-10 21:15:55 UTC 
(In reply to comment #6)
> (In reply to comment #5)
> > it's not clear that they'd add it to the stable branch anyway
> 
> Also: the code is already in gnome-shell - and removing the duplicates now will
> make it a tiny bit easier to replace it with the clutter version (should it
> land).

Yeah that makes sense.

(In reply to comment #5)
> it's not clear that they'd add it to the stable branch anyway

We should move to 1.3/1.4 soon anyway.


But anyway was just curious nothing is actually wrong with this patch.
Comment 8 Dan Winship 2010-06-11 14:07:38 UTC 
Attachment 163285 [details] pushed as a4befeb - De-duplicate "actor contains actor" code