|
Description
Simon McVittie
2014-04-22 18:24:32 UTC
Created attachment 99069 [details] [review] add tp_intset_to_variant() Created attachment 99070 [details] [review] add tp_intset_from_variant() Created attachment 99071 [details] [review] handle-set: add GVariant API Comment on attachment 99069 [details] [review] add tp_intset_to_variant() Review of attachment 99069 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/intset.c @@ +395,5 @@ > + * > + * <!--Returns: says it all--> > + * > + * Returns: (transfer full): a new #GVariant of type 'au' > + * containing the same integers as @set. It looks as though you're returning a new *floating* GVariant. Please document that explicitly in the text, and I think (transfer none) is the correct annotation (check e.g. g_variant_new_uint32() docs if unsure, it should match). "of type 'au' (array of uint32)" might be clearer. ::: tests/intset.c @@ +204,5 @@ > + GVariant *v; > + > + v = tp_intset_to_variant (a); > + g_assert (g_variant_is_of_type (v, G_VARIANT_TYPE ("au"))); > + g_variant_unref (v); I'd prefer to g_variant_ref_sink() it before the assertion. Comment on attachment 99070 [details] [review] add tp_intset_from_variant() Review of attachment 99070 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/intset.c @@ +417,5 @@ > + * @variant: a #GVariant of type 'au', consumed if floating > + * > + * <!--Returns: says it all--> > + * > + * Returns: A set containing the same integers as @variant. Explicit is better than implicit: Returns: (transfer full): A new set ... Comment on attachment 99071 [details] [review] handle-set: add GVariant API Review of attachment 99071 [details] [review]: ----------------------------------------------------------------- ::: telepathy-glib/handle-set.c @@ +299,5 @@ > + * > + * <!--Returns: says it all, this comment is just to keep gtkdoc happy--> > + * > + * Returns: (transfer full): a new #GVariant of type 'au' > + * representing the handles in the set Same comments about floating as for tp_intset_to_variant It would be nice to replace the uses of to_array and from_array in base-contact-list.c (and perhaps tests/lib/contact-list-manager.c), which I think currently convert the array into a variant. It would eventually be nice to deprecate or remove those functions entirely, but I don't think that needs to be a 1.0 blocker. (In reply to comment #7) > It would eventually be nice to deprecate or remove those functions entirely, > but I don't think that needs to be a 1.0 blocker. Other users of to_array, from_array: TpGroupMixin Gabble (muc-channel, muc-factory use to_array only) so I'm tempted to say: make them internal to telepathy-glib (_tp_*), and either convert the Gabble MUC code to work in terms of GVariant<au>, TpIntSet or TpHandleSet, or give Gabble a stupid "gabble_au_variant_to_garray()" for now. Created attachment 99086 [details] [review] add tp_intset_to_variant() Created attachment 99087 [details] [review] add tp_intset_from_variant() Created attachment 99088 [details] [review] handle-set: add GVariant API Created attachment 99089 [details] [review] base-contact-list: use tp_handle_set_new_from_variant() Created attachment 99090 [details] [review] add tp_handles_are_valid_variant() Created attachment 99091 [details] [review] base-contact-list: use tp_handles_are_valid_variant() Created attachment 99092 [details] [review] base-contact-list: start using tp_handle_set_to_variant() Created attachment 99142 [details] [review] remove tp_handle_set_new_from_array() Created attachment 99143 [details] [review] stop using tp_handle_set_to_array() in tests Created attachment 99144 [details] [review] move tp_handle_set_to_array() as private API Comment on attachment 99086 [details] [review] add tp_intset_to_variant() Review of attachment 99086 [details] [review]: ----------------------------------------------------------------- Looks good Comment on attachment 99087 [details] [review] add tp_intset_from_variant() Review of attachment 99087 [details] [review]: ----------------------------------------------------------------- Looks good Comment on attachment 99088 [details] [review] handle-set: add GVariant API Review of attachment 99088 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 99089 [details] [review] base-contact-list: use tp_handle_set_new_from_variant() Review of attachment 99089 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 99090 [details] [review] add tp_handles_are_valid_variant() Review of attachment 99090 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 99091 [details] [review] base-contact-list: use tp_handles_are_valid_variant() Review of attachment 99091 [details] [review]: ----------------------------------------------------------------- ++ Comment on attachment 99092 [details] [review] base-contact-list: start using tp_handle_set_to_variant() Review of attachment 99092 [details] [review]: ----------------------------------------------------------------- ++ > We are still using tp_handle_set_to_array() before calling > tp_base_connection_dup_contact_attributes(). Not sure if we should have a > version of this API taking a GVariant as well or not. IMO, tp_base_connection_dup_contact_attributes() should eventually either take a GVariant<au> or a TpHandleSet. Comment on attachment 99142 [details] [review] remove tp_handle_set_new_from_array() Review of attachment 99142 [details] [review]: ----------------------------------------------------------------- \o/ Comment on attachment 99143 [details] [review] stop using tp_handle_set_to_array() in tests Review of attachment 99143 [details] [review]: ----------------------------------------------------------------- ::: tests/lib/contact-list-manager.c @@ +431,5 @@ > + gconstpointer arr; > + gsize len; > + > + handles = tp_handle_set_to_variant (contacts); > + arr = g_variant_get_fixed_array (handles, &len, sizeof (TpHandle)); I prefer to sink floating refs before calling any other API on them (other than "add to container" which sinks the floating ref), to make it clear what's going on. Comment on attachment 99144 [details] [review] move tp_handle_set_to_array() as private API Review of attachment 99144 [details] [review]: ----------------------------------------------------------------- Looks good. Did you also patch Gabble? (In reply to comment #29) > Did you also patch Gabble? Oh, yes, you did. Sorry, I didn't spot that. The Gabble patches look fine and are exactly what I had in mind. That implementation is not wonderful, but will probably fall off if/when we reduce TpGroupMixin's API "surface area" (or supersede it with TpGroupMixin2 if Telepathy 1.0 happens first). Comment on attachment 99143 [details] [review] stop using tp_handle_set_to_array() in tests Review of attachment 99143 [details] [review]: ----------------------------------------------------------------- I merged the tp-glib and gabble branches. ::: tests/lib/contact-list-manager.c @@ +431,5 @@ > + gconstpointer arr; > + gsize len; > + > + handles = tp_handle_set_to_variant (contacts); > + arr = g_variant_get_fixed_array (handles, &len, sizeof (TpHandle)); fixed. (In reply to comment #26) > Comment on attachment 99092 [details] [review] [review] > base-contact-list: start using tp_handle_set_to_variant() > > Review of attachment 99092 [details] [review] [review]: > ----------------------------------------------------------------- > > ++ > > > We are still using tp_handle_set_to_array() before calling > > tp_base_connection_dup_contact_attributes(). Not sure if we should have a > > version of this API taking a GVariant as well or not. > > IMO, tp_base_connection_dup_contact_attributes() should eventually either > take a GVariant<au> or a TpHandleSet. Still TODO. Created attachment 99307 [details] [review] [tp-glib] tp_base_connection_dup_contact_attributes: take a TpHandleSet Created attachment 99308 [details] [review] [gabble] tp_base_connection_dup_contact_attributes() now takes a TpHandleSet The other components don't need to be updated. Fixed in git for 0.99.12, thanks |
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.