Bug 54198

This is the whole point in having supersedes account spec.

>-  const char *account_name = get_account_name (account);
>+  const gchar *account_name = account_path +
>+      strlen (TP_ACCOUNT_OBJECT_PATH_BASE);

Please add an assert to check that account_path is long enough. (multiple places)

>log_store_xml_exists (TplLogStore *store,

Ishh, that synchronous function becomes worst then ever, hopefully it won't heart to much the UI performance.

>+          if (g_list_find_custom (ret, new->data,
>+                (GCompareFunc) g_date_compare))
>+            g_date_free (new->data);
>+          else
>+            ret = g_list_insert_sorted (ret, new->data,
>+                  (GCompareFunc) g_date_compare);

We have had serious performance issues in the past. In this case there is faster solution, like inserting everything once, and removing the duplicates in 1 pass. Second option is to implement an _insert_sorted_and_unique() method in util.c, so it can be reused all over. This one is probably the optimal solution.

>+          ret = g_list_insert_sorted (ret, new->data,
>+                (GCompareFunc) _tpl_event_timestamp_compare);

Use of insert sorted with events is wrong as it reverts the order of event with equal timestamps. See util.h for an insert_after, you'll have to use queues.

Finally, this won't go through without rigorous unit tests. Thinking of it, I'm not sure that superseded account API really works. It seems you can break it by creating a second time the same account and removing the old one. The resulting supersedes will not match.

(In reply to comment #2)
> >-  const char *account_name = get_account_name (account);
> >+  const gchar *account_name = account_path +
> >+      strlen (TP_ACCOUNT_OBJECT_PATH_BASE);
> 
> Please add an assert to check that account_path is long enough. (multiple
> places)

Done.

> >log_store_xml_exists (TplLogStore *store,
> 
> Ishh, that synchronous function becomes worst then ever, hopefully it won't
> heart to much the UI performance.
> 
> >+          if (g_list_find_custom (ret, new->data,
> >+                (GCompareFunc) g_date_compare))
> >+            g_date_free (new->data);
> >+          else
> >+            ret = g_list_insert_sorted (ret, new->data,
> >+                  (GCompareFunc) g_date_compare);
> 
> We have had serious performance issues in the past. In this case there is
> faster solution, like inserting everything once, and removing the duplicates in
> 1 pass. Second option is to implement an _insert_sorted_and_unique() method in
> util.c, so it can be reused all over. This one is probably the optimal
> solution.

It should be using GSequence, really.

> >+          ret = g_list_insert_sorted (ret, new->data,
> >+                (GCompareFunc) _tpl_event_timestamp_compare);
> 
> Use of insert sorted with events is wrong as it reverts the order of event with
> equal timestamps. See util.h for an insert_after, you'll have to use queues.

It would be hard to have equals, since msg are coming from different accounts that never existed at the same time.

Note that in _tpl_log_manager_get_events_for_date() it does not even sort the result :/

> Finally, this won't go through without rigorous unit tests. Thinking of it, I'm
> not sure that superseded account API really works. It seems you can break it by
> creating a second time the same account and removing the old one. The resulting
> supersedes will not match.

supersedes is used for account migration, for example when deleting a butterfly account and create a new haze account. It could be that the new account won't reuse the old account's path. That's why in that case we set superseded to tell logger we still want to use old account's logs.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/telepathy/telepathy-logger/issues/27.