Bug 35886

After having a ref-counting crisis and talking with wjt for a while, I think I have a minimal API:

supported use case is:
monday: message a
tuesday: message b supersedes a
wednesday: message c supersedes b

LogStore.get_events(monday) -> [c, b, a] (even though c happened on Wed) a naive log viewer implementation should not lose the information that a was edited.

c.dup_supersedes() -> [b, a]

a.dup_superseded_by() is not possible to implement without getting into horrible refcounting issues, so I'm not going to implement it. If it is needed later then LogStore.dup_superseded_by(a) is possible. Making get_events(monday) also return edits means that this shouldn't be a problem.

The following representation is less trivial to implement (will file a bug and do it later) but is representable by the api.
message a
message b supersedes a
message c supersedes a

c.dup_supersedes() -> [a, b]

Progress can be found at:
http://cgit.freedesktop.org/~alsuren/telepathy-logger/log/?h=edits

Currently, It doesn't properly link events that happen on different days (requires some refactoring to make it easier to store events in random bullshit places and deciding how to write a test for it), but it should be usable as a base for CM/UI writers to test against if you avoid that bug.

(In reply to comment #1)
> After having a ref-counting crisis and talking with wjt for a while, I think I
> have a minimal API:
> 
> supported use case is:
> monday: message a
> tuesday: message b supersedes a
> wednesday: message c supersedes b
> 
> LogStore.get_events(monday) -> [c, b, a] (even though c happened on Wed) a
> naive log viewer implementation should not lose the information that a was
> edited.
-- Actually, sjoerd suggested that LogStore.get_events(monday) should only return
c, and dup_supersedes() can be used to get more information. This means that a naive log viewer presents the user with messages as they are semantically intended.

> The following representation is less trivial to implement (will file a bug and
> do it later) but is representable by the api.
> message a
> message b supersedes a
> message c supersedes a
> 
> c.dup_supersedes() -> [a, b]
-- Actually, I just fixed that.

> Currently, It doesn't properly link events that happen on different days
> (requires some refactoring to make it easier to store events in random bullshit
> places and deciding how to write a test for it), but it should be usable as a
> base for CM/UI writers to test against if you avoid that bug.
-- This is still outstanding.

Please review: http://cgit.freedesktop.org/~alsuren/telepathy-logger/log/?h=edits-35886. I will fix the editing events from yesterday issue as a separate issue.

I just added a few commits to my branch to deal with (and test) getting events from different days.

Note that there are now two functions to get the filename (one that takes a GDate and one a guint64). Merging the two should probably wait until we switch everything to GDateTime, because GDate is a pain in the ass to work with)

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.

(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

Feh. That'll teach me to run make check before pushing

http://cgit.freedesktop.org/~alsuren/telepathy-logger/log/?h=edits-35886 contains my updated fixes

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

Merged yesterday.