Re: [Tracker] Thumbnail branch review

[Philip Van Hoof <spam pvanhoof be>]

Hi there,

Fixes for these issues can be found here, let me know if the branch is
ready for push to master.

http://git.gnome.org/browse/tracker/commit/?h=thumbnails&id=fd4a19f50793f7aaed0f8e7fe435b5e50abbe019

Cheers and thanks for the quick review, Martyn

Philip

On Mon, 2010-02-01 at 13:31 +0000, Martyn Russell wrote:
> Hi all,
>
> Philip recently re-added some thumbnail work in a branch, this is the 
> review of that branch:
>
>    http://git.gnome.org/browse/tracker/log/?h=thumbnails
>
>
> 1. No need to add if() around these:
>
> + if (mime_types)
> +         g_strfreev (mime_types);
> +
> + if (uri_schemes)
> +         g_strfreev (uri_schemes);
>
>
> 2. We shouldn't need these anymore either, we only use URIs. This would 
> speed things up a bit (there are several cases of this):
>
> + if (!strstr (from_uri, "://")) {
>
>
> 3. I think this is pointless and will fail anyway AFAICS. Looking up 
> file information when the file is removed can't success:
>
> + file_info = g_file_query_info (file,
> +                                G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE,
> +                                G_FILE_QUERY_INFO_NOFOLLOW_SYMLINKS,
> +                                NULL, NULL);
> +
> + if (file_info) {
> +         tracker_thumbnailer_remove_add (uri, 
> g_file_info_get_content_type (file_info));
> +         g_object_unref (file_info);
> + }
> +
>
>
> 4. I am not sure this is the right way to do this. I would also make the 
> 50 a #define so it is easily changed in both places. Right now without 
> this code we already push the updates when the queues are done, this is 
> only problematic if we want more immediate updates (i.e. before the 
> miner is done).
>
> + if ((g_slist_length (private->moves_from) +
> +      g_slist_length (private->removes)) > 50) {
> +         tracker_thumbnailer_send ();
> + }
> +
>
>
> The rest looks fine to me. I share your sentiments on the recursive 
> comment too.

-- 
Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org 
http://pvanhoof.be/blog
http://codeminded.be