GNOME Bugzilla – Bug 639928
Update Tracker plugin to support new libtracker-sparql API
Last modified: 2011-01-28 12:40:41 UTC
Still uses old libtracker-client API which is now deprecated.
Created attachment 178698 [details] [review] Fixes the bug
Review of attachment 178698 [details] [review]: A couple of things to clean up, but looks good overall. ::: src/plugins/tracker/totem-tracker-widget.c @@ +169,2 @@ file = g_file_new_for_uri (uri); + info = g_file_query_info (file, "thumbnail::path", G_FILE_QUERY_INFO_NONE, NULL, &error); Should this be made into an async function instead? Right now, it would be blocking on getting info results. @@ +173,2 @@ + if (error) { +#if 0 Remove code if it won't be used. @@ +197,1 @@ + if (thumbnail) { No need for curly braces for a single line condition. @@ +199,3 @@ } + if (info) { Ditto.
(In reply to comment #2) > Review of attachment 178698 [details] [review]: > > A couple of things to clean up, but looks good overall. > > ::: src/plugins/tracker/totem-tracker-widget.c > @@ +169,2 @@ > file = g_file_new_for_uri (uri); > + info = g_file_query_info (file, "thumbnail::path", G_FILE_QUERY_INFO_NONE, > NULL, &error); > > Should this be made into an async function instead? Right now, it would be > blocking on getting info results. Yes, that makes sense. I didn't change it, but I can do that now. > @@ +173,2 @@ > + if (error) { > +#if 0 > > Remove code if it won't be used. Yea, didn't know if you wanted to keep it. It seems silly IMO to put a dialog up because a thumbnail couldn't be found, but if you're happy, I will remove it. > @@ +197,1 @@ > + if (thumbnail) { > > No need for curly braces for a single line condition. Habits ;) > @@ +199,3 @@ > } > > + if (info) { > > Ditto. Will fix these up ASAP
Review of attachment 178698 [details] [review]: ::: src/plugins/tracker/totem-tracker-widget.c @@ +518,3 @@ + if (srd->widget->priv->total_result_count < 1) { + gtk_label_set_text (GTK_LABEL (priv->status_label), _("No results")); + return; This will leak 'srd' if there are no results.
Review of attachment 178698 [details] [review]: A few more leaks managed to sneak into the patch. ::: src/plugins/tracker/totem-tracker-widget.c @@ +309,3 @@ + g_warning ("Call to tracker_sparql_cursor_next_finish() failed getting next hit: %s", error->message); + search_finish (srd, error); + return; Unref cursor before calling finish. @@ +314,3 @@ + if (!more_results) { + /* got all results */ + search_finish (srd, NULL); Same here. @@ +360,3 @@ + /* got all results */ + search_finish (srd, NULL); + return; This can never happen. If there is no error, you will always get a cursor, even if there are no results. @@ +442,3 @@ + g_warning ("Call to tracker_sparql_cursor_next() failed getting hit count: %s", error->message); + search_finish (srd, error); + return; Unref cursor before calling finish. @@ +445,3 @@ + } + + srd->widget->priv->total_result_count = tracker_sparql_cursor_get_integer (cursor, 0); Unref cursor as you no longer use it after this line.
(In reply to comment #4) > Review of attachment 178698 [details] [review]: > > ::: src/plugins/tracker/totem-tracker-widget.c > @@ +518,3 @@ > + if (srd->widget->priv->total_result_count < 1) { > + gtk_label_set_text (GTK_LABEL (priv->status_label), _("No results")); > + return; > > This will leak 'srd' if there are no results. Eek, how did I miss that, nice catch.
(In reply to comment #5) > Review of attachment 178698 [details] [review]: > > A few more leaks managed to sneak into the patch. > > ::: src/plugins/tracker/totem-tracker-widget.c > @@ +309,3 @@ > + g_warning ("Call to tracker_sparql_cursor_next_finish() failed getting > next hit: %s", error->message); > + search_finish (srd, error); > + return; > > Unref cursor before calling finish. Ah of course, it's not a new data object. I had in mind that it would return NULL for the cursor and hence would have nothing to free. Well spotted. > @@ +314,3 @@ > + if (!more_results) { > + /* got all results */ > + search_finish (srd, NULL); > > Same here. > > @@ +360,3 @@ > + /* got all results */ > + search_finish (srd, NULL); > + return; > > This can never happen. If there is no error, you will always get a cursor, even > if there are no results. Right. I think with the old API, you could have no error but a GPtrArray which was NULL, I forgot we always returned a pointer even for no results. > @@ +442,3 @@ > + g_warning ("Call to tracker_sparql_cursor_next() failed getting > hit count: %s", error->message); > + search_finish (srd, error); > + return; > > Unref cursor before calling finish. > > @@ +445,3 @@ > + } > + > + srd->widget->priv->total_result_count = > tracker_sparql_cursor_get_integer (cursor, 0); > > Unref cursor as you no longer use it after this line. After applying all these, everything seems to work perfectly without that any criticals.
Created attachment 178856 [details] [review] Fixes issues raised up to now in this bug report
I'll let Jürg do another pass at reviewing. You'll need to make changes to configure.ac to up the dependencies, if required. Rest looks fine to me.
Created attachment 179505 [details] [review] Full patch Martyn accidentally attached an incremental patch. I've now attached the full patch with just a tiny change - it was saying "Showing 0 - 20 of N matches" instead of "Showing 1 - 20 of N matches". The rest looks fine to me and appears to work well (tested with Tracker 0.9.36).
Comment on attachment 179505 [details] [review] Full patch Changed the commit message to match that of other plugins. Thanks very much for the review and the updated patch.
commit b8245b585074fe4a14147e6c00a222008a1bd2ad Author: Martyn Russell <martyn@lanedo.com> Date: Wed Jan 19 09:55:17 2011 +0000 tracker: Use new libtracker-sparql API https://bugzilla.gnome.org/show_bug.cgi?id=639928