[Bug 24107] support Statuses on TpConnection

[bugzilla-daemon at freedesktop.org]

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

Tomeu Vizoso <tomeu at sugarlabs.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|review-                     |

--- Comment #15 from Tomeu Vizoso <tomeu at sugarlabs.org> 2010-06-07 02:50:00 PDT ---
(In reply to comment #14)
> Review of attachment 35769 [details]:
> 
> ::: 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!

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