GNOME Bugzilla – Bug 768175
bookmark-list: port to GTask
Last modified: 2016-08-13 08:27:23 UTC
As the quest to eliminate deprecated code continues, I’ve decided to tackle bookmark-list.
Created attachment 330556 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Review of attachment 330556 [details] [review]: Nice job, I think this is the last nail in the coffin of non-GTask multithreading in Nautilus! Still a few little things that I think should be adjusted though. ::: src/nautilus-bookmark-list.c @@ +335,3 @@ + + if (error != NULL) { + g_warning ("Unable to get contents of the bookmarks file: %s", there's a similar warning in load_io_thread as well so you might want to remove one of them. also, I think error will never be != NULL, because g_task_return_error is not used and the task cannot be cancelled @@ +344,1 @@ if (contents == NULL) { can contents be NULL if error is also NULL? @@ +397,2 @@ } g_error_free (error); you should use g_task_return_error here @@ +454,3 @@ g_object_unref (parent); + contents = g_task_propagate_pointer (task, &error); use task_data instead of returning and propagating a pointer. also, I'm not sure error will ever be != NULL, because the task cannot be cancelled and there is no other operation that can cause errors before the thread func is called. @@ +521,2 @@ contents = g_string_free (bookmark_string, FALSE); + g_task_return_pointer (task, contents, g_free); looking at the above code, "contents" should rather be task_data, not the return value of the task
Created attachment 330569 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Created attachment 330583 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Created attachment 330587 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Review of attachment 330587 [details] [review]: The patch looks good to me except this small comment. Razvan, can you double check the patch too and mark as accept_commit_now if you agree (after the small comment I did fixed)? Thanks Ernestas! GTask is kinda hard the first time :/ ::: src/nautilus-bookmark-list.c @@ +427,3 @@ + } + + g_warning ("Unable to replace contents of the bookmarks file: %s", The docs say "if an error has occurred, this function will return FALSE and set error appropriately if present." That means returning FALSE and not having error set is fine (I'm not aware of the details though). So this comment can be removed, or at least reworded to something like: "g_file_replace_contents can be unsuccessful and not set an error."
(In reply to Carlos Soriano from comment #6) > ::: src/nautilus-bookmark-list.c > @@ +427,3 @@ > + } > + > + g_warning ("Unable to replace contents of the bookmarks file: %s", Did you perhaps mean this? + // g_file_replace_contents() returned FALSE mysteriously. + if (!success) { + g_warning ("Unable to replace contents of the bookmarks file."); + }
I think bugzilla is tricking us :( click on the review link, you will see exactly the line I was reviewing (which is the one you point to)
Created attachment 330769 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Review of attachment 330769 [details] [review]: besides that comment, the patch looks good to me! ::: src/nautilus-bookmark-list.c @@ +427,3 @@ + } + + // g_file_replace_contents() returned FALSE, but did not set an error. Use /* ... */ for comments
Created attachment 331002 [details] [review] bookmark-list: port to GTask GSimpleAsyncResult utilities have been deprecated since GLib version 2.46 and using GTask is recommended. This commit replaces the deprecated code.
Attachment 331002 [details] pushed as 54b6341 - bookmark-list: port to GTask
Review of attachment 331002 [details] [review]: Congrats, you broke Nautilus for the first time! :) ::: src/nautilus-bookmark-list.c @@ +415,3 @@ { + NautilusBookmarkList *self = NAUTILUS_BOOKMARK_LIST (source_object); + GError *error; Initialize this to NULL.
Created attachment 333204 [details] [review] bookmark-list: null-initialize error pointer A GError pointer was not being null-initialized, which can result in crashes. This commit fixes that.
(In reply to Razvan Chitu from comment #14) > Congrats, you broke Nautilus for the first time! :) And here’s to many more. *clink* :)
Review of attachment 333204 [details] [review]: LGTM thanks!
Attachment 333204 [details] pushed as d316950 - bookmark-list: null-initialize error pointer