[Bug 24107] support Statuses on TpConnection

Fri May 7 14:39:57 CEST 2010

https://bugs.freedesktop.org/show_bug.cgi?id=24107

--- Comment #6 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-05-07 05:39:57 PDT ---
Review of attachment 35489:
 --> (https://bugs.freedesktop.org/review?bug=24107&attachment=35489)

::: docs/reference/telepathy-glib-sections.txt
@@ +2780,3 @@
+tp_connection_get_allowed_presence_statuses
+TpConnectionSetSelfPresenceCb
+tp_connection_set_self_presence

The feature name should also appear here. It's probably worth having a
<SUBSECTION> with the feature name as the first thing.

The underlying function for the feature should be added to a <SUBSECTION
Standard> nearby, otherwise gtk-doc will complain.

The typedef, the TP_TYPE_FOO #define and any public API for the struct also
need documenting. Please `make check` with --enable-gtk-doc and it will whine
about anything you haven't included.

::: telepathy-glib/connection.c
@@ +2304,3 @@
+ * statuses that the client does not recognise (as explained in SetPresence),
+ * but MAY be used to check that the client's assumptions about a particular
+ * status name match the connection manager's.

This should be a TpConnectionPresenceType.

SetPresence doesn't exist in this context, and telepathy-glib doesn't have
RFC-style MUSTs. How about this?

"This should not be used as a way to set statuses that the client does not
recognise (see SetPresence in the Telepathy D-Bus Interface Specification), but
may be used to check that the client's assumptions about a particular status
name match the connection manager's."

Bonus points if you hyperlink to the right place :-) (don't worry about
exceeding the usual 80-character limit if you do).

@@ +2307,3 @@
+ * @status: The string identifier of this status.
+ * @may_set_on_self: If true, the user can set this status on themselves using
+ * tp_connection_set_self_presence.

Use tp_connection_set_self_presence() to make a gtk-doc hyperlink (but it'll be
tp_connection_set_self_presence_async() anyway).

@@ +2312,3 @@
+ * be Away with a status message, but if you are available you cannot set a
+ * status message.
+ *

This text suffers as a result of losing the <tp:rationale> markup. I suggest:

"(For instance, on IRC you can be Away with a status message, but if you are
available you cannot set a status message.)"

@@ +2315,3 @@
+ * The possible presence statuses for this connection in its current status.
+ *
+ * Since: 0.11.5

You're assuming that your branch will be merged before the next release. Please
use 0.11.UNRELEASED (with that capitalization); I replace all occurrences of
UNRELEASED during the release process (and there's a `make check` hook to make
sure I do).

@@ +2323,3 @@
+ * The boxed type of a #TpStatusSpec.
+ *
+ * Since: 0.11.5

0.11.UNRELEASED

@@ +2353,3 @@
+ * Returns: a newly allocated #TpStatusSpec, free it with
+ * tp_status_spec_destroy()
+ * Since: 0.11.5

0.11.UNRELEASED

@@ +2356,3 @@
+ */
+TpStatusSpec *
+tp_status_spec_new (guint type,

TpConnectionPresenceType type

@@ +2357,3 @@
+TpStatusSpec *
+tp_status_spec_new (guint type,
+                    gchar *status,

const gchar *status

@@ +2363,3 @@
+  TpStatusSpec *self;
+
+  self = g_slice_new (TpStatusSpec);

I prefer g_slice_new0 (which will zero-fill the @priv member, here).

@@ +2380,3 @@
+ * Returns: a newly allocated #TpStatusSpec, free it with
+ * tp_status_spec_destroy()
+ * Since: 0.11.5

0.11.UNRELEASED

@@ +2399,3 @@
+ * Free all memory used by the #TpStatusSpec.
+ *
+ * Since: 0.11.5

0.11.UNRELEASED

@@ +2418,3 @@
+ * The value may have changed arbitrarily during the time the Connection
+ * spends in status TP_CONNECTION_STATUS_CONNECTING, again staying fixed for
+ * the entire time in TP_CONNECTION_STATUS_CONNECTED.

%TP_CONNECTION_STATUS_CONNECTING, %TP_CONNECTION_STATUS_CONNECTED

@@ +2420,3 @@
+ * the entire time in TP_CONNECTION_STATUS_CONNECTED.
+ *
+ * This method requires TP_CONNECTION_FEATURE_SELF_PRESENCE to be enabled. 

%TP_CONNECTION_FEATURE_SELF_PRESENCE

@@ +2424,3 @@
+ * with the feature %TP_CONNECTION_FEATURE_SELF_PRESENCE.
+ *
+ * Returns: (element-type utf8 StatusSpec): Dictionary from string identifiers
to structs for each valid status.

(element-type utf8 TelepathyGLib.StatusSpec)

@@ +2426,3 @@
+ * Returns: (element-type utf8 StatusSpec): Dictionary from string identifiers
to structs for each valid status.
+ *
+ * Since: 0.11.5

0.11.UNRELEASED

@@ +2437,3 @@
+  GHashTable *dict = g_hash_table_new_full (g_str_hash, g_str_equal, g_free,
+                                            (GDestroyNotify)
tp_status_spec_destroy);
+  TpStatusSpec *status = tp_status_spec_new (1, "brb", TRUE, TRUE);

Please use an appropriate enum member, even in throwaway code

@@ +2441,3 @@
+  
+  g_hash_table_ref (dict);
+  self->priv->allowed_presence_statuses = dict;

You're leaking one reference to dict - new_full gives you one ref, ref gives
you a second, and assigning to self->priv->allowed_presence_statuses takes one.

You'll leak the old self->priv->allowed_presence_statuses if you do this
without checking for NULL.

@@ +2450,3 @@
+ * tp_connection_set_self_presence:
+ * @self: a connection
+ * @status: the desired status, one of the statuses returned by

cons

@@ +2467,3 @@
+void
+tp_connection_set_self_presence (TpConnection *self, gchar *status,
+    gchar *status_message, GAsyncReadyCallback callback, gpointer user_data)

Break arguments one per line in definitions.

Both string arguments should be of type const gchar *.

::: telepathy-glib/connection.h
@@ +91,3 @@
+struct _TpStatusSpec
+{
+  guint type;

TpConnectionPresenceType

@@ +97,3 @@
+
+  /*<private>*/
+  gpointer _1;

gpointer priv

@@ +103,3 @@
+GType tp_status_spec_get_type (void);
+
+TpStatusSpec * tp_status_spec_new (

TpStatusSpec *tp_status_spec_new, sans space.

Also, this should be G_GNUC_WARN_UNUSED_RESULT or whatever it's called.

(Also, "(guint type," without the intervening newline would be conventional)

@@ +105,3 @@
+TpStatusSpec * tp_status_spec_new (
+    guint type,
+    gchar *status,

TpConnectionPresenceType type, and const gchar *status, please

@@ +109,3 @@
+    gboolean can_have_message);
+
+TpStatusSpec * tp_status_spec_copy (TpStatusSpec *self);

TpStatusSpec *tp_status_spec_copy

@@ +186,3 @@
     const GHashTable **details);

+GHashTable *tp_connection_get_allowed_presence_statuses (TpConnection *self);

I'd prefer these to appear just below TP_CONNECTION_FEATURE_SELF_PRESENCE in
the header, for a visual link.

@@ +188,3 @@
+GHashTable *tp_connection_get_allowed_presence_statuses (TpConnection *self);
+
+void tp_connection_set_self_presence (TpConnection *self, gchar *status,

set_self_presence_async, and it needs a corresponding _finish function, and the
strings should be const

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.