Bug 33546 - Refactore and cleanup TplEvent/TplEventText
Summary: Refactore and cleanup TplEvent/TplEventText
Status: RESOLVED FIXED
Alias: None
Product: Telepathy
Classification: Unclassified
Component: logger (show other bugs)
Version: git master
Hardware: Other All
: medium normal
Assignee: Nicolas Dufresne
QA Contact: Telepathy bugs list
URL:
Whiteboard: review+ (confirmed on IRC)
Keywords:
Depends on: 32902
Blocks: 27271 33993
  Show dependency treegraph
 
Reported: 2011-01-26 12:51 UTC by Nicolas Dufresne
Modified: 2011-02-08 07:46 UTC (History)
2 users (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-26 12:51:03 UTC
TplEvent and TplEventText need some cleanup and rework. Many of the private method are simply not useful, some function (is_room(), pending-msg-id, etc) are badly placed. Also code readability can be improved by using g_object_new instead of incomplete allocators and bunch of internal setters.
Comment 1 Nicolas Dufresne 2011-01-26 12:54:30 UTC
http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/event-refactoring

commit 0456844fe7b6ea9009d974d0e26bfeec710747c9
Author: Nicolas Dufresne <nicolas.dufresne@collabora.co.uk>
Date:   Wed Jan 26 15:37:28 2011 -0500

    Refactored TplEvent/TplEventText
    
    This is a refactoring/cleanup of TplEvent/TplEventText classes. Main
    changes are:
    
    - Removal of all the setters in -private.h files
    - Made properties construct only
    - Moved pending-msg-id to TplEvent
    - Made _get_message_type() public (bug #27621)
    - Moved/Renamed _text_is_charoom() to tpl_event_target_is_room()
    - Removed TplEventTextSignalType
    - Remove incomplete allocators (in favor of g_object_new())
    
    Note that while porting the log-store I accidently fixed a bug in Pidgin
    log-store where message body is always NULL, this is now reflected in
    the test.
Comment 2 Emilio Pozuelo Monfort 2011-02-06 17:05:13 UTC
Looks mostly good. Just two comments:

+static void
+received_data_free (gpointer arg)
+{
+  ReceivedData *data = arg;
+  g_free (data->log_id);
+  g_free (data->text);
+  g_slice_free (ReceivedData,data);
+}

Why gpointer arg and not ReceivedData *data ?

Please split this into smaller commits. At the very least, the pidgin bug fix should be a different one. And if you can split it even more (e.g. one commit for removing the setters, another one for making props construct-only...), even better.
Comment 3 Nicolas Dufresne 2011-02-07 03:26:33 UTC
(In reply to comment #2)
> Looks mostly good. Just two comments:
> 
> +static void
> +received_data_free (gpointer arg)
> +{
> +  ReceivedData *data = arg;
> +  g_free (data->log_id);
> +  g_free (data->text);
> +  g_slice_free (ReceivedData,data);
> +}
> 
> Why gpointer arg and not ReceivedData *data ?

This is implementing a GDestroyNotify function, so the gpointer matches the signature, otherwise I would have to cast the function pointer.

> 
> Please split this into smaller commits. At the very least, the pidgin bug fix
> should be a different one. And if you can split it even more (e.g. one commit
> for removing the setters, another one for making props construct-only...), even
> better.

I'll have a look, but will try no to spend to much time though.
Comment 4 Nicolas Dufresne 2011-02-07 09:28:44 UTC
Ok, I've split as much as I could and pushed the rework at http://git.collabora.co.uk/?p=user/nicolas/telepathy-logger.git;a=shortlog;h=refs/heads/event-refactoring
Comment 5 Emilio Pozuelo Monfort 2011-02-07 10:42:44 UTC
Looks good. I have a few comments though

> +/*
>   * tpl_event_text_get_pending_msg_id
> - * @self: a #TplEventText
> + * @self: a #TplEvent

It's still a TplEventText. Also add a '_' to the function name.

> -_tpl_event_text_get_message_type (TplEventText * self)
> +tpl_event_text_get_message_type (TplEventText * self)

Not your fault, but while you're at it, make that '*self' (without the space).


_tpl_event_text_equal() did more checks than what tpl_event_equal_default() does. Is that alright?

> +      const gchar *room_id = _tpl_channel_text_get_chatroom_id (tpl_text);
> +      receiver = _tpl_entity_new_from_room_id (room_id);
> +      target_id = room_id;

You can just do:

target_id = _tpl_channel_...
receiver = _tpl_entity_... (target_id);

> +  data = g_slice_new0(ReceivedData);

Add an space after _new0

> -  TplEventText *self = TPL_EVENT_TEXT (object);
> +  TplEventTextPriv *priv = TPL_EVENT_TEXT (object)->priv;

I'd rather you did self->priv->foo instead.

> +      "(maybe NULL with some log store)",

s/maybe/may be/
s/store/stores/

> +          /* FIXME: in text format it's not possible to guess who is the
> +           * reveiver (unless we are in a room). In this case the receiver will

receiver

> +        tp_proxy_get_object_path (TP_PROXY(account)),

missing space

> +  /* Determin if it's a chatroom */

determine

> +          log_id = _tpl_create_message_token (instead_of_channel_path,\

that backslash seems wrong

> -              regex = g_regex_new ("^\\((.+)\\) (.+):", 0, 0, NULL);
> +              regex = g_regex_new ("^\\((.+)\\) (.+): (.+)", 0, 0, NULL);

what's this about?

> -          if (hits == NULL || g_strv_length (hits) < 3)
> +          if (hits == NULL || g_strv_length (hits) < (4 + (guint)is_html))

Same here.

TBH I haven't closely looked at the pidgin log store changes...
Comment 6 Nicolas Dufresne 2011-02-08 06:25:27 UTC
(In reply to comment #5)
> Looks good. I have a few comments though
> 
> > +/*
> >   * tpl_event_text_get_pending_msg_id
> > - * @self: a #TplEventText
> > + * @self: a #TplEvent
> 
> It's still a TplEventText. Also add a '_' to the function name.

Fixed.

> 
> > -_tpl_event_text_get_message_type (TplEventText * self)
> > +tpl_event_text_get_message_type (TplEventText * self)
> 
> Not your fault, but while you're at it, make that '*self' (without the space).
Fixed.

> 
> 
> _tpl_event_text_equal() did more checks than what tpl_event_equal_default()
> does. Is that alright?

Actually no since the code you are referring was left commented. Also, code is smaller since I access log_id directly instead of using the getters.

> 
> > +      const gchar *room_id = _tpl_channel_text_get_chatroom_id (tpl_text);
> > +      receiver = _tpl_entity_new_from_room_id (room_id);
> > +      target_id = room_id;
> 
> You can just do:
> 
> target_id = _tpl_channel_...
> receiver = _tpl_entity_... (target_id);

Hmm, was pure weirdness, fixed. Also the target_id was a char instead of gchar, fixed too.

> 
> > +  data = g_slice_new0(ReceivedData);
> 
Fixed.

> 
> > -  TplEventText *self = TPL_EVENT_TEXT (object);
> > +  TplEventTextPriv *priv = TPL_EVENT_TEXT (object)->priv;
> 
> I'd rather you did self->priv->foo instead.

Ok, can't find this one, though the convention is that if you only use priv in your function you should extract it first. It's less typing, easier to read and reduce the number of dereferences (for cases compiler failed to optimise). I've revised the event-text.c file accordingly.

> 
> > +      "(maybe NULL with some log store)",
> 
> s/maybe/may be/
> s/store/stores/

Fixed.

> 
> > +          /* FIXME: in text format it's not possible to guess who is the
> > +           * reveiver (unless we are in a room). In this case the receiver will
> 
> receiver
> 
> > +        tp_proxy_get_object_path (TP_PROXY(account)),
> 
> missing space

Fixed

> 
> > +  /* Determin if it's a chatroom */
> 
> determine

Fixed.

> 
> > +          log_id = _tpl_create_message_token (instead_of_channel_path,\
> 
> that backslash seems wrong

Fixed.

> 
> > -              regex = g_regex_new ("^\\((.+)\\) (.+):", 0, 0, NULL);
> > +              regex = g_regex_new ("^\\((.+)\\) (.+): (.+)", 0, 0, NULL);
> 
> what's this about?

It add a third element to be saved by the regex, the message itself. Message was always NULL with text backend because of that.

> 
> > -          if (hits == NULL || g_strv_length (hits) < 3)
> > +          if (hits == NULL || g_strv_length (hits) < (4 + (guint)is_html))
> 
> Same here.

The check was weak, checking for three elements in the array. Actually this depends on if it's HTML or not, and there's one more element because of my previous change. So basically, check for 4, or 5 in the case of html. I must agree adding the boolean is a bit hugly, so I've made it more explicit.

         if (hits == NULL
              || (is_html && g_strv_length (hits) < 5)
              || (g_strv_length (hits) < 4))

> 
> TBH I haven't closely looked at the pidgin log store changes...

I'm the third dev on it, so I guess this also count for reviewing. I'd say good enough ...
Comment 7 Nicolas Dufresne 2011-02-08 07:46:04 UTC
Merged.


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.