Bug 41772

Ideally this API should allow us to get logs from meta-contacts rather than a single persona (see https://bugzilla.gnome.org/show_bug.cgi?id=653338 and https://bugzilla.gnome.org/show_bug.cgi?id=662983).

So either the logger should grow a dep on Folks and provide API to get logs from a FolksIndividual. Or it should provide API to get logs from a bunch of (TP account, contact ID) tuples and aggregate them together.

No, that's not possible to merge conversion from MSN and Google withing the same walker. The end-result would be a non-sense.

A review of the new API would be nice.

I'll have a look today, but it will be in the afternoon (EST).

I did an API only review.The log manager part seems fine to me, other method can be added later. For the TplLogWalker though I'd do some changes.

> void tpl_log_walker_get_filtered_events_async (TplLogWalker *walker,
>     guint num_events,
>     GAsyncReadyCallback callback,
>     gpointer user_data);
> 
> gboolean tpl_log_walker_get_filtered_events_finish (TplLogWalker *walker,
>     GAsyncResult *result,
>     GList **events,
>     GError **error);


In this one I'd clearly drop _filtered_, as the walker should not be that specific. Also the filter cannot be changed anyway. Wheter it's a search, a filtered search, or other type of data request does not matter here.

One of the thing that was important in the original design was the direction. This one I guess is forward, though I'd appreciate to see it in the method name. Going backward can be added later without break API.

> 
> gboolean tpl_log_walker_is_begin (TplLogWalker *walker);

This one make some sense, since we can't add event in the past. It might be worth looking around other API to see what would be the best naming. GList uses g_list_first() g_list_last() in a similar API, GHashTableIter does not provide such API.

> 
> gboolean tpl_log_walker_is_end (TplLogWalker *walker);

This one does not make as much sense, as new event could be added later. I think we should introduce an error type that says no more data, and the direction of the request would tell if it's first or last. We should also document that more data might be added later, there is a bug I think that suggest adding signals when new event are inserted.

(In reply to comment #5)
> I did an API only review.The log manager part seems fine to me, other method
> can be added later. For the TplLogWalker though I'd do some changes.

Thank you for the review!

I am, personally, a bit confused about what is forward and what is backward. eg., if we have 100 events, with 0 being the oldest and 99 being the most recent, then currently we return 95-99 in the first batch, 90-94 in the second batch and so on? Is this forward or backward?

(In reply to comment #6)
> (In reply to comment #5)
> > I did an API only review.The log manager part seems fine to me, other method
> > can be added later. For the TplLogWalker though I'd do some changes.
> 
> Thank you for the review!
> 
> I am, personally, a bit confused about what is forward and what is backward.
> eg., if we have 100 events, with 0 being the oldest and 99 being the most
> recent, then currently we return 95-99 in the first batch, 90-94 in the second
> batch and so on? Is this forward or backward?

Good point, as this return stuff from most recent, going forward would be getting older data. But that could be confusing.

The idea is that I don't want UI to keep all the data into RAM. So UIs need a way to forget some of the information, and request it later (in an efficient way).

(In reply to comment #7)
> The idea is that I don't want UI to keep all the data into RAM. So UIs need a
> way to forget some of the information, and request it later (in an efficient
> way).

Right.

I thought a bit on how this will be used in Empathy. As the user scrolls down, we can start forgetting some of the older logs, and then fetch them back as the user scrolls up.

For this to work, we need a way to tell TplLogWalker to rewind a few steps in the other direction. eg., if 0 is the oldest event, then ...
0 ---
1 ---
2 ---
3 ---
4 ---
5 ---
6 ---
7 ---
8 ---
9 ---
... at some point we might want to forget events 0 to 4, and tell the walker to rewind back to 4, so that the next time the users scrolls up we can again start fetching from 4. Question is, what should the API look like? Do we provide an offset or a message token? Or both?

Secondly, do we want to combine forward and backward iterating functionality into the same class(es)? Or do we want to keep them separate? C++ has them as separate types.

Maybe it's another crazy idea, but we could leave the caching to empathy by having bidirectional iterator. Empathy would create two iterator (tailIt, headIt). With headIt it would read a certain amount of information (the amount it wants to be cached. The iterator would have a method to go forward or backward without transfering the data. So if you want to go forward, you query n element from headIt, and advance tailIt of same amount of event.

This could also serve an internal design, as this might be too complicated. The Walker could be considered a cache, initialize with a cached size, and event list being accessible uppon request.

I definitely need to think about it, Guillaume, what did we concluded initially ?

I don't remember tbh.

 I don't think we'll want to remove older logs in Empathy's chat view, they will just stay there until the chat view is closed. I'm not sure it's worth for the walker too keep already returned events in memory. Why would the user app needs them? They are already displayed.

(In reply to comment #5)
> > void tpl_log_walker_get_filtered_events_async (TplLogWalker *walker,
> >     guint num_events,
> >     GAsyncReadyCallback callback,
> >     gpointer user_data);
> > 
> > gboolean tpl_log_walker_get_filtered_events_finish (TplLogWalker *walker,
> >     GAsyncResult *result,
> >     GList **events,
> >     GError **error);
> 
> 
> In this one I'd clearly drop _filtered_, as the walker should not be that
> specific. Also the filter cannot be changed anyway. Wheter it's a search, a
> filtered search, or other type of data request does not matter here.

Fixed. I have dropped "_filtered".

I have implemented a rewind method now:

void tpl_log_walker_rewind_async (TplLogWalker *walker,
    guint num_events,
    GAsyncReadyCallback callback,
    gpointer user_data);

gboolean tpl_log_walker_rewind_finish (TplLogWalker *walker,
    GAsyncResult *result,
    GError **error);

Code looks good. The only thing that is missing for this to be merged is unit tests. For the log manager part, we try and do a code coverage, and trigger some code related to multi-backend merging. And there is more serious tests for log store themeself. See tests/dbus folder.

(In reply to comment #13)
> Code looks good. The only thing that is missing for this to be merged is unit
> tests. For the log manager part, we try and do a code coverage, and trigger
> some code related to multi-backend merging. And there is more serious tests for
> log store themeself. See tests/dbus folder.

Could you please take a look again? I have added test cases for get_events. Now we need some for rewind.

> Could you please take a look again? I have added test cases for get_events. Now
> we need some for rewind.

Added test cases for rewind.

I have now changed the algorithm used by TplLogWalker so that multiple events with the same timestamp are handled better and the size of the event cache is constant.

Added some test cases for TplLogWalker's get_events and rewind API.

Good work. I have merged upstream this work. There is one last chance to influence this new API, look at this link: http://people.collabora.com/~nicolas/tp-logger/TplLogWalker.html

A release shall be made tomorrow, so it's really the last chance.

The API looks good to me but the method to create a new walker is missing from the doc.

(In reply to comment #18)
> The API looks good to me but the method to create a new walker is missing from
> the doc.

You can create a new walker from the manager using:

/**
 * tpl_log_manager_walk_filtered_events:
 * @manager: a #TplLogManager
 * @account: a #TpAccount
 * @target: a non-NULL #TplEntity
 * @type_mask: event type filter see #TplEventTypeMask
 * @filter: (scope call) (allow-none): an optional filter function
 * @filter_data: user data to pass to @filter
 *
 * Create a #TplLogWalker to traverse all the events exchanged with @target.

 * Returns: (transfer full): a #TplLogWalker
 */
TplLogWalker *
tpl_log_manager_walk_filtered_events (TplLogManager *manager,
    TpAccount *account,
    TplEntity *target,
    gint type_mask,
    TplLogEventFilter filter,
    gpointer filter_data)

Ah ok :) Maybe add some words in the walker doc explaining this then?

But yeah, the API looks all good to me!

(In reply to comment #20)
> Ah ok :) Maybe add some words in the walker doc explaining this then?

Yeah, an example at the top of this page would be cool.

There were some more discussions with people at GUADEC about this kind of thing but I can't remember what we said, so, great... Regardless, I think this API is fine and can be extended easily. Cool!

A couple of comments:

> gboolean            tpl_log_walker_is_begin             (TplLogWalker *walker);

_is_begin doesn't sound English to me? I'd say _is_beginning or _is_start (better).

In fact, I'm really confused about the use of this API. It talks about sequentially looking over logs, but then it can only be rewound, which feels weird. Perhaps some function renaming would be ideal, but more explanation will be thoroughly helpfully. I'd be happy with just some more docs tbh.

Nice work! I'm looking forward to making use of it!

(In reply to comment #21)
> > gboolean            tpl_log_walker_is_begin             (TplLogWalker *walker);
> 
> _is_begin doesn't sound English to me? I'd say _is_beginning or _is_start
> (better).
> 

yeah, I had the same thought, I'd be happy with _is_start() (the shortest).

(In reply to comment #21)
> Yeah, an example at the top of this page would be cool.

Defiantly, that would solve most of the confusion.

(In reply to comment #23)
> (In reply to comment #21)
> > Yeah, an example at the top of this page would be cool.
> 
> Defiantly, that would solve most of the confusion.

oops, I meant definitely.

Created attachment 66291 [details] [review]
log-walker: Add an example explaining how to use the API

(In reply to comment #25)
> Created attachment 66291 [details] [review] [review]
> log-walker: Add an example explaining how to use the API

Merged.