[Bug 27270] TplLogManager: clean up API

[bugzilla-daemon at freedesktop.org]

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

--- Comment #17 from Emilio Pozuelo Monfort <pochu27 at gmail.com> 2011-01-10 02:37:33 PST ---
(In reply to comment #16)
> >-gboolean tpl_log_manager_get_filtered_messages_finish (TplLogManager *self,
> >+gboolean tpl_log_manager_get_filtered_events_finish (TplLogManager *self,
> >     GAsyncResult *result,
> >     GList **messages,
> >     GError **error);
> 
> Need to change GList **messages into GList **events

Done, and a bunch of other s/message/event/ that made sense.

> >+ * Checks if @id exists for @account and is of type @type.
> 
> In this case can we have multiple types? Does it mean it exists for all type,
> or one of ?

I don't think so, since type there is associated to id. It's really a pair {id,
type}. I could improve that documentation to e.g. "Checks if @id of type @type
exists for @account".

> >@@ -520,18 +515,18 @@ log_manager_message_date_cmp (gconstpointer a,
> >   one_time = tpl_entry_get_timestamp (one);
> >   two_time = tpl_entry_get_timestamp (two);
> > 
> >-  /* return -1, o or 1 depending on message1 is newer, the same or older than
> >+  /* return -1, 0 or 1 depending on message1 is newer, the same or older than
> >    * message2 */
> is newer -> being newer? and message -> event?

Yes, fixed.

> 
> > gint
> > _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
> >     TplLogSearchHit *b)
> 
> There is something weird about this function now, and removing the
> documentation does not help much.

Well I didn't remove it, I just changed it to document what it was doing after
my changes

> 
> >+      if (a->type == b->type)
> >+        return 0;
> >     }
> 
> This function seem to be used to sort the result, so it must have a finit order
> (whatever the order is). Maybe you should do that instead:

Not really. It's only used to compare (as in == , !=) to know whether to
include an event in a list or not (because it's already there), so order
doesn't matter. I've changed it to do that again though, since it doesn't cost
us much

>   if (a->type < b->type)
>     return -1;
>   else if (a->type > b->type)
>    return 1;
>   else
>     return strcmp(a->id, b->id);
> 
> Take note that I don't waste time doing strcmp() if type mismatch.

Makes sense, changed accordingly.

> >diff --git a/extensions/Logger.xml b/extensions/Logger.xml
> >@@ -82,6 +82,10 @@ Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.</
> >+      <!-- FIXME: we're returning a list of events that can be
> >+           messages (chats), but also calls and any other event
> >+           type in the future, so make the return type a list of
> >+           TplLogSearchHit or something generic than a(ssx) ? -->
> >       <arg direction="out" name="Messages" type="a(ssx)"
> >            tp:type="Chat_Message[]" />
> 
> I think we need aa(sv). Some field will be shared between events and other
> specific (which will have to be documented in the protocol). But that can be
> kept for when we move from TplSearchHit to TplEvent (TplEvent will simply have
> to know how to serialise/deserialise itself into a(sv).

Why aa(sv) ? How do we know that will be enough for future event types?

> >diff --git a/telepathy-logger/log-store-internal.h b/telepathy-logger/log-store-internal.h
> >@@ -63,21 +63,22 @@ typedef struct
> >   gboolean (*add_message) (TplLogStore *self, TplEntry *message,
> >       GError **error);
> 
> Is this suppose to be add_event now ?

Yes, fixed.

> >diff --git a/telepathy-logger/log-store-xml.c b/telepathy-logger/log-store-xml.c
> >@@ -1042,6 +1043,7 @@ log_store_xml_search_in_identifier_chats_new (TplLogStore *store,
> >   g_return_val_if_fail (!TPL_STR_EMPTY (text), NULL);
> > 
> >   account_dir = log_store_account_to_dirname (account);
> >+  /* FIXME: take @type into account for the dir. */
> 
> Is it something you forgot, or it's not needed for now ?

I fixed it, but it's in the next commit (in the sqlite log store commit) by
mistake... :(

I can squash that part of the commit into the xml log store one where it should
be.

> >@@ -1120,35 +1122,35 @@ log_store_xml_get_chats_for_dir (TplLogStoreXml *self,
> >-/* returns a Glist of TplEntryText instances */
> >+/* returns a Glist of FIXME TplEntryText instances */
> 
> As this one used TplEntryText (sub-class of TplEntry) it should be very easy to
> fix, s/TplEntryText/TplEntry/ ;). Unless you wanted this note for
> implementation of Call support ?

It needs more changes, so it doesn't make a lot of sense to just do that... we
also need to change the parsing in log_store_xml_get_messages_for_file()...
although since there's nothing else to parse right now, I can probably change
that. Done :)

Branch updated.

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