+++ This bug was initially created as a clone of Bug #30422 +++ TpSimpleClientFactory's methods take a dbus-glib GHashTable. We should have a version that takes a G_VARIANT_TYPE_VARDICT, taking ownership if it is floating. Step 1 is to have methods in the public API, but implement them in terms of the existing virtual methods. Step 2 is to make them virtual, with a default implementation in terms of the existing virtual methods. This is only necessary if you're writing a channel factory with g-i.
Created attachment 94874 [details] [review] client-factory: take a vardict for new account props
Created attachment 94875 [details] [review] logger tests: use a GVariant as params
Created attachment 94876 [details] [review] client-factory: take a vardict for new connection props
Created attachment 94877 [details] [review] file-transfer-chan: set "" as default URI
Created attachment 94878 [details] [review] channel-introspect: pass an empty array as Interfaces prop
Created attachment 94879 [details] [review] client-factory: take a vardict for new channel props
Comment on attachment 94874 [details] [review] client-factory: take a vardict for new account props Review of attachment 94874 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/client-factory.c @@ +483,4 @@ > > + if (immutable_properties != NULL) > + { > + g_variant_ref_sink (immutable_properties); Please sink this *before* passing it to create_account, so that the create_account vfunc can rely on being called with a non-floating vardict. (I'd also be tempted to create an empty a{sv} if it's NULL, so the vfunc doesn't need to cope with NULL.) ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c @@ +93,4 @@ > > fixture->account = tp_client_factory_ensure_account (fixture->factory, > tp_asv_get_string (params, "account-path"), > + tp_asv_to_vardict (params), Leaked. tp_asv_to_vardict() returns a new ref, not a floating ref. (If you want to change its API, now is the time.) ::: tests/logger/dbus/test-tpl-log-walker.c @@ +93,4 @@ > > fixture->account = tp_client_factory_ensure_account (fixture->factory, > tp_asv_get_string (params, "account-path"), > + tp_asv_to_vardict (params), Leaked.
Comment on attachment 94875 [details] [review] logger tests: use a GVariant as params Review of attachment 94875 [details] [review]: ----------------------------------------------------------------- I've only commented on the Pidgin test, but the log walker has the same issue. ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c @@ +830,2 @@ > g_test_add ("/log-iter-xml/get-events", > + PidginTestCaseFixture, create_params (), This is floating, and is consumed by setup(). That's weird. Either sink it and explicitly unref it after g_test_run() like the old code did with the GHashTable, or have the user_data be a static or constant string containing the GVariant string serialization, and parse it in setup().
Comment on attachment 94876 [details] [review] client-factory: take a vardict for new connection props Review of attachment 94876 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/client-factory.c @@ +623,4 @@ > > + if (immutable_properties != NULL) > + { > + g_variant_ref_sink (immutable_properties); As with the account, sink this before calling the vfunc, so the vfunc doesn't have to care.
Comment on attachment 94877 [details] [review] file-transfer-chan: set "" as default URI Review of attachment 94877 [details] [review]: ----------------------------------------------------------------- > NULL isn't a valid D-Bus property, breaking tp_asv_to_vardict(). It's silently treated as "" in most of dbus-glib. The implementation of dbus_g_value_build_g_variant() was wrong in dbus-glib < 0.102, but Xavier fixed that in 0.102 (Bug #71811).
Comment on attachment 94877 [details] [review] file-transfer-chan: set "" as default URI Review of attachment 94877 [details] [review]: ----------------------------------------------------------------- ::: tests/lib/file-transfer-chan.c @@ +694,4 @@ > param_spec = g_param_spec_string ("uri", > "URI", > "The URI property of this channel", > + "", This change is fine though.
Comment on attachment 94878 [details] [review] channel-introspect: pass an empty array as Interfaces prop Review of attachment 94878 [details] [review]: ----------------------------------------------------------------- Again, this change is fine, but if you upgrade to dbus-glib 0.102 it's also unnecessary.
Comment on attachment 94879 [details] [review] client-factory: take a vardict for new channel props Review of attachment 94879 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/base-client.c @@ +1542,4 @@ > _tp_connection_set_account (*connection, *account); > > *channel = tp_client_factory_ensure_channel (self->priv->factory, > + *connection, chan_path, tp_asv_to_vardict (chan_props), error); Non-floating, leaked ::: telepathy-glib/channel-dispatch-operation.c @@ +277,5 @@ > return; > > self->priv->channel = tp_client_factory_ensure_channel ( > + tp_proxy_get_factory (self), self->priv->connection, path, > + tp_asv_to_vardict (properties), &error); Non-floating, leaked ::: telepathy-glib/channel-request.c @@ +222,4 @@ > } > > channel = tp_client_factory_ensure_channel (tp_proxy_get_factory (self), > + connection, chan_path, tp_asv_to_vardict (chan_props), &error); Non-floating, leaked ::: telepathy-glib/client-factory.c @@ +776,4 @@ > > + if (immutable_properties != NULL) > + { > + g_variant_ref_sink (immutable_properties); Sink before calling the vfunc ::: tests/dbus/call-channel.c @@ +154,4 @@ > g_assert_no_error ((GError *) error); > > test->chan = tp_client_factory_ensure_channel (test->factory, > + connection, object_path, tp_asv_to_vardict (immutable_properties), leaked @@ +874,4 @@ > g_assert (test->chan == NULL); > > test->chan = tp_client_factory_ensure_channel (test->factory, > + conn, object_path, tp_asv_to_vardict (properties), &error); leaked ::: tests/dbus/dbus-tube.c @@ +139,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->tube = (TpDBusTubeChannel *) tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/file-transfer-channel.c @@ +216,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->channel = TP_FILE_TRANSFER_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/stream-tube.c @@ +208,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->tube = TP_STREAM_TUBE_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/dbus/text-channel.c @@ +83,4 @@ > > factory = tp_proxy_get_factory (test->connection); > test->channel = TP_TEXT_CHANNEL (tp_client_factory_ensure_channel (factory, > + test->connection, chan_path, tp_asv_to_vardict (props), &test->error)); leaked @@ +107,4 @@ > NULL); > > test->sms_channel = TP_TEXT_CHANNEL (tp_client_factory_ensure_channel ( > + factory, test->connection, chan_path, tp_asv_to_vardict (props), leaked ::: tests/lib/util.c @@ +678,4 @@ > > factory = tp_proxy_get_factory (conn); > return tp_client_factory_ensure_channel (factory, conn, > + object_path, tp_asv_to_vardict (immutable_properties), error); leaked
(In reply to comment #7) > Comment on attachment 94874 [details] [review] [review] > client-factory: take a vardict for new account props > > Review of attachment 94874 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: telepathy-glib/client-factory.c > @@ +483,4 @@ > > > > + if (immutable_properties != NULL) > > + { > > + g_variant_ref_sink (immutable_properties); > > Please sink this *before* passing it to create_account, so that the > create_account vfunc can rely on being called with a non-floating vardict. done. > (I'd also be tempted to create an empty a{sv} if it's NULL, so the vfunc > doesn't need to cope with NULL.) done. (In the 3 functions). > ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c > @@ +93,4 @@ > > > > fixture->account = tp_client_factory_ensure_account (fixture->factory, > > tp_asv_get_string (params, "account-path"), > > + tp_asv_to_vardict (params), > > Leaked. tp_asv_to_vardict() returns a new ref, not a floating ref. > > (If you want to change its API, now is the time.) Yeah I think it's best to return a floating ref: easier to use and more coherent with GVariant's API. I've done that in next already. (In reply to comment #8) > ::: tests/logger/dbus/test-tpl-log-iter-pidgin.c > @@ +830,2 @@ > > g_test_add ("/log-iter-xml/get-events", > > + PidginTestCaseFixture, create_params (), > > This is floating, and is consumed by setup(). That's weird. > > Either sink it and explicitly unref it after g_test_run() like the old code > did with the GHashTable, or have the user_data be a static or constant > string containing the GVariant string serialization, and parse it in setup(). Fixed. Branch updated: http://cgit.collabora.com/git/user/cassidy/telepathy-glib/log/?h=next-factory-55109 (sorry the !fixup have been merged when I rebased on top of the new 'next' branch).
(In reply to comment #14) > > Either sink it and explicitly unref it after g_test_run() like the old code > > did with the GHashTable, or have the user_data be a static or constant > > string containing the GVariant string serialization, and parse it in setup(). > > Fixed. Only in the log walker test; Pidgin too, please. Otherwise looks good. > + immutable_properties = g_variant_new ("a{sv}", NULL); I didn't know that worked. Nice trick.
(In reply to comment #15) > (In reply to comment #14) > > > Either sink it and explicitly unref it after g_test_run() like the old code > > > did with the GHashTable, or have the user_data be a static or constant > > > string containing the GVariant string serialization, and parse it in setup(). > > > > Fixed. > > Only in the log walker test; Pidgin too, please. Oh yeah sorry. Done and merged.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.