Bug 24107

Now that we have tp_proxy_prepare_async(), this shouldn't be hard for someone to work on; please look at telepathy-qt4 for design ideas, since we already designed this once :-)

*** Bug 19097 has been marked as a duplicate of this bug. ***

Created attachment 35463 [details] [review]
Add stubs to TpConnection for setting the self presence and getting the allowed statuses

Created attachment 35489 [details] [review]
Add stubs to TpConnection for setting the self presence and getting the allowed statuses

Created attachment 35490 [details]
example of use in python

Review of attachment 35489 [details] [review]:

::: 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

Created attachment 35499 [details] [review]
Add tp_connection_get_allowed_presence_statuses and tp_connection_set_self_presence_async

Created attachment 35501 [details]
example of use in python

Actually works!

Review of attachment 35499 [details] [review]:

I don't see a regression test here? :-)

Unfortunately the only example/test CM we have that implements this is examples/cm/contactlist, which has an extremely simple concept of presence (a boolean "away?" flag), but you could enhance that to store and re-broadcast the message?

(See tests/dbus/callable.c, and its piece of Makefile.am, if you need an example of how a test can link against an example CM.)

::: telepathy-glib/connection.c
@@ +61,3 @@
  * <listitem>connection status tracking</listitem>
  * <listitem>calling GetInterfaces() automatically</listitem>
+ * <listitem>Getting the valid presence statuses automatically</listitem>

I don't think this is really appropriate in a list of things done automatically, given that you have to opt in. Just remove it.

We should re-do this introduction to reflect the current recommendation to do everything possible through high-level API and de-emphasis of generated API, but that's a job for another branch.

@@ +2465,3 @@
+  self = g_slice_new0 (TpStatusSpec);
+  self->type = type;
+  self->status = g_strdup (status);

I'd like a g_return_if_fail that @status is non-NULL, I think.

@@ +2518,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.

You don't need this paragraph if you refuse to retrieve Statuses before going CONNECTED, as you currently do.

(But, don't do that... see elsewhere.)

@@ +2537,3 @@
+    g_hash_table_ref (self->priv->allowed_presence_statuses);
+  return self->priv->allowed_presence_statuses;
+}

Blank lines around the if block, please (i.e. add a blank line before the "return", in this case).

@@ +2559,3 @@
+ * @status: the desired status, one of the statuses returned by
+ * tp_connection_get_allowed_presence_statuses()
+ * @status_message: desired status message

I think it would be nice to accept NULL for this, and silently translate that to "".

@@ +2588,3 @@
+      status, status_message, tp_connection_set_self_presence_cb, result, NULL,
+      G_OBJECT (self));
+}

Don't pass @self as the weak_object. TpProxy refs itself for as long as a tp_cli call (with a non-NULL callback) is in-flight, so it can't die; if the callback needs it, it can get it from @proxy.

If @self wasn't automatically reffed in this way, then your purported API guarantee of "called exactly once" would not be true, so it's a good thing it is :-)

The callback you've written would crash if @callback is NULL. You could fix that in the callback, but that's actually not ideal.

For better D-Bus efficiency, what you should actually do is to use a NULL callback instead of tp_connection_set_self_presence_cb if the reply is going to be ignored. If you do that, the proxy will just fire off a D-Bus message with the "no reply needed" flag, and forget about it; the CM won't bother to reply, and libdbus, dbus-glib, TpProxy and dbus-daemon won't try to track the reply either.

(In reply to comment #8)
> Created an attachment (id=35501) [details]
> example of use in python
> 
> Actually works!

I was about to say "you could put this in examples/client", but in fact we want to encourage setting statuses via the account manager in general, so don't do that.

MC could benefit from this API, though.

(In reply to comment #9)
> @@ +2518,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.
> 
> You don't need this paragraph if you refuse to retrieve Statuses before going
> CONNECTED, as you currently do.
> 
> (But, don't do that... see elsewhere.)

"Elsewhere" is this comment. I was going to explain this in the review, but I forgot :-)

Statuses is useful before CONNECTED (particularly inside MC), so the fact that you check for ->ready is a problem.

Ideally it should behave like Interfaces. We don't actually download that using properties at the moment - we still use GetInterfaces - but the principle is the same.

Interesting strings to search for: introspecting_after_connected, tp_connection_got_interfaces_cb.

The API I'd like is this:

* Whenever we start to fill the introspection queue (right now this means tp_connection_got_interfaces_cb), enqueue a call to maybe_prepare_self_presence

* At the time that that callback is called, if we don't actually care, it'll just do nothing

* After the real, underlying status goes to CONNECTED (you can tell by introspecting_after_connected), re-fetch Statuses if it's wanted, before signalling TP_CONNECTION_FEATURE_CONNECTED up to user code

You can probably achieve that fairly easily by storing a boolean "final_statuses" alongside the hash table, and changing the check for "do we already have the statuses?" to only do the early-return if (we already have the statuses && (final_statuses || introspecting_after_connected)).

Created attachment 35769 [details] [review]
Add tp_connection_get_allowed_presence_statuses and tp_connection_set_self_presence_async

(In reply to comment #12)
> Created an attachment (id=35769) [details]
> Add tp_connection_get_allowed_presence_statuses and
> tp_connection_set_self_presence_async

Addresses comments and adds tests

Review of attachment 35769 [details] [review]:

::: telepathy-glib/connection.c
@@ +486,3 @@
+      k,
+      g_value_get_boolean (value->values + 1),
+      g_value_get_boolean (value->values + 2));

You could use tp_value_array_unpack().

@@ +516,3 @@
+  self->priv->allowed_presence_statuses = g_hash_table_new_full (g_str_hash,
+      g_str_equal, g_free, (GDestroyNotify) tp_status_spec_destroy);
+

Blank line after the if/unref block, please

@@ +521,3 @@
+
+  g_hash_table_foreach (statuses, allowed_presence_statuses_foreach,
+                        self->priv->allowed_presence_statuses);

GHashTableIter is often more readable than g_hash_table_foreach?

@@ +537,3 @@
+  if ((self->priv->allowed_presence_statuses != NULL) &&
+        (self->priv->final_statuses))
+    return;   /* already done */

This has more parentheses than it needs to. && has lower precedence than != and we assume C programmers know that.

@@ +551,3 @@
+        (self->priv->final_statuses))
+    return;   /* not interested right now */
+

You don't need the parentheses here - && has low precedence.

@@ +2492,3 @@
+
+/**
+ * tp_status_spec_copy:

Should this be (skip)?

@@ +2509,3 @@
+                             self->status,
+                             self->may_set_on_self,
+                             self->can_have_message);

We don't indent multi-line arguments like that; see http://telepathy.freedesktop.org/wiki/Style

@@ +2513,3 @@
+
+/**
+ * tp_status_spec_destroy:

Should this be (skip)?

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

In telepathy-glib we usually make these function-based accessors not return a new ref, with a convention of using _dup_ in the name of an accessor that does return a copy.

Should this be made available as a GObject property too, with this function as a "C binding"?

@@ +2615,3 @@
+  else
+    tp_cli_connection_interface_simple_presence_call_set_presence (self, -1,
+        status, status_message, NULL, NULL, NULL, NULL);

The "if" block has {}, which means the "else" needs them too, even though it's only one line.

::: telepathy-glib/connection.h
@@ +105,3 @@
+TpStatusSpec *tp_status_spec_new (TpConnectionPresenceType type,
+    const gchar *status, gboolean may_set_on_self, gboolean can_have_message)
+  G_GNUC_WARN_UNUSED_RESULT;

Does this need to be public API? If not, put it in connection-internal.h and prefix it with an underscore.

@@ +108,3 @@
+
+TpStatusSpec *tp_status_spec_copy (TpStatusSpec *self)
+  G_GNUC_WARN_UNUSED_RESULT;

The argument should be const

::: tests/lib/contacts-conn.c
@@ +320,1 @@
     return FALSE;

You don't need this many parentheses. == and != have higher precedence than && and ||, and we assume C programmers will know that.

(In reply to comment #14)
> Review of attachment 35769 [details] [review]:
> 
> ::: telepathy-glib/connection.c
> @@ +486,3 @@
> +      k,
> +      g_value_get_boolean (value->values + 1),
> +      g_value_get_boolean (value->values + 2));
> 
> You could use tp_value_array_unpack().
> 
> @@ +516,3 @@
> +  self->priv->allowed_presence_statuses = g_hash_table_new_full (g_str_hash,
> +      g_str_equal, g_free, (GDestroyNotify) tp_status_spec_destroy);
> +
> 
> Blank line after the if/unref block, please
> 
> @@ +521,3 @@
> +
> +  g_hash_table_foreach (statuses, allowed_presence_statuses_foreach,
> +                        self->priv->allowed_presence_statuses);
> 
> GHashTableIter is often more readable than g_hash_table_foreach?
> 
> @@ +537,3 @@
> +  if ((self->priv->allowed_presence_statuses != NULL) &&
> +        (self->priv->final_statuses))
> +    return;   /* already done */
> 
> This has more parentheses than it needs to. && has lower precedence than != and
> we assume C programmers know that.
> 
> @@ +551,3 @@
> +        (self->priv->final_statuses))
> +    return;   /* not interested right now */
> +
> 
> You don't need the parentheses here - && has low precedence.
> 
> @@ +2492,3 @@
> +
> +/**
> + * tp_status_spec_copy:
> 
> Should this be (skip)?

Library authors shouldn't need to worry about this, either g-i or the bindings should hide it from bindings users.

> @@ +2509,3 @@
> +                             self->status,
> +                             self->may_set_on_self,
> +                             self->can_have_message);
> 
> We don't indent multi-line arguments like that; see
> http://telepathy.freedesktop.org/wiki/Style
> 
> @@ +2513,3 @@
> +
> +/**
> + * tp_status_spec_destroy:
> 
> Should this be (skip)?

Same as with _copy()

> @@ +2544,3 @@
> + *
> + * Returns: (element-type utf8 TelepathyGLib.StatusSpec): Dictionary from
> + * string identifiers to structs for each valid status.
> 
> In telepathy-glib we usually make these function-based accessors not return a
> new ref, with a convention of using _dup_ in the name of an accessor that does
> return a copy.
> 
> Should this be made available as a GObject property too, with this function as
> a "C binding"?

Done, but then we depend on a new g-i release when this gets in:

https://bugzilla.gnome.org/show_bug.cgi?id=620484

> @@ +2615,3 @@
> +  else
> +    tp_cli_connection_interface_simple_presence_call_set_presence (self, -1,
> +        status, status_message, NULL, NULL, NULL, NULL);
> 
> The "if" block has {}, which means the "else" needs them too, even though it's
> only one line.
> 
> ::: telepathy-glib/connection.h
> @@ +105,3 @@
> +TpStatusSpec *tp_status_spec_new (TpConnectionPresenceType type,
> +    const gchar *status, gboolean may_set_on_self, gboolean can_have_message)
> +  G_GNUC_WARN_UNUSED_RESULT;
> 
> Does this need to be public API? If not, put it in connection-internal.h and
> prefix it with an underscore.
> 
> @@ +108,3 @@
> +
> +TpStatusSpec *tp_status_spec_copy (TpStatusSpec *self)
> +  G_GNUC_WARN_UNUSED_RESULT;
> 
> The argument should be const
> 
> ::: tests/lib/contacts-conn.c
> @@ +320,1 @@
>      return FALSE;
> 
> You don't need this many parentheses. == and != have higher precedence than &&
> and ||, and we assume C programmers will know that.

All comments should have been addressed, thanks!

Created attachment 36093 [details] [review]
Add tp_connection_get_allowed_presence_statuses and tp_connection_set_self_presence_async

Created attachment 36094 [details] [review]
Add tp_connection_get_allowed_presence_statuses and tp_connection_set_self_presence_async

Review of attachment 36094 [details] [review]:

::: docs/reference/telepathy-glib-sections.txt
@@ +2871,3 @@
+TP_TYPE_STATUS_SPEC
+TpStatusSpec
+tp_status_spec_new

You deleted tp_status_spec_new from the public header, so it shouldn't be documented.

::: telepathy-glib/connection.c
@@ +504,3 @@
+
+  value = g_hash_table_lookup (properties, "Statuses");
+  statuses = g_value_get_boxed (value);

If either value or statuses is NULL, this will critical. We shouldn't critical on malformed D-Bus messages, so there should be a check and a "goto finally".

In fact, this should be a single call to tp_asv_get_boxed (properties, THE_TYPE_OF_STATUSES); if the result is NULL, goto finally.

@@ +1385,3 @@
+   *
+   * Type: GLib.HashTable<utf8,StatusSpec>
+   * Transfer: container

Will this fail to build with g-i 0.6.14, or just not work? If it'll just not work, we can merge this branch without bumping the dependency, IMO.

Do you mean TelepathyGLib.StatusSpec, or will this "just work" as-is?

::: tests/dbus/self-presence.c
@@ +31,3 @@
+  tp_connection_set_self_presence_finish (client_conn, result, &error);
+
+  MYASSERT_SAME_STRING (g_quark_to_string (error->domain),

g_assert_cmpstr (., ==, .) in new tests, please.

This should really be g_assert_error (error, TP_ERRORS, TP_ERROR_NOT_AVAILABLE) or whatever it is.

@@ +130,3 @@
+
+  status_spec = g_hash_table_lookup (statuses, "available");
+  MYASSERT (status_spec != NULL, "");

Please use g_assert, g_assert_cmpuint, g_assert_cmpstr in new code. I haven't systematically replaced MYASSERT with g_assert yet, but only because some of the tests rely on MYASSERT always evaluating its argument.

Tomeu, are you still working on this? We should probably have this earlyish in 0.13; if you're not working on it yourself please drop it back to NEW state and someone else can steal it.

(In reply to comment #19)
> Tomeu, are you still working on this? We should probably have this earlyish in
> 0.13; if you're not working on it yourself please drop it back to NEW state and
> someone else can steal it.

Still think of it from time to time, today is Sugar code freeze so I should have more time soon. Of course, I'm fine if someone else wants to take it.

-- 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/17.