Bug 30422

Summary: Provide GVariant-based access to all a{sv} things
Product: Telepathy Reporter: Simon McVittie <smcv>
Component: tp-glibAssignee: Telepathy bugs list <telepathy-bugs>
Status: RESOLVED MOVED QA Contact: Telepathy bugs list <telepathy-bugs>
Severity: enhancement    
Priority: medium CC: alban.crequy, guillaume.desmottes, xclaesse
Version: unspecified   
Hardware: Other   
OS: All   
Whiteboard:
i915 platform: i915 features:
Bug Depends on: 30423, 48780, 49505, 55024, 55095, 55096, 55099, 55100, 55101, 55103, 55105, 55106, 55107, 55108, 55109    
Bug Blocks: 75204    
Attachments: [1/7] Add tp_account_dup_storage_identifier_variant
[2/7] Add _tp_asv_to_vardict, a utility function to convert a{sv} representations
3/7] account test: rename variables in macros to not shadow normal variables
[4/7] tp_account_dup_storage_specific_information_vardict_async: add
[5/7] tp_account_dup_parameters_vardict: add
[6/7] tp_account_update_parameters_vardict_async: add
[7/7] tp_account_dup_detailed_error_vardict: add
[1/3] tp_connection_manager_param_dup_default_variant: add and test
[2/3] tp_base_connection_disconnect_with_dbus_error_vardict: add and test
[3/3] tp_connection_dup_detailed_error_vardict: add and test
Add tp_capabilities_get_channel_classes_variant()
add _tp_boxed_to_variant()
_tp_asv_to_vardict: use _tp_boxed_to_variant()
Add tp_capabilities_get_channel_classes_variant()
Add tp_capabilities_get_channel_classes_variant()
Add tp_capabilities_get_channel_classes_variant()
Add tp_capabilities_dup_channel_classes_variant()
Add tp_capabilities_dup_channel_classes_variant()
Add tp_capabilities_dup_channel_classes_variant()
test_supports_tube: don't leak the classes array

Description Simon McVittie 2010-09-28 08:47:40 UTC
+++ This bug was initially created as a clone of Bug #28782 +++

As a migration path from dbus-glib to GDBus, we should provide GVariant versions of things like tp_channel_borrow_immutable_properties().

While we're still using dbus-glib, we can do this by depending on dbus-glib 0.88 and calling dbus_g_value_build_g_variant().

To be useful, this requires convenience API to look things up in a{sv}s, for which I'll clone another bug.
Comment 1 Simon McVittie 2010-09-28 08:52:01 UTC
(In reply to comment #0)
> To be useful, this requires convenience API to look things up in a{sv}s

Bug #30423.
Comment 2 Simon McVittie 2010-09-28 09:04:41 UTC
I think this depends on TpVariantMap, because it would be nicer to have tp_channel_borrow_properties_map() (or whatever) return a frozen TpVariantMap rather than requiring the caller to make one.

Methods that need a GVariant version:

* tp_channel_borrow_immutable_properties and ::channel-properties
* tp_channel_dispatch_operation_borrow_immutable_properties and ::cdo-properties
* tp_account_get_parameters
* tp_account_channel_request_get_request
* TpProtocol::protocol-properties

and indirectly (a{sv} in a larger structure):

* tp_capabilities_get_channel_classes and ::channel-classes
Comment 3 Simon McVittie 2010-10-07 03:32:25 UTC
The information in TP_ACCOUNT_FEATURE_STORAGE from Bug #30478 also needs a GVariant equivalent.
Comment 4 Simon McVittie 2012-03-08 06:56:30 UTC
*** Bug 30427 has been marked as a duplicate of this bug. ***
Comment 5 Simon McVittie 2012-03-08 06:59:57 UTC
Created attachment 58189 [details] [review]
[1/7] Add tp_account_dup_storage_identifier_variant

---

As phase 1, to see how feasible it is, I did what this bug asks for TpAccount (only).

There'll be some renaming for next; I'm using naming/transfer conventions from master here.
Comment 6 Simon McVittie 2012-03-08 07:00:18 UTC
Created attachment 58190 [details] [review]
[2/7] Add _tp_asv_to_vardict, a utility function to convert  a{sv} representations
Comment 7 Simon McVittie 2012-03-08 07:00:41 UTC
Created attachment 58191 [details] [review]
3/7] account test: rename variables in macros to not shadow  normal variables
Comment 8 Simon McVittie 2012-03-08 07:01:02 UTC
Created attachment 58192 [details] [review]
[4/7] tp_account_dup_storage_specific_information_vardict_async:  add

This is just like tp_account_get_storage_specific_information_async,
but with more GVariant.
Comment 9 Simon McVittie 2012-03-08 07:01:19 UTC
Created attachment 58193 [details] [review]
[5/7] tp_account_dup_parameters_vardict: add
Comment 10 Simon McVittie 2012-03-08 07:01:35 UTC
Created attachment 58194 [details] [review]
[6/7] tp_account_update_parameters_vardict_async: add
Comment 11 Simon McVittie 2012-03-08 07:01:55 UTC
Created attachment 58195 [details] [review]
[7/7] tp_account_dup_detailed_error_vardict: add
Comment 12 Jonny Lamb 2012-03-08 13:24:15 UTC
Comment on attachment 58194 [details] [review]
[6/7] tp_account_update_parameters_vardict_async: add

Review of attachment 58194 [details] [review]:
-----------------------------------------------------------------

::: tests/dbus/account.c
@@ +268,5 @@
> +  if (!tp_strdiff (data, "vardict"))
> +    {
> +      tp_account_update_parameters_vardict_async (test->account,
> +          g_variant_new_parsed ("{ 'set': <%s> }", "value"), unset,
> +          tp_tests_result_ready_cb, &test->result);

doesn't this leak the new GVariant?
Comment 13 Jonny Lamb 2012-03-08 13:27:47 UTC
The rest looks fine.
Comment 14 Simon McVittie 2012-03-09 03:37:34 UTC
(In reply to comment #12)
> ::: tests/dbus/account.c
> @@ +268,5 @@
> > +  if (!tp_strdiff (data, "vardict"))
> > +    {
> > +      tp_account_update_parameters_vardict_async (test->account,
> > +          g_variant_new_parsed ("{ 'set': <%s> }", "value"), unset,
> > +          tp_tests_result_ready_cb, &test->result);
> 
> doesn't this leak the new GVariant?

I believe it does not, because g_variant_new() and friends are just that clever. (It returns a floating reference, and I made tp_account_update_parameters_vardict_async consume floating references).

I'll valgrind it to make sure this isn't leaking, though.
Comment 15 Simon McVittie 2012-03-09 10:32:29 UTC
(In reply to comment #14)
> I'll valgrind it to make sure this isn't leaking, though.

It isn't, at least with Bug #41125 fixed.

All merged for 0.17.6, thanks.
Comment 16 Simon McVittie 2012-04-11 03:25:10 UTC
(This is still open because there's a lot more a{sv} API to do.)
Comment 17 Simon McVittie 2012-04-12 10:45:13 UTC
Created attachment 59870 [details] [review]
[1/3] tp_connection_manager_param_dup_default_variant: add and  test
Comment 18 Simon McVittie 2012-04-12 10:45:33 UTC
Created attachment 59871 [details] [review]
[2/3] tp_base_connection_disconnect_with_dbus_error_vardict:    add and test
Comment 19 Simon McVittie 2012-04-12 10:45:47 UTC
Created attachment 59872 [details] [review]
[3/3] tp_connection_dup_detailed_error_vardict: add and test
Comment 20 Simon McVittie 2012-04-12 10:53:52 UTC
Here are some more.

I think we should be a little more careful than adding these indiscriminately: it would sometimes be better to have higher-level objects (like TpCapabilities), particularly when the keys are D-Bus property names.

However, I think these three cases do make sense as a plain a{sv}. We could always add accessors to the TpConnection later; for instance, for the keys currently defined in telepathy-spec we could have

    /* Might be NULL */
    gchar *tp_connection_dup_error_server_message (TpConnection *);

    /* true if TP_CONNECTION_STATUS_REASON_REQUESTED or
       { user-requested: True } */
    gboolean tp_connection_get_error_user_requested (TpConnection *);

    /* hopefully non-NULL if
       TP_CONNECTION_STATUS_REASON_CERT_HOSTNAME_MISMATCH */
    gchar *tp_connection_get_error_expected_hostname (TpConnection *);
    gchar *tp_connection_get_error_certificate_hostname (TpConnection *);

(The debug-message is already represented in the GError.)
Comment 21 Jonny Lamb 2012-04-13 15:02:17 UTC
Your three patches look fine.
Comment 22 Simon McVittie 2012-04-16 11:52:36 UTC
(In reply to comment #20)
> it would sometimes be better to have higher-level objects (like
> TpCapabilities), particularly when the keys are D-Bus property names.

Bug #48780 is one such case.
Comment 23 Simon McVittie 2012-04-16 12:00:52 UTC
Some more easy cases that can be done without extra thought:

(In reply to comment #2)
> * tp_channel_borrow_immutable_properties and ::channel-properties

TpChannel is the high-level API. (We should still have accessors for individual properties, though.

> * tp_channel_dispatch_operation_borrow_immutable_properties and
> ::cdo-properties

Similar.

> * TpProtocol::protocol-properties

Similar.

> * tp_capabilities_get_channel_classes and ::channel-classes

Similar.

I think the Parameters on tube channels are also something we can do in a simple way.
Comment 24 Simon McVittie 2012-04-16 12:04:18 UTC
(In reply to comment #21)
> Your three patches look fine.

Thanks, merged.
Comment 25 Guillaume Desmottes 2012-04-23 04:31:00 UTC
(In reply to comment #23)
> > * tp_capabilities_get_channel_classes and ::channel-classes

What should tp_capabilities_get_channel_classes_vadict return exactly? AFAIK there is no GVariant equivalent of TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS.

Should it return an array of TpChannelClass having accessors to get the fixed and allowed props?
Comment 26 Xavier Claessens 2012-04-23 05:07:31 UTC
(In reply to comment #25)
> (In reply to comment #23)
> > > * tp_capabilities_get_channel_classes and ::channel-classes
> 
> What should tp_capabilities_get_channel_classes_vadict return exactly? AFAIK
> there is no GVariant equivalent of TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS.

_vardict is wrong name here since it's not a dictionary.

It should return a GVariant with type a(a{sv}as)

You can create a GVariant for everything that goes over DBus, a(a{sv}as) is no exception. I'm pretty sure you can do almost the same as _tp_asv_to_vardict() but with TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS as GType for the value.

> Should it return an array of TpChannelClass having accessors to get the fixed
> and allowed props?

I don't think we want to add higher level API to expose classes, we already have the _supports_ methods. Also TpChannelClass is very bad name because that's the GObjectClass for TpChannel already.
Comment 27 Simon McVittie 2012-04-23 05:21:19 UTC
(In reply to comment #26)
> _vardict is wrong name here since it's not a dictionary.

Yes, it should be _variant I think. I've been using _vardict for things that return a %G_VARIANT_TYPE_VARDICT (i.e. a{sv}) because they're so common, they deserve an extra hint.

> I'm pretty sure you can do almost the same as _tp_asv_to_vardict()
> but with TP_STRUCT_TYPE_REQUESTABLE_CHANNEL_CLASS as GType for the value.

Yes, do that inline. (_tp_asv_to_vardict() is only worthwhile because a{sv} is so common.)

> > Should it return an array of TpChannelClass having accessors to get the fixed
> > and allowed props?
> 
> I don't think we want to add higher level API to expose classes, we already
> have the _supports_ methods. Also TpChannelClass is very bad name because
> that's the GObjectClass for TpChannel already.

I agree with Xavier.

If we make TpCapabilities iterable at all, my inclination would be to make iterating it return a list of single-channel-class TpCapabilities objects so you can still call supports_X on them (like the way iterating a Python str returns single-character strs).

If you understand the Telepathy D-Bus API well enough to care about fixed and allowed properties, then you understand it well enough to know about property names, IMO. (Where "you" might mean "your application" sometimes.)

I'm curious: what else does Empathy do with TpCapabilities, and can we put it in tp-glib?
Comment 28 Guillaume Desmottes 2012-04-23 05:45:11 UTC
I see; I'll do that then, thanks!

(In reply to comment #27)
> I'm curious: what else does Empathy do with TpCapabilities, and can we put it
> in tp-glib?

That's to check if Conference is supported; see https://bugzilla.gnome.org/show_bug.cgi?id=673843#c1

I'm not sure how we should express that as higher level API; we don't have any high level API for Conference atm.
Comment 29 Guillaume Desmottes 2012-04-23 07:34:12 UTC
Created attachment 60483 [details] [review]
Add tp_capabilities_get_channel_classes_variant()
Comment 30 Guillaume Desmottes 2012-04-25 01:30:12 UTC
Created attachment 60557 [details] [review]
add _tp_boxed_to_variant()
Comment 31 Guillaume Desmottes 2012-04-25 01:30:18 UTC
Created attachment 60558 [details] [review]
_tp_asv_to_vardict: use _tp_boxed_to_variant()
Comment 32 Guillaume Desmottes 2012-04-25 01:30:24 UTC
Created attachment 60559 [details] [review]
Add tp_capabilities_get_channel_classes_variant()
Comment 33 Guillaume Desmottes 2012-04-25 01:44:13 UTC
Created attachment 60560 [details] [review]
Add tp_capabilities_get_channel_classes_variant()
Comment 34 Guillaume Desmottes 2012-04-25 02:50:58 UTC
Created attachment 60563 [details] [review]
Add tp_capabilities_get_channel_classes_variant()
Comment 35 Simon McVittie 2012-04-25 04:38:37 UTC
Comment on attachment 60557 [details] [review]
add _tp_boxed_to_variant()

Review of attachment 60557 [details] [review]:
-----------------------------------------------------------------

Looks OK
Comment 36 Simon McVittie 2012-04-25 04:38:56 UTC
Comment on attachment 60558 [details] [review]
_tp_asv_to_vardict: use _tp_boxed_to_variant()

Review of attachment 60558 [details] [review]:
-----------------------------------------------------------------

Looks OK
Comment 37 Simon McVittie 2012-04-25 05:33:09 UTC
Comment on attachment 60563 [details] [review]
Add tp_capabilities_get_channel_classes_variant()

Review of attachment 60563 [details] [review]:
-----------------------------------------------------------------

Looks generally OK.

::: telepathy-glib/capabilities.c
@@ +907,5 @@
> + *
> + * Since: UNRELEASED
> + */
> +GVariant *
> +tp_capabilities_get_channel_classes_variant (TpCapabilities *self)

I'd personally make this be a _dup_ function, since "copying" (reffing) a GVariant is cheap, and it can't lead to weird side-effects since GVariants are immutable anyway.

Note that if you don't make it _dup_, this method can never be implemented without caching the value in the object.

If it remains a _get_ it should document how long the returned value is valid. (I think the answer is "as long as the #TpCapabilities is", since TpCapabilities are immutable?
Comment 38 Guillaume Desmottes 2012-04-26 00:48:47 UTC
Created attachment 60599 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()
Comment 39 Xavier Claessens 2012-04-30 04:59:39 UTC
Comment on attachment 60599 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()

Review of attachment 60599 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/capabilities.c
@@ +909,5 @@
> + */
> +GVariant *
> +tp_capabilities_dup_channel_classes_variant (TpCapabilities *self)
> +{
> +  return self->priv->classes_variant;

This is not a dup, you should return g_variant_ref()
Comment 40 Guillaume Desmottes 2012-04-30 05:13:59 UTC
Created attachment 60785 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()
Comment 41 Guillaume Desmottes 2012-04-30 05:14:51 UTC
Comment on attachment 60599 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()

Review of attachment 60599 [details] [review]:
-----------------------------------------------------------------

::: telepathy-glib/capabilities.c
@@ +909,5 @@
> + */
> +GVariant *
> +tp_capabilities_dup_channel_classes_variant (TpCapabilities *self)
> +{
> +  return self->priv->classes_variant;

Weird, I'm pretty sure I changed it. I guess I just forgot to record the change when I commited.
Comment 42 Simon McVittie 2012-04-30 06:20:06 UTC
Comment on attachment 60785 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()

Review of attachment 60785 [details] [review]:
-----------------------------------------------------------------

Minor fixes needed. (It would be great if you could valgrind the test to see whether I missed anything.)

::: telepathy-glib/capabilities.c
@@ +183,5 @@
>      case PROP_CHANNEL_CLASSES:
>        self->priv->classes = g_value_dup_boxed (value);
> +      self->priv->classes_variant = _tp_boxed_to_variant (
> +          TP_ARRAY_TYPE_REQUESTABLE_CHANNEL_CLASS_LIST, "a(a{sv}as)",
> +          self->priv->classes);

You don't appear to free this in dispose/finalize?

@@ +901,5 @@
> + * @self: a #TpCapabilities
> + *
> + * Return the #TpCapabilities:channel-classes-variant property
> + *
> + * Returns: (transfer full): the value of

the value of the

::: tests/capabilities.c
@@ +1006,5 @@
> +      TP_PROP_CHANNEL_CHANNEL_TYPE, "&s", &chan_type));
> +  g_assert_cmpstr (chan_type, ==, TP_IFACE_CHANNEL_TYPE_TEXT);
> +
> +  g_assert (g_variant_lookup (fixed,
> +      TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, "u", &handle_type));

A common GVariant trap: handle_type should be a guint32, not a TpHandleType (which we assume is the same size as a guint). guint is not necessarily 32-bit forever.
Comment 43 Guillaume Desmottes 2012-04-30 06:57:00 UTC
(In reply to comment #42)
> Minor fixes needed. (It would be great if you could valgrind the test to see
> whether I missed anything.)

done; I found and fixed an unrelated leak.

> ::: telepathy-glib/capabilities.c
> @@ +183,5 @@
> >      case PROP_CHANNEL_CLASSES:
> >        self->priv->classes = g_value_dup_boxed (value);
> > +      self->priv->classes_variant = _tp_boxed_to_variant (
> > +          TP_ARRAY_TYPE_REQUESTABLE_CHANNEL_CLASS_LIST, "a(a{sv}as)",
> > +          self->priv->classes);
> 
> You don't appear to free this in dispose/finalize?

oooops; fixed.

> @@ +901,5 @@
> > + * @self: a #TpCapabilities
> > + *
> > + * Return the #TpCapabilities:channel-classes-variant property
> > + *
> > + * Returns: (transfer full): the value of
> 
> the value of the

fixed.

> ::: tests/capabilities.c
> @@ +1006,5 @@
> > +      TP_PROP_CHANNEL_CHANNEL_TYPE, "&s", &chan_type));
> > +  g_assert_cmpstr (chan_type, ==, TP_IFACE_CHANNEL_TYPE_TEXT);
> > +
> > +  g_assert (g_variant_lookup (fixed,
> > +      TP_PROP_CHANNEL_TARGET_HANDLE_TYPE, "u", &handle_type));
> 
> A common GVariant trap: handle_type should be a guint32, not a TpHandleType
> (which we assume is the same size as a guint). guint is not necessarily 32-bit
> forever.

Humm that's nasty :\
Can't we do anything to fix this in next?
Comment 44 Guillaume Desmottes 2012-04-30 06:57:28 UTC
Created attachment 60788 [details] [review]
Add tp_capabilities_dup_channel_classes_variant()
Comment 45 Guillaume Desmottes 2012-04-30 06:58:02 UTC
Created attachment 60789 [details] [review]
test_supports_tube: don't leak the classes array
Comment 46 Simon McVittie 2012-04-30 11:16:44 UTC
(In reply to comment #43)
> > A common GVariant trap: handle_type should be a guint32, not a TpHandleType
> > (which we assume is the same size as a guint). guint is not necessarily 32-bit
> > forever.
> 
> Humm that's nasty :\
> Can't we do anything to fix this in next?

Not really. enums are as big as the compiler says they are: strictly speaking, we shouldn't assume they're int-sized either, since ISO C says they can have any type big enough for all the declared values (but I think GObject relies on enums being int-sized too, so we're more or less safe on real-world platforms).

I tried to convince desrt that returning the integer and having a boolean "out" parameter was more forgiving than the other way round, but he wasn't having any of it... so we'll just have to be careful about this.
Comment 47 Guillaume Desmottes 2012-04-30 22:37:37 UTC
I see, thanks for the explanation.

Did you re-review my 2 last patches btw?
Comment 48 Simon McVittie 2012-05-02 03:35:40 UTC
(In reply to comment #47)
> Did you re-review my 2 last patches btw?

I didn't, but I have now, and they look good. Ship it!
Comment 49 Guillaume Desmottes 2012-05-02 06:27:07 UTC
Merged to master for 0.19.0
Comment 50 Xavier Claessens 2012-09-06 11:07:31 UTC
Removing from -next blockers.

This is about adding new functions, so it can be done during 1.x cycles. The real API-break will be when we port to GDBus which is bug #28782.
Comment 51 Simon McVittie 2012-09-19 14:31:01 UTC
(In reply to comment #20)
> I think we should be a little more careful than adding these indiscriminately:
> it would sometimes be better to have higher-level objects (like
> TpCapabilities), particularly when the keys are D-Bus property names.

On the other hand, that's clearly going to take a while, and we shouldn't let potential future perfection prevent improvements.
Comment 52 Simon McVittie 2012-09-19 15:26:58 UTC
I think I've now filed bugs for everything non-deprecated on the client-side. I've ignored the CM side for the moment - CMs are going to have to use dbus-glib directly anyway.
Comment 53 GitLab Migration User 2019-12-03 20:04:03 UTC
-- 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/43.

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.