Bug 33252

Here's my refactoring branch, it's still missing a unit-test for _tpl_entity_new_from_tp_contact() since I didn't know how to craft a TpContact.

http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/entity-refactoring

+++ b/telepathy-logger/channel-text.c
+    _tpl_event_set_id (log, _tpl_channel_text_get_chatroom_id ( tpl_text));

Remove whitespace after parenthesis.

--- a/telepathy-logger/entity.c
+++ b/telepathy-logger/entity.c

+      case PROP_TYPE:
+        priv->type = g_value_get_int (value);

Shouldn't it be g_value_get_uint (since it's an enum) ?

-      G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
+      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);

Use G_PARAM_CONSTRUCT_ONLY.

-        _tpl_entity_set_alias (self, g_value_get_string (value));
+        g_free (priv->alias);
+        priv->alias = g_value_dup_string (value);

Since they are construct-only, instead g_free(), do
g_assert (priv->alias == NULL); /* construct-only */
Same for the other construct-only properties.


+  param_spec = g_param_spec_int ("type",
+      "Type",
+      "The entity's type",
+      TPL_ENTITY_UNKNOWN,
+      TPL_ENTITY_SELF,
+      TPL_ENTITY_UNKNOWN,
+      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);

Not sure if you should set the maximum to TPL_ENTITY_SELF, since if at some point we add a new type, we'll probably forget to update this. Also I've usually seen G_MAXINT there.

Looks quite good otherwise!

(In reply to comment #2)
> +++ b/telepathy-logger/channel-text.c
> +    _tpl_event_set_id (log, _tpl_channel_text_get_chatroom_id ( tpl_text));
> 
> Remove whitespace after parenthesis.

Done.

> 
> --- a/telepathy-logger/entity.c
> +++ b/telepathy-logger/entity.c
> 
> +      case PROP_TYPE:
> +        priv->type = g_value_get_int (value);
> 
> Shouldn't it be g_value_get_uint (since it's an enum) ?

No because enums are int in C.

> 
> -      G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS);
> +      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);
> 
> Use G_PARAM_CONSTRUCT_ONLY.

That was my intent, Done.

> 
> -        _tpl_entity_set_alias (self, g_value_get_string (value));
> +        g_free (priv->alias);
> +        priv->alias = g_value_dup_string (value);
> 
> Since they are construct-only, instead g_free(), do
> g_assert (priv->alias == NULL); /* construct-only */
> Same for the other construct-only properties.

Done.

> 
> 
> +  param_spec = g_param_spec_int ("type",
> +      "Type",
> +      "The entity's type",
> +      TPL_ENTITY_UNKNOWN,
> +      TPL_ENTITY_SELF,
> +      TPL_ENTITY_UNKNOWN,
> +      G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS);
> 
> Not sure if you should set the maximum to TPL_ENTITY_SELF, since if at some
> point we add a new type, we'll probably forget to update this. Also I've
> usually seen G_MAXINT there.

Yeah I know, I'm being laze because the param_spec_enum() exist bug requires a bit a work to get the stuff generated. I'll ask others about that.

> 
> Looks quite good otherwise!

So I updated the branch, it now contains the part you reviewed plus I implemented the test for _tpl_entity_new_from_tp_account(). Note I had to add a missing sentinel in the g_object_new() call in this function but it's already squashed it (as I'll do with the fixup when review is done).

The stuff from tests/lib is all from tp-glib, I did no changes at all there.

(In reply to comment #4)
> So I updated the branch, it now contains the part you reviewed plus I
> implemented the test for _tpl_entity_new_from_tp_account(). Note I had to add a
> missing sentinel in the g_object_new() call in this function but it's already
> squashed it (as I'll do with the fixup when review is done).
> 
> The stuff from tests/lib is all from tp-glib, I did no changes at all there.

I mean _tpl_entity_new_from_tp_contact(), not _account(), sorry for that.

Looks good.

Pushed