|
Description
Simon McVittie
2010-09-28 08:47:40 UTC
(In reply to comment #0) > To be useful, this requires convenience API to look things up in a{sv}s Bug #30423. 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
The information in TP_ACCOUNT_FEATURE_STORAGE from Bug #30478 also needs a GVariant equivalent. *** Bug 30427 has been marked as a duplicate of this bug. *** 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. Created attachment 58190 [details] [review] [2/7] Add _tp_asv_to_vardict, a utility function to convert a{sv} representations Created attachment 58191 [details] [review] 3/7] account test: rename variables in macros to not shadow normal variables 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. Created attachment 58193 [details] [review] [5/7] tp_account_dup_parameters_vardict: add Created attachment 58194 [details] [review] [6/7] tp_account_update_parameters_vardict_async: add Created attachment 58195 [details] [review] [7/7] tp_account_dup_detailed_error_vardict: add 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? The rest looks fine. (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. (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. (This is still open because there's a lot more a{sv} API to do.)
Created attachment 59870 [details] [review] [1/3] tp_connection_manager_param_dup_default_variant: add and test Created attachment 59871 [details] [review] [2/3] tp_base_connection_disconnect_with_dbus_error_vardict: add and test Created attachment 59872 [details] [review] [3/3] tp_connection_dup_detailed_error_vardict: add and test 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.)
Your three patches look fine. (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. 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. (In reply to comment #21) > Your three patches look fine. Thanks, merged. (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? (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. (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? 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. Created attachment 60483 [details] [review] Add tp_capabilities_get_channel_classes_variant() Created attachment 60557 [details] [review] add _tp_boxed_to_variant() Created attachment 60558 [details] [review] _tp_asv_to_vardict: use _tp_boxed_to_variant() Created attachment 60559 [details] [review] Add tp_capabilities_get_channel_classes_variant() Created attachment 60560 [details] [review] Add tp_capabilities_get_channel_classes_variant() Created attachment 60563 [details] [review] Add tp_capabilities_get_channel_classes_variant() Comment on attachment 60557 [details] [review] add _tp_boxed_to_variant() Review of attachment 60557 [details] [review]: ----------------------------------------------------------------- Looks OK Comment on attachment 60558 [details] [review] _tp_asv_to_vardict: use _tp_boxed_to_variant() Review of attachment 60558 [details] [review]: ----------------------------------------------------------------- Looks OK 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? Created attachment 60599 [details] [review] Add tp_capabilities_dup_channel_classes_variant() 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() Created attachment 60785 [details] [review] Add tp_capabilities_dup_channel_classes_variant() 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 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. (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? Created attachment 60788 [details] [review] Add tp_capabilities_dup_channel_classes_variant() Created attachment 60789 [details] [review] test_supports_tube: don't leak the classes array (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. I see, thanks for the explanation. Did you re-review my 2 last patches btw? (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! Merged to master for 0.19.0 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. (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. 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. -- 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.