Bug 33213

+/* This macro is used to check if a list has been taken by an _finish()
+ * function call. It detects the the marker set by _take_list() method. Those
+ * are use to avoid copying the full list on every calls. */

an _finish -> a _finish
the the -> the
are use -> are used
every calls -> every call

-      _search_in_identifier_async_result_free);
+      (GDestroyNotify) tpl_log_manager_search_free);

-      _search_new_async_result_free);
+      (GDestroyNotify) tpl_log_manager_search_free);

I don't think you need the cast here.

Please split this commit in two, one for the free funcs and another for the _take_list.

For the second commit:

The _get_events() method was miss-leading. This function would return
partially filled TplLogSearchHit that would in fact represent entities.
This patch rename the function an return GList of TplEntity.

miss-leading -> misleading
rename -> renames
an return -> and returns a


-GList * _tpl_log_manager_get_events (TplLogManager *manager,
+GList * _tpl_log_manager_get_entities (TplLogManager *manager,

OK, that's fine as that's what that function is doing anyway. We may want to write a tpl_log_manager_get_events() function that actually returns a list of TplEvents (that would be needed for my new Empathy viewer proposal for example).


-gint _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
-    TplLogSearchHit *b);
+/*gint _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
+    TplLogSearchHit *b);*/

Just remove it.


+ * _tpl_entity_compare:
+ * @a: a #TplEntity
+ * @b: a #TplEntity
+_tpl_entity_compare (TplEntity *e1,
+    TplEntity *e2)

Either rename e1/e2 or @a/@b. Also make it consistent with the header if you decide to make it a/b.

-  g_return_val_if_fail (a != NULL && a->id != NULL, 1);
-  g_return_val_if_fail (b != NULL && b->id != NULL, 1);

Why remove the != NULL check? If we keep them they should become TPL_IS_ENTITY though.


- * Returns: a list of pointer to TplLogSearchHit, having id and
- * type fields filled. The result needs to be freed after use, see
- * _tpl_log_manager_search_hit_free()
+ * Returns: a list of pointer to #TplEntity, The result needs to be freed after
+ * use.

Do the TplEntities need to be unreffed? Please mention that there too.

+ * tpl_log_manager_get_entities_finish:

+ * @chats: a pointer to a #GList used to return the list of #TplEntity
     GList **events,

events should be renamed to entities I guess. And @chats too.


-gboolean tpl_log_manager_get_events_finish (TplLogManager *self,
+gboolean tpl_log_manager_get_entities_finish (TplLogManager *self,
     GAsyncResult *result,
     GList **events,

Likewise.


+      /* FIXME Facking alias with identifier */

Faking I think.

(In reply to comment #1)
> +/* This macro is used to check if a list has been taken by an _finish()
> + * function call. It detects the the marker set by _take_list() method. Those
> + * are use to avoid copying the full list on every calls. */
> 
> an _finish -> a _finish
> the the -> the
> are use -> are used
> every calls -> every call

Done.

> 
> -      _search_in_identifier_async_result_free);
> +      (GDestroyNotify) tpl_log_manager_search_free);
> 
> -      _search_new_async_result_free);
> +      (GDestroyNotify) tpl_log_manager_search_free);
> 
> I don't think you need the cast here.

The arg1 type is not a void*, thus GCC complains, that's why it's required.

> 
> Please split this commit in two, one for the free funcs and another for the
> _take_list.

Ok, I've merged them because I originally wrote them in reverse order, which I found confusing.

Done.

> 
> For the second commit:
> 
> The _get_events() method was miss-leading. This function would return
> partially filled TplLogSearchHit that would in fact represent entities.
> This patch rename the function an return GList of TplEntity.
> 
> miss-leading -> misleading
> rename -> renames
> an return -> and returns a
> 
> 
> -GList * _tpl_log_manager_get_events (TplLogManager *manager,
> +GList * _tpl_log_manager_get_entities (TplLogManager *manager,
> 
> OK, that's fine as that's what that function is doing anyway. We may want to
> write a tpl_log_manager_get_events() function that actually returns a list of
> TplEvents (that would be needed for my new Empathy viewer proposal for
> example).

Adding functions is not an issue for ABI. Note that getting all events for an account is a bad idea. If they want to do like the N900 we will implement a two way iterator or something similar. Do you have a draft for this proposal ?

> 
> 
> -gint _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
> -    TplLogSearchHit *b);
> +/*gint _tpl_log_manager_search_hit_compare (TplLogSearchHit *a,
> +    TplLogSearchHit *b);*/
> 
> Just remove it.

Oops, left over.

> 
> 
> + * _tpl_entity_compare:
> + * @a: a #TplEntity
> + * @b: a #TplEntity
> +_tpl_entity_compare (TplEntity *e1,
> +    TplEntity *e2)
> 
> Either rename e1/e2 or @a/@b. Also make it consistent with the header if you
> decide to make it a/b.

Done, used a/b.

> 
> -  g_return_val_if_fail (a != NULL && a->id != NULL, 1);
> -  g_return_val_if_fail (b != NULL && b->id != NULL, 1);
> 
> Why remove the != NULL check? If we keep them they should become TPL_IS_ENTITY
> though.

Added with TPL_IS_ENTITY(). Note that I did something tricky to guaranty finite order:
  g_return_val_if_fail (TPL_IS_ENTITY (a), TPL_IS_ENTITY (b) ? -1 : 0);
  g_return_val_if_fail (TPL_IS_ENTITY (b), 1);

> 
> 
> - * Returns: a list of pointer to TplLogSearchHit, having id and
> - * type fields filled. The result needs to be freed after use, see
> - * _tpl_log_manager_search_hit_free()
> + * Returns: a list of pointer to #TplEntity, The result needs to be freed
> after
> + * use.
> 
> Do the TplEntities need to be unreffed? Please mention that there too.

Yes, fixed.

> 
> + * tpl_log_manager_get_entities_finish:
> 
> + * @chats: a pointer to a #GList used to return the list of #TplEntity
>      GList **events,
> 
> events should be renamed to entities I guess. And @chats too.
> 
> 
> -gboolean tpl_log_manager_get_events_finish (TplLogManager *self,
> +gboolean tpl_log_manager_get_entities_finish (TplLogManager *self,
>      GAsyncResult *result,
>      GList **events,
> 
> Likewise.

Done.

> 
> 
> +      /* FIXME Facking alias with identifier */
> 
> Faking I think.

Done.

Branch updated too.

(In reply to comment #2)
> > -      _search_new_async_result_free);
> > +      (GDestroyNotify) tpl_log_manager_search_free);
> > 
> > I don't think you need the cast here.
> 
> The arg1 type is not a void*, thus GCC complains, that's why it's required.

GDestroyNotify has a gpointer arg, and tpl_log_manager_search_free has the same too. Both return void. I don't understand why it is needed... Plus I think in another place you don't cast it.

Good to go otherwise.

(In reply to comment #2)
> > -GList * _tpl_log_manager_get_events (TplLogManager *manager,
> > +GList * _tpl_log_manager_get_entities (TplLogManager *manager,
> > 
> > OK, that's fine as that's what that function is doing anyway. We may want to
> > write a tpl_log_manager_get_events() function that actually returns a list of
> > TplEvents (that would be needed for my new Empathy viewer proposal for
> > example).
> 
> Adding functions is not an issue for ABI.

I know

> Note that getting all events for an
> account is a bad idea. If they want to do like the N900 we will implement a two
> way iterator or something similar. Do you have a draft for this proposal ?

See https://bugzilla.gnome.org/show_bug.cgi?id=619866

Why is it a bad idea? It may be too much? If so it would be fine to just get the last N events, with a possibility to get more and more in subsequent queries. Also having everything in sqlite would make this less resource-hungry.

(In reply to comment #3)
> (In reply to comment #2)
> > > -      _search_new_async_result_free);
> > > +      (GDestroyNotify) tpl_log_manager_search_free);
> > > 
> > > I don't think you need the cast here.
> > 
> > The arg1 type is not a void*, thus GCC complains, that's why it's required.
> 
> GDestroyNotify has a gpointer arg, and tpl_log_manager_search_free has the same
> too. Both return void. I don't understand why it is needed... Plus I think in
> another place you don't cast it.
> 
> Good to go otherwise.

void tpl_log_manager_search_free (GList *hits);
void (*GDestroyNotify)           (gpointer data);

As you can see, one has GList* and the other gpointer, this is enough to generate compilation warning. Thanks for the review.

(In reply to comment #4)
> > Note that getting all events for an
> > account is a bad idea. If they want to do like the N900 we will implement a two
> > way iterator or something similar. Do you have a draft for this proposal ?
> 
> See https://bugzilla.gnome.org/show_bug.cgi?id=619866
> 
> Why is it a bad idea? It may be too much? If so it would be fine to just get
> the last N events, with a possibility to get more and more in subsequent
> queries. Also having everything in sqlite would make this less resource-hungry.

Thanks for the link, I'll have a close look. Having a get_events() function that return all events for an account will result in degrading performance over years. While the data itself is probably not so big, it will consume a lot of memory when put inside the widget. Progressive loading (probably based on get next N, get prev N) would ensure that performance will not degrade and reduce memory footprint. Let's discuss this somewhere else though.

Pushed.

Also resolved ...