Bug 33546

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.

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.

(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.

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

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

(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 ...

Merged.