[Bug 35886] Add support for text edits

[bugzilla-daemon at freedesktop.org]

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

David Laban <david.laban at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|review-                     |
           Keywords|                            |patch

--- Comment #5 from David Laban <david.laban at collabora.co.uk> 2011-05-19 16:51:46 PDT ---
(In reply to comment #4)
> 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().
> 
> 
addressed in "fixup! tpl_text_event_{add,dup}_supersedes"

Also, I realised that my annotation (transfer full) was incorrect:
Since I ref the object that is passed in, I think (transfer none) is the
correct annotation (since I create a new ref rather than stealing it off
the caller). Correct me if I'm wrong again.

> 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.
> 
> 
I assume that by this, you mean only record it if message-token also exists?
If so, "fixup! Store, save and test message-token and supersedes-token"
should be what you are looking for.

> 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.
> 
Actually, this is for the chained supersedes use-case, but yeah: it's wrong.
Fixed it, and fixed the comment on superseded_links

> > + 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 ?
(apparently it wasn't funny; fixed)
> 
> > + 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 ?
> 
Created method event_queue_replace_and_supersede with similar feel
to event_queue_insert_sorted_after.
This is something that I wanted to do, but couldn't see a clean way to do.
Fixing the semantics of superseded_links made this easier.

> > -
> > - if (event != NULL)
> > {
> 
> You don't check anymore if parsing event worked. A corrupted XML file will
> cause your code to crash.
> 
*_parse_*() will currently only return NULL if g_object_new() does, but you're
right: if someone changes this behaviour later, I should be resilient to it.

^^ addresssed in "fixup! log_store_xml_get_events_for_file: replace edited
messages."

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

fixed in "fixup! Store events in the correct file according to their timestamp"
> 
> 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.

fixed in "fixup! Test message edits that have broken timestamps" and
"fixup! Add tests to cover messages arriving a bit/a lot late"

http://cgit.freedesktop.org/~alsuren/telepathy-logger/log/?h=edits-35886 is the
branch you want.

http://cgit.freedesktop.org/~alsuren/telepathy-logger/log/?h=edits-35886-rebased
is the result of git rebase --autosquash origin/master

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