Bug 55024 - Provide tp_stream_tube_channel_dup_parameters_vardict() returning GVariant
Summary: Provide tp_stream_tube_channel_dup_parameters_vardict() returning GVariant
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: tp-glib (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Simon McVittie
QA Contact: Telepathy bugs list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 30422
  Show dependency treegraph
 
Reported: 2012-09-17 15:38 UTC by Chandni Verma
Modified: 2012-09-26 11:50 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
TpDBusTube: obtain parameters as vardict (4.93 KB, patch)
2012-09-17 15:48 UTC, Chandni Verma
Details | Splinter Review
TpStreamTubeChannel: add parameters-vardict and its getter (9.13 KB, patch)
2012-09-19 14:41 UTC, Simon McVittie
Details | Splinter Review
TpDBusTubeChannel: add parameters-vardict (3.39 KB, patch)
2012-09-19 14:41 UTC, Simon McVittie
Details | Splinter Review
Add ':' to tp_dbus_tube_channel_dup_parameters_vardict comments (829 bytes, patch)
2012-09-24 09:52 UTC, Chandni Verma
Details | Splinter Review

Description Chandni Verma 2012-09-17 15:38:59 UTC
This one is a sub to https://bugs.freedesktop.org/show_bug.cgi?id=30422
and makes it easier to achieve access to parameters property using high level languages like vala.
Comment 1 Simon McVittie 2012-09-17 15:48:08 UTC
Stream tube parameters would be nice too (they're basically the same).
Comment 2 Chandni Verma 2012-09-17 15:48:09 UTC
Created attachment 67287 [details] [review]
TpDBusTube:  obtain parameters as vardict
Comment 3 Simon McVittie 2012-09-17 15:56:33 UTC
Comment on attachment 67287 [details] [review]
TpDBusTube:  obtain parameters as vardict

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

This looks great. Want to add the stream-tube equivalent while you're here? :-)

A few minor documentation things, which I'll just fix myself while I'm committing it:

::: telepathy-glib/dbus-tube-channel.c
@@ +517,5 @@
> + * @self: a #TpDBusTubeChannel
> + *
> + * Return the parameters of the dbus-tube channel in a variant of
> + * type %G_VARIANT_TYPE_VARDICT whose keys are strings representing 
> + * parameter names and values are GValues representing corresponding 

... and values are variants representing ...

@@ +525,5 @@
> + * yet been offered or the parameters property has not been set.
> + *
> + * Use g_variant_lookup() or g_variant_lookup_value() for convenient
> + * access to the values, the expected types of which should match the ones
> + * defined upon the creation of the parameters #GHashTable.

The second clause "the expected types..." looks like copy/paste damage, it's not really particularly relevant here. I'd replace this sentence with something like:

    Use g_variant_lookup(), g_variant_lookup_value() or functions like tp_vardict_get_uint32() for convenient access to the values.

(tp_vardict_get_uint32 is new in master.)

@@ +527,5 @@
> + * Use g_variant_lookup() or g_variant_lookup_value() for convenient
> + * access to the values, the expected types of which should match the ones
> + * defined upon the creation of the parameters #GHashTable.
> + *
> + * Since: 0.20.0

We say "Since: 0.UNRELEASED" in patches, and whoever does the release greps for UNRELEASED and fixes the reference. Otherwise, if your patch didn't get applied until 0.21.5 or something, it'd be inaccurate.
Comment 4 Simon McVittie 2012-09-17 16:08:46 UTC
Thanks, committed for 0.19.10 with the documentation adjustments I mentioned.

For future reference, it also needed a Returns: line:

 * Returns: (transfer full): a new reference to a #GVariant

and please avoid trailing whitespace in future (git should generally warn you about that, I think?)
Comment 5 Simon McVittie 2012-09-17 16:16:55 UTC
Retitling to what's left, feel free to assign to yourself if you intend to work on this.
Comment 6 Simon McVittie 2012-09-19 14:41:24 UTC
Created attachment 67388 [details] [review]
TpStreamTubeChannel: add parameters-vardict and its  getter

---

Much like Chandni did for TpDBusTubeChannel, but with the addition of a "parameters-vardict" property.
Comment 7 Simon McVittie 2012-09-19 14:41:43 UTC
Created attachment 67389 [details] [review]
TpDBusTubeChannel: add parameters-vardict
Comment 8 Chandni Verma 2012-09-21 13:01:20 UTC
I read the patch and it looks fine to me. I didn't apply or did a make check though. I think this patch is important to make this bug completely fixed.
Comment 9 Chandni Verma 2012-09-24 09:52:07 UTC
Created attachment 67622 [details] [review]
Add ':' to tp_dbus_tube_channel_dup_parameters_vardict comments

I checked out the applied patch from the master again and found that my build was failing at a missing ':' so attaching a correction.
Comment 10 Simon McVittie 2012-09-24 10:15:08 UTC
Comment on attachment 67622 [details] [review]
Add ':' to tp_dbus_tube_channel_dup_parameters_vardict comments

(In reply to comment #9)
> I checked out the applied patch from the master again and found that my
> build was failing at a missing ':' so attaching a correction.

Travis already fixed this in <http://cgit.freedesktop.org/telepathy/telepathy-glib/commit/?id=1bbda3bc>.
Comment 11 Chandni Verma 2012-09-24 11:06:10 UTC
Oh, then it's fine! I probably missed that commit due to insufficient commit message.
Comment 12 Guillaume Desmottes 2012-09-26 11:30:46 UTC
Comment on attachment 67388 [details] [review]
TpStreamTubeChannel: add parameters-vardict and its  getter

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

++
Comment 13 Guillaume Desmottes 2012-09-26 11:31:18 UTC
Comment on attachment 67389 [details] [review]
TpDBusTubeChannel: add parameters-vardict

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

++
Comment 14 Guillaume Desmottes 2012-09-26 11:50:02 UTC
Merged to master for 0.19.10 (or 0.20.0 depending on which one we release).


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.