GNOME Bugzilla – Bug 654406
Can't use tp_connection_get_contacts_by_handle()
Last modified: 2012-02-06 20:33:11 UTC
I tried to get rid of the shell_get_tp_contacts() C wrapper: conn.get_contacts_by_handle([targetHandle], contactFeatures, Lang.bind(...) but I'm hitting this exception: aaaaaaaaaaa JS ERROR: !!! Exception was: Error: FIXME: Only supporting fixed size ARRAYs JS ERROR: !!! lineNumber = '0' JS ERROR: !!! fileName = '"gjs_throw"' JS ERROR: !!! stack = '"Error("FIXME: Only supporting fixed size ARRAYs")@:0 ("FIXME: Only supporting fixed size ARRAYs")@gjs_throw:0
http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-contact.html#tp-connection-get-contacts-by-handle the annotations appear to be correct; this should work... did we not actually test the multiple array case?
(In reply to comment #1) > http://telepathy.freedesktop.org/doc/telepathy-glib/telepathy-glib-contact.html#tp-connection-get-contacts-by-handle > > the annotations appear to be correct; this should work... did we not actually > test the multiple array case? I guess we forgot conversion when invoking callbacks (in fact, that error is for converting C arrays to JS, not the other way round).
Created attachment 191749 [details] [review] Add tests for array arguments in callbacks Add a test that accepts a callback receiving array arguments
Created attachment 191751 [details] [review] Support callbacks that accept array arguments Introduce a caching layer for callback parameters, similar to that used for functions, and use it to support arrays with explicit length, as well as ruling out (out) and (inout) arguments, that were never really supported.
I tried your patch and now I'm hitting this issue: JS ERROR: !!! Unhandled type error converting GArgument to JavaScript
(In reply to comment #5) > I tried your patch and now I'm hitting this issue: > > JS ERROR: !!! Unhandled type error converting GArgument to JavaScript Which is caused by marshalling a GError in the callback (which is GI_TYPE_TAG_ERROR in introspection). Yay for another bug!
Comment on attachment 191751 [details] [review] Support callbacks that accept array arguments looks right... maybe wait until fixing the GError issue as well before committing, to make sure it works for the tp-glib case?
(In reply to comment #7) > (From update of attachment 191751 [details] [review]) > looks right... maybe wait until fixing the GError issue as well before > committing, to make sure it works for the tp-glib case? Actually, I fixed that in bug 618193, which seemed more appropriate. Do you think I should squash the patches? They are otherwise unrelated.
no, wasn't suggesting squashing, I was just saying that since Guillaume has not yet successfully tested this bugfix against his code, we might not want to commit/close it yet.
Yep, works fine with these 2 patches. Thanks guys!
Hum actually there is still an issue. conn.get_contacts_by_handle([targetHandle], contactFeatures, Lang.bind(this, function (connection, contacts, failed, wtf, weak_object) { (...) }), null); I add to add this 'wtf' arg to the callback to have the right value of the weak_object param (null here but still). 'wtf' seems to always be null.
(In reply to comment #11) > Hum actually there is still an issue. > > > conn.get_contacts_by_handle([targetHandle], contactFeatures, > Lang.bind(this, function (connection, contacts, failed, > wtf, weak_object) { > (...) > }), null); > > I add to add this 'wtf' arg to the callback to have the right value of the > weak_object param (null here but still). 'wtf' seems to always be null. wtf is the user_data argument. Normally user_data are at the end of the callback (so you can ignore them, in C and in JS), so they go unnoticed, whereas in this case there is another param after it. In gjs you cannot use user_data (it is used for an internal structure), so it is be appropriate to skip this param.
(In reply to comment #12) > (In reply to comment #11) > > Hum actually there is still an issue. > > > > > > conn.get_contacts_by_handle([targetHandle], contactFeatures, > > Lang.bind(this, function (connection, contacts, failed, > > wtf, weak_object) { > > (...) > > }), null); > > > > I add to add this 'wtf' arg to the callback to have the right value of the > > weak_object param (null here but still). 'wtf' seems to always be null. > > wtf is the user_data argument. Normally user_data are at the end of the > callback (so you can ignore them, in C and in JS), so they go unnoticed, > whereas in this case there is another param after it. In gjs you cannot use > user_data (it is used for an internal structure), so it is be appropriate to > skip this param. No wait, I misread the callback signature. wtf is the GError parameter, and it is correctly NULL. (In fact, the user_data should be undefined, being GI_TYPE_TAG_VOID, not null)
Oh, we do expose GError in the JS API? Shouldn't we raise an exception instead?
Created attachment 191876 [details] [review] Skip only user_data in callbacks Previously, all void* arguments in callbacks were skipped. Now we check in the closure id in the caching layer and correctly mark them skipped. Not a real fix, but I think it is cleaner like this.
(In reply to comment #14) > Oh, we do expose GError in the JS API? Shouldn't we raise an exception instead? How can we? Exceptions are thrown out from a function, not inside a function. You could expose a JS Error object instead of a GError, but it wouldn't make much difference.
(In reply to comment #16) > (In reply to comment #14) > > Oh, we do expose GError in the JS API? Shouldn't we raise an exception instead? > > How can we? Exceptions are thrown out from a function, not inside a function. Right. If you want an exception, you should make the API use the gio callback style, where the callback just gets a GAsyncResult*, and you have to pass that to a _finish() function, which takes the GAsyncResult* and a GError**, and either returns the real result(s), or throws an error.
Good point. Fine with me then.
These patches fix my issue and so allow me to remove wrapper in the Shell (bug #654475). Any chance to get them merged?
Review of attachment 191876 [details] [review]: I'm not sure how the second part works; the closure invocation doesn't use the cached function data, right? So I think this change would effectively do the same thing if we just used the deletions.
(In reply to comment #20) > Review of attachment 191876 [details] [review]: > > I'm not sure how the second part works; the closure invocation doesn't use the > cached function data, right? So I think this change would effectively do the > same thing if we just used the deletions. Right now, it does not use the cached data, but it will start doing it if the first patch is merged.
Any updates on this?
(In reply to comment #22) > Any updates on this? It's currently a holiday in the US. Ping Colin on Monday.
Created attachment 206908 [details] [review] Support callbacks that accept array arguments Introduce a caching layer for callback parameters, similar to that used for functions, and use it to support arrays with explicit length.
Created attachment 206909 [details] [review] Skip only user_data in callbacks Previously, all void* arguments in callbacks were skipped. Now we check in the closure id in the caching layer and correctly mark them skipped.
(In reply to comment #23) > (In reply to comment #22) > > Any updates on this? > > It's currently a holiday in the US. Ping Colin on Monday. It's monday (XD). Patches rebased.
Comment on attachment 191749 [details] [review] Add tests for array arguments in callbacks Attachment 191749 [details] pushed as 6757460 - Add tests for array arguments in callbacks
Review of attachment 206908 [details] [review]: Makes sense.
Review of attachment 206909 [details] [review]: Yep.
Attachment 206908 [details] pushed as 0a9e958 - Support callbacks that accept array arguments Attachment 206909 [details] pushed as 97e08e5 - Skip only user_data in callbacks
(In reply to comment #29) > Review of attachment 206909 [details] [review]: > > Yep. No, this commit is wrong, and I reverted. It breaks callbacks that receive void* and expect to ignore it. One example of it is the callback Meta.keybindings_set_custom_handler (so this breaks all global keybindings in gnome-shell).
Is there anything that needs that commit, or is it just a "clean up"?
(In reply to comment #32) > Is there anything that needs that commit, or is it just a "clean up"? Honestly, I don't remember. I'd say "just a clean up", at this point, which makes it useless.
OK, then.