GNOME Bugzilla – Bug 606258
remove user_data arguments
Last modified: 2010-03-18 17:33:35 UTC
We should delete the user_data arguments for callbacks from the GScript API, because it's trivial to create closures in GScript, and passing "null" is annoying. This change needs to be made sooner rather than later, otherwise it will never happen; it is an API breaking change. Agreement? Would a patch for this be accepted?
Yes, user_data arguments should be removed. There are many ways to pass in user arguments, Lang.bind() is one of them.
Created attachment 154286 [details] [review] Remove user_data arguments for callbacks We already remove user_data arguments to the timeout/idle and signal functions - this change does the same for generic callbacks (assuming correct gi-annotations of course). Note that this change constitutes an API break - although I'd expect the impact on existing code rather low, because of one notable exception where code will continue to work: 1) the user_data argument is unused (read: null) 2) user_data is the last argument If both conditions are met, code will continue to work as excess arguments on the JS side are simply ignored. I'd expect 1) to be usually the case (I've never stumbled upon code like the callback unit tests), and 2) not being met is something I'll yet have to encounter. With that said, additional arguments after the user_data parameter might still be a corner case worth adding to Everything-1.0.
Gjs maintainers, any opinions on when/how to land API-breaking changes like this?
I would expect that while this is technically an API break, in practice it barely breaks anything - unless we do throw an error if someone's provided an extra "null" arg? (Don't know if our g-i invocation stuff ignores extra args in the way JS normally does. if not it probably should.) If nobody is really going to break I think we can just put this in. It's not like gjs is in stable locked-down release mode.
(In reply to comment #4) > I would expect that while this is technically an API break, in practice it > barely breaks anything - unless we do throw an error if someone's provided an > extra "null" arg? (Don't know if our g-i invocation stuff ignores extra args in > the way JS normally does. if not it probably should.) Extra arguments are ignored, so providing an extra "null" is fine as long as the user_data argument is the last one - as already pointed out above, I think it's more realistic to expect code breakage because of the user_data argument being actually used...
Created attachment 155556 [details] [review] Remove user_data arguments for callbacks Rebased to master
Review of attachment 155556 [details] [review]: Looks like you didn't modify gi/function.c:gjs_callback_closure ? That means user_data will still come through in the callback, but not really a big deal. I'm actually not sure offhand if introspection even marks that. Don't worry about this. The changes to function.c look reasonable to me. ::: test/js/testEverythingBasic.js @@ +218,3 @@ + testObj.userData.foo.length); + testObj.callbackUserData)), + Lang.bind(testObj, I don't understand what's going on this test. Is this just trying to test the JavaScript GC? There's no way that Everything.test_callback_user_data can change testObj.userData now. I think this test should just be deleted, basically.
Created attachment 155911 [details] [review] Remove user_data arguments for callbacks Remove test_callback_user_data()
Review of attachment 155911 [details] [review]: This looks right to me. Note I'd like a bit more test coverage here, so review of https://bugzilla.gnome.org/show_bug.cgi?id=611811 would be appreciated.
Attachment 155911 [details] pushed as 906ad5a - Remove user_data arguments for callbacks