[Bug 33546] Refactore and cleanup TplEvent/TplEventText

[bugzilla-daemon at freedesktop.org]

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

--- Comment #6 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2011-02-08 06:25:27 PST ---
(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 ...

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