[Bug 35886] Add support for text edits

[bugzilla-daemon at freedesktop.org]

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

--- Comment #4 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2011-05-19 13:07:31 PDT ---
Patch "TplTextEvent: add {message,supersedes}-token properties":

> +tpl_text_event_dispose (GObject *obj)
> +{
> +   TplTextEventPriv *priv = TPL_TEXT_EVENT (obj)->priv;
> +
> +   if (priv->dispose_has_run)
> +     return;
> +   priv->dispose_has_run = TRUE;
> +
> +   g_list_free_full (priv->supersedes.head, g_object_unref);
> +}

g_list_free_full() is glib 2.28 while the configure.ac says we need 0.2.25.11.
We usually to a g_list_foreach(list, unref), g_list_free() so we don't have to
bump to very recent glib version.

Also,I strongly prefer if you clear the queue structure, and not keep random
pointers around then using this boolean. Also, as NULL is a correct empty list,
you don't have to do any NULL check.


Patch: "tpl_text_event_{add,dup}_supersedes":

add_supersedes() writes into a TplTextEvent, this is not allowed in the public
interface, move this function to text-event-private.h and prepend a _.

> + for (l = old_event->priv->supersedes.head; l != NULL; l = l->next)
> +   g_queue_push_tail (&self->priv->supersedes, g_object_ref (l->data));

Use g_list_next() instead of "l = l->next" for readability, don't worry it's a
macro, not a function call.

> + for (l = self->priv->supersedes.tail; l != NULL; l = l->prev)
> +   supersedes = g_list_prepend (supersedes, g_object_ref (l->data));

Use g_list_previous().


Patch "Store, save and test message-token and supersedes-token":

> + if (token_str != NULL)
> + {

You might want to use TPL_STR_EMPTY() instead.

> + token_str = tpl_text_event_get_supersedes_token (message);
> + if (token_str != NULL)
> + {

We should be a little more robust, and ignore supersedes if there is no token.


Patch "log_store_xml_get_events_for_file: replace edited messages.":

+ if (l != NULL)
+ {
+   tpl_text_event_add_supersedes (event, l->data);
+   g_object_unref (l->data);
+   l->data = event;
+   g_hash_table_insert (superseded_links, (gpointer) supersedes_token, l);
+   return index;
+ }

I think you don't have to call g_hash_table_insert() here ? And same in next
for loop.

> + for (l = index; l != NULL; l = l->prev)
> + {

Use g_list_previous().

> + "Better <s>drink my own piss</s> fake it.",

Is that suppose to be funny ?

> + tpl_text_event_add_supersedes (event, dummy_event);
> + index = event_queue_insert_sorted_after (events, index,
> + TPL_EVENT (event));
> + g_hash_table_insert (superseded_links, (gpointer) supersedes_token,
> + index);
> + return index;

Your leaking a reference on the dumm_event. Maybe you want to rework you
_add_supersedes() method into something like link_supersedes() that would take
ownership ?

> -
> - if (event != NULL)
> {

You don't check anymore if parsing event worked. A corrupted XML file will
cause your code to crash.


Patch "Store events in the correct file according to their timestamp":

> + date = g_date_time_new_from_unix_local (timestamp);

Timestamp in TplEvent are always UTC.


Pathc: "Add tests to cover messages arriving a bit/a lot late":

> - g_object_unref (events->data);
> - g_list_free (events);
> + g_list_free_full (events, g_object_unref);

Don't do that until we have a really good reason to update to 2.28. Also wrong
cause you did not update configure.ac.

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