[Bug 27270] TplLogManager: clean up API

[bugzilla-daemon at freedesktop.org]

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

Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Status Whiteboard|                            |review+ (with trivial
                   |                            |fixes)

--- Comment #18 from Nicolas Dufresne <nicolas.dufresne at collabora.co.uk> 2011-01-10 08:12:34 PST ---
(In reply to comment #17)
> (In reply to comment #16)
> > >+ * 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".
Fair, small doc like this would be nice.

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

a(sv) is the most generic type I know in DBus, we also have bunch of tp_asv_*
helpers in telepathy-glib for those. You can pretty much represent anything
into that, but that's for future anyway. Sorry for the noise in this review.


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

I think it would be better if it's not too hard to do.

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

Nod. So after those two small changes it's good to go.

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