|
Description
Simon McVittie
2014-04-08 17:58:08 UTC
Xavier's gdbus-all branch fixes this, although it needs rebasing onto my branch from Bug #77189, and all the CMs need to catch up. Review, ignoring the two trivial gtk-doc commits that I cherry-picked to my gdbus-object branch in Bug #77189: ---- 5d0ab7f2: looks fine ---- 805dfb02: It isn't necessarily a huge problem that this lacks a regression test in telepathy-glib, because Idle and Gabble should be testing it. +static void +channel_died_cb (gpointer data, + GObject *deceased_channel) How can this happen, now that we take a strong ref to the channel? (I think that strong ref is probably a mistake - isn't it circular?) ---- 1460a071: This is really Bug #77197 and I don't see how it relates to channels at all really, so I'll review it over there. ---- e664dd8b: + if (klass->fill_immutable_properties) + klass->fill_immutable_properties (chan, &properties); No need for the conditional, the base class implements it. Not a blocker. This commit does part of Bug #77197 too - it makes TpBaseProtocol's immutable properties GVariant-based. I can see that you wanted to avoid having to have both GHashTable and GVariantDict versions of tp_dbus_properties_mixin_fill_properties_hash(), but it still seems odd to me. Still, we want to do both eventually... + gchar *key = g_strdup_printf ("%s.%s", iface, property); + + g_variant_dict_insert_value (dict, key, + dbus_g_value_build_g_variant (&value)); + g_value_unset (&value); key is leaked > tp_dbus_properties_mixin_fill_properties_hash This should have a different name now. No hash table is involved. fill_properties_dict? ---- General: I think we should make TpBaseChannel and TpBasePasswordChannel use a non-TpSvc* skeleton on this bug too (or clone a bug), to keep all the channel stuff vaguely together. It can be a follow-up branch if that's easier. (In reply to comment #1) > Xavier's gdbus-all branch fixes this, although it needs rebasing onto my > branch from Bug #77189, and all the CMs need to catch up. Rebased my branch. > 805dfb02: > > +static void > +channel_died_cb (gpointer data, > + GObject *deceased_channel) > > How can this happen, now that we take a strong ref to the channel? (I think > that strong ref is probably a mistake - isn't it circular?) The code actually leaked priv->channel, set_property() shouldn't use g_value_dup_object(). I added a fixup commit that change it to be a GWeakRef. > e664dd8b: > > + if (klass->fill_immutable_properties) > + klass->fill_immutable_properties (chan, &properties); > > No need for the conditional, the base class implements it. Not a blocker. Right, added a fixup. > This commit does part of Bug #77197 too - it makes TpBaseProtocol's > immutable properties GVariant-based. I can see that you wanted to avoid > having to have both GHashTable and GVariantDict versions of > tp_dbus_properties_mixin_fill_properties_hash(), but it still seems odd to > me. Still, we want to do both eventually... I did both because it's pretty trivial renaming and we have to stop TpBaseProtocol:immutable-properties from being a TP_HASH_TYPE_QUALIFIED_PROPERTY_VALUE_MAP anyway. Are you ok leaving both in the same commit or do you want to split by having temporally both GVariantDict and GHashTable versions in properties mixin ? > + gchar *key = g_strdup_printf ("%s.%s", iface, property); > + > + g_variant_dict_insert_value (dict, key, > + dbus_g_value_build_g_variant (&value)); > + g_value_unset (&value); > > key is leaked Added fixup. > > tp_dbus_properties_mixin_fill_properties_hash > > This should have a different name now. No hash table is involved. > fill_properties_dict? I ran a s/_hash// on all files. > ---- > > General: > > I think we should make TpBaseChannel and TpBasePasswordChannel use a > non-TpSvc* skeleton on this bug too (or clone a bug), to keep all the > channel stuff vaguely together. It can be a follow-up branch if that's > easier. Yep, that's my goal :) Created attachment 106446 [details] [review] TpBaseProtocol: GVariant-ify TpCMParamSpec From: Xavier Claessens <xavier.claessens@collabora.com> --- (Includes all fixup patches) Created attachment 106447 [details] [review] TpBaseChannel:fill_immutable_properties: Take a GVariantDict From: Xavier Claessens <xavier.claessens@collabora.com> --- (likewise) Created attachment 106448 [details] [review] tp_base_protocol_sanitize_parameters: set error if unable to coerce Created attachment 106449 [details] [review] tp_variant_convert: support converting to/from (u)int16 Now that we're using native GVariant instead of GValue, (u)int16 can happen. Created attachment 106450 [details] [review] tp_cm_param_filter_uint_nonzero: allow it to be used with guint16 Created attachment 106451 [details] [review] [gabble] Adapt to tp-glib: fill channel properties in terms of GVariant, redo TpCMParamSpec as objects Created attachment 106452 [details] [review] [idle] idle_tls_certificate_class_init: tp_dbus_properties_mixin_class_init has gone away Created attachment 106453 [details] [review] [idle] Update for GVariant TpCMParamSpec and GVariant channel properties --- I think this is right, but the tests just hang, so... maybe not. (In reply to comment #1) > Xavier's gdbus-all branch fixes this, although it needs rebasing onto my > branch from Bug #77189, and all the CMs need to catch up. "All the CMs need to catch up" is secret code for "... and also, telepathy-glib needs to be fixed such that their tests can pass again". I tried to land a greater proportion of gdbus-all at once, but my conclusion was "that's too much to do as one chunk of work and won't get finished". -- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-glib/issues/126. |
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.