GNOME Bugzilla – Bug 733414
Don't use addSignalMethods when not needed
Last modified: 2014-10-10 18:13:58 UTC
When you call addSignalMethods to a GObject inherited class, it overrides the native connect, emit and disconnect functions coming from GObject. And this impede us to connect or emit native signals (those signals inherited from the native type) from JavaScript, because we are using the built-in signal management in Gjs instead of the GObject one. For example: --- const Foo = new Lang.Class({ Extends: Gtk.Button ... }); Signals.addSignalMethods(Foo.prototype); let a = new Foo(); a.connect('clicked', function() { //never emited (emitted by GObject system, connected using //Gjs signal management) }); --- We need to avoid addSignalMethods if we are inheriting from GObject.Object and use the Signals property of the Lang.Class constructor to add the custom signals. What do you think?
Sounds sane. Do we have many such cases atm? And are they trivial to convert? Do we have any bugs or buglets stemming from this atm? Is it urgent or could it be a gnome-love thing?
It seems that it didn't cause much problem (I didn't search about related bugs, just deducting from regular usage), it should not be urgent if we are careful or if we have luck (as is happening now!), but we don't want to play with luck :P They are trivial to convert, just rip off the addSignalMethods call to classes which have GObject in the inheritance tree and look for custom signal connection/emission to add it to the Lang.Class Signals property.
I am interested in solving this bug and am going to be an OPW applicant this year. I read through this page and understood this much ! Basically wherever its a call to an inherited Gobject we have to avoid use addSignalMethods and instead use Lang.Class Signals property ? How would that go?
(In reply to comment #3) > I am interested in solving this bug and am going to be an OPW applicant this > year. I read through this page and understood this much ! Basically wherever > its a call to an inherited Gobject we have to avoid use addSignalMethods and > instead use Lang.Class Signals property ? How would that go? Yes, you got it right, summarizing: 1) Remove Signals.addSignalMethods or Utils.addSignalMethods for classes that inherits from GObject (directly or indirectly) 2) If a modified class adds a custom signal, add it by using the Signals property. 3) We usually use our Utils.addSignalMethods instead of gjs's Signals.addSignalMethods. Our function adds the "once" method to the classes besides connect, disconnect, etc; make sure that you don't use the "once" method for instances of modified classes and use Utils.once instead.
Created attachment 287214 [details] [review] Geoclue inherits from Gobject hence added Signals property instead of using Utils.addSignalMethod
Ummm sorry I did not do a reset after my first commit hence it gives a diff with my prev wrong commit! It isnt giving a diff with the original file in use which had Utils.addSignalMethods() for a class inheriting from a Gobject!
Created attachment 287215 [details] [review] Geoclue inherits from Gobject hence added Signals property instead of using Utils.addSignalMethod
Created attachment 287216 [details] [review] Geoclue inherits from Gobject hence added Signals property instead of using Utils.addSignalMethod
Okay I updated the patch. This is fine. Please review it ! :)
Review of attachment 287216 [details] [review]: Hi, thanks for the patch! It looks nice. For the commit message I would prefer something like: "geoclue: Use GObject Signals property" Was this the only module were this change was needed? Thx!
Review of attachment 287216 [details] [review]: Add maybe use your patch description in this bug for commit message body. Also the body should include a link to this bug. See other commits, using git log, to get the idea.
Ok cool (y) ! Should I commit this patch again? And yes this is the only module where this was required. I grepped addSignals in the module and a few files came up. I all of them this was the only class I found inheriting from Gobject.
And nothing inheriting a subclass of GObject? Like a Gtk widget?
I think there were a few classes inheriting from Gtk Popbox . I will check... Do we have to fix that too?
They have the same issue, yes.
I was trying to fix the class searchPopup which inherits from Gtk widget PopOver . I removed the addSignalsMethod() and instead added a Signals for 'selected' which is getting emitted with 1 argument. So when I write Signals: 'selected': { } without any argument it gives me an error when I try to run search. How do I fix this? Basically how do I add an argument here => Signals: { 'selected': { } } Also when no signal is getting emitted, do I need to include a Signals property.
Hi, you can look here for syntax: https://git.gnome.org/browse/gjs/tree/installed-tests/js/testGObjectClass.js#n115 When we do not emit a signal, we do not need to define one.
And the parameter that geys emitted is a GeocodePlace which inherits from GObject, so type should beGObject.TYPE_OBJECT
Created attachment 287314 [details] [review] https://bugzilla.gnome.org/show_bug.cgi?id=733414
Created attachment 287315 [details] [review] Used Signals Property instead of addSignalsMethods
Review of attachment 287315 [details] [review]: Thanks! Did something happen while generating this patch? It does not look very complete. Have you looked into using git bz for attaching patches to bugzilla? https://wiki.gnome.org/Projects/GnomeShell/Development/WorkingWithPatches
I attached one patch and then realized that another line had to be removed so attached another patch. Hence, it is showing the most recent changes. If you go see the obsolete patches , https://bugzilla.gnome.org/attachment.cgi?id=287314&action=diff , this shows the changes I made in the first go.
Ok. :) Well don't do that :) The patch attached represents the commit that I will push/merge. It should follow all the guidelines that are in the links I sent you with regards to commit message and scope. So please re-attach a patch that has all the changes that you want to be commited and with a commit message and shortlog that follow the guidelines. Thanks!
If you have questions about using git to split a commit into multiple commit or to squash together multiple commits to one commit, feel free to ask on irc or another media.
Created attachment 287320 [details] [review] Added Signals property instead of using Utils.addSignalMethod
This final patch fixes all the files where class was inheriting from Gtk.object. Please review this. This is working :)
Ohh okay I will resubmit a patch which has all the commits together merged.
Review of attachment 287320 [details] [review]: Thanks! What about the geoclue.js? It is a GObject as well? And emits a signal. And what about contextMenu.js? Also, I am wondering, are you really basing this of the master branch? I do not see a GeocodeGLib imports in searcPopup.js in my repo. I think I want you to re-check to make sure you catch all, at least geoclue.js is missing in this patch. Also the commit message is not formatted like we usually do them in Maps, check with git log to see. The link to the bug should stand on its own, on the last line of the commit message. Also, change 'Gtk inherited classes' 'GObject inherited classes'. Try to keep the shortlog message (the header) to be less than or equal to 50 chars. So maybe: 'Remove addSignalsMethods for GObject classes' And, "Used" => "Use" Otherwise looks fine. ::: src/searchPopup.js @@ +23,3 @@ const GdkPixbuf = imports.gi.GdkPixbuf; const GObject = imports.gi.GObject; +const GeocodeGlib = imports.gi.GeocodeGlib; I do not have this line in my repo.
Created attachment 287322 [details] [review] Added Signals property instead of using Utils.addSignalMethod
Review of attachment 287322 [details] [review]: Thanks! Almost there! Please use the imperative form in the commit log: "Removed" => "Remove" For the log, maybe you could add a bit of Damiáns explanation of why this is needed? ::: src/searchPopup.js @@ +23,3 @@ const GdkPixbuf = imports.gi.GdkPixbuf; const GObject = imports.gi.GObject; +const GeocodeGlib = imports.gi.GeocodeGlib; Do not add this line. It should not be there and has no relevance to this change.
Created attachment 287324 [details] [review] Use Signals Property instead of addSignalsMethods for Gobject classes
Review of attachment 287324 [details] [review]: Thanks this looks great! There are some nit comments on the formatting and spelling in the commit message. And also you removed an non-relevant empty line in searchPopup.js. Please consider these things in the future! As for now I can fix up the last nits when I merge this. However it will not be merged right now. We are still in a period where we collect bugs to be fixed in the stable releases. So this will get commited after we branch of the stable branch gnome-3-14. Thanks! ::: src/searchPopup.js @@ +23,2 @@ const GdkPixbuf = imports.gi.GdkPixbuf; const GObject = imports.gi.GObject; You removed an empty line here. Please do not do that.
Ok will make sure I don't do that in my further work.