Code review comment for lp:~aauzi/midori/fix-1179624

Revision history for this message
André Auzi (aauzi) wrote :

> 385 + else if (!child_bookmarks_count && child_folders_count)
> 386 + text = g_strdup_printf(
> 387 + _("%s : folder, %d subfolders and no bookmark"),
> 388 + name, child_folders_count);
>
> This should be done using ngettext, leaving gettext to figure out plural forms
> (there may be a dual form or other grammatical implications the code can't
> handle).

Interesting. I was not aware of ngettext existence. I will give it a try.

> Is it necessary to implement midori_array_count_recursive using
> midori_array_query_recursive rather than asking SQlite to count rows? This
> seems a bit wasteful.

Well, the count within a folder is done using a COUNT(*) sql statement but SQLite does not implement recursive queries therefore I had to implement recursion within the code.

I nevertheless agree, midori_array_query_recursive was the easy path and this has to be changed.
Especially if we consider the fact that I not only retrieve the sub folder id but also its *uri* (!!!goof!!!)

This being noticed I still have doubts about the feature, initially I thought we could need to implement fancy stuff like giving the counts for the whole tree depth if the user press the 'alt' or 'ctrl' modifiers but the feedback I get with one child level actually suits my needs, I wonder if the need of such feature would come up one day.

If nobody needs it, it's the whole idea of recursion that's wasteful and we could just get rid of it and stick to one child level count which is achieved with a single SQL statement.

I wonder if it was the meaning of the remark?

« Back to merge proposal