Bug 33252 - Refactoring of TplEntity
Summary: Refactoring of TplEntity
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: logger (show other bugs)
Version: git master
Hardware: Other All
: medium enhancement
Assignee: Telepathy bugs list
QA Contact: Telepathy bugs list
URL: http://git.collabora.co.uk/?p=user/ni...
Whiteboard: review+
Keywords:
Depends on: 33213
Blocks: 27271
  Show dependency treegraph
 
Reported: 2011-01-18 11:45 UTC by Nicolas Dufresne
Modified: 2011-01-20 09:30 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Dufresne 2011-01-18 11:45:57 UTC
TplEntity can be improved in multiple ways:
1. The entity type (contact, self, room, etc) should be a property
2. Property should not be writable (should be construction only)
3. Constructor helper should be called _new_from_
4. Is not covered by the unit test.
Comment 1 Nicolas Dufresne 2011-01-18 11:47:46 UTC
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
Comment 2 Emilio Pozuelo Monfort 2011-01-19 09:47:46 UTC
+++ 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!
Comment 3 Nicolas Dufresne 2011-01-19 10:07:52 UTC
(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!
Comment 4 Nicolas Dufresne 2011-01-19 10:10:54 UTC
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.
Comment 5 Nicolas Dufresne 2011-01-19 10:13:36 UTC
(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.
Comment 6 Emilio Pozuelo Monfort 2011-01-19 10:59:17 UTC
Looks good.
Comment 7 Nicolas Dufresne 2011-01-20 09:30:05 UTC
Pushed


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.