Re: [Tracker] Thumbnail branch review



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




[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]