GNOME Bugzilla – Bug 786039
Add favorite files
Last modified: 2017-12-08 13:25:34 UTC
See patch.
Created attachment 357247 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Created attachment 357249 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Just wondering why not implement this as gvfs backend in order to make this location accessible for other apps also... for file chooser at least?
Created attachment 357902 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Review of attachment 357902 [details] [review]: Looks quite good generally! I just found minor issues with the patch. ::: src/nautilus-application.c @@ +1127,3 @@ + priv->tag_manager = nautilus_tag_manager_new (priv->tag_manager_tags_cancellable, + priv->tag_manager_notifier_cancellable, + priv->tag_manager_favorite_cancellable); If all operations are going to be cancelled at once, I think it makes sense that we just use/pass once cancellable to control all requests. ::: src/nautilus-column-utilities.c @@ +73,3 @@ "description", _("The type of the file."), NULL)); + Stray whitespace change ::: src/nautilus-favorite-directory.c @@ +194,3 @@ +static gboolean +real_contains_file (NautilusDirectory *directory, + NautilusFile *file) This argument has odd indenting @@ +267,3 @@ + if (callback != NULL) + { + (*callback)(directory, favorite->files, callback_data); Missing space between ) and ( @@ +310,3 @@ + monitor = list->data; + + if (monitor->client == client) To reduce levels of indentation here, I'd suggest either using g_list_find_custom(), or inverting this check to: if (monitor->client != client) continue; So the following code can lose one level. @@ +328,3 @@ + uri = g_file_get_uri (location); + + if (g_strcmp0 (uri, "favorites:///") == 0) It would be nice to have this stuff on a helper function. I see this check repeated in a few places... ::: src/nautilus-file-undo-operations.c @@ +1469,3 @@ + self->priv->starred = starred; + + self->priv->files = nautilus_file_list_copy (files); I think would be nice to have these two variables as construct only properties, so g_object_new() already returns a fully initialized object. ::: src/nautilus-file.c @@ +3683,3 @@ + + if ((file_1_is_favorite && file_2_is_favorite) || + (!file_1_is_favorite && !file_2_is_favorite)) Could be simplified to: if (!!file_1_is_favorite == !!file_2_is_favorite) ::: src/nautilus-files-view.c @@ +1552,3 @@ + if (NAUTILUS_IS_LIST_VIEW (view)) + { + nautilus_list_view_redraw_tree_view (view); This is odd... As the theory goes, the model row/column should change, which would trigger invalidation of the specific cell renderer for that given row. In other words, the update comes from the model. @@ +7812,3 @@ + else + { + show_unstar = FALSE; A minor optimization here is bailing out early if show_star and show_unstar are already true. @@ +9800,3 @@ + priv->favorite_cancellable = g_cancellable_new (); + priv->tag_manager = nautilus_tag_manager_new (NULL, NULL, NULL); No cancellable(s)? ::: src/nautilus-list-view.c @@ +767,3 @@ + gtk_tree_view_column_get_x_offset (column); + + if (event->x > m - 10 && event->x < m + 10) I think it would be better to use gtk_tree_view_get_path_at_pos() to figure out whether the event happened on top of the favorites column. @@ +2152,3 @@ else { + if (!g_strcmp0 (name, "favorite")) My personal preference is to use g_strcmp0() == 0. The '!' makes the retval look boolean, while it's not. @@ +3804,3 @@ list_view->details->regex = g_regex_new ("\\R+", 0, G_REGEX_MATCH_NEWLINE_ANY, NULL); + + list_view->details->tag_manager = nautilus_tag_manager_new (NULL, NULL, NULL); Cancellables are null again :), and this happens in other places too... ::: src/nautilus-list-view.h @@ +54,3 @@ NautilusFilesView * nautilus_list_view_new (NautilusWindowSlot *slot); +void nautilus_list_view_redraw_tree_view (NautilusFilesView *view); We should be able to do without this function. ::: src/nautilus-search-directory.c @@ +638,2 @@ file_list = g_list_prepend (file_list, file); + Stray whitespace ::: src/nautilus-tag-manager.c @@ +25,3 @@ + +#define GINT64_TO_POINTER(i) ((gpointer) (gint64) (i)) +#define GPOINTER_TO_GINT64(p) ((gint64) (p)) I think this can't work on 32-bit platforms, because the gint64 type is wider than the available pointer size. You'll have to use gint64 pointers as hashtable values to ensure it works on both 32 and 64bits. ::: src/nautilus-tag-manager.h @@ +39,3 @@ +NautilusTagManager* nautilus_tag_manager_new (GCancellable *cancellable_tags, + GCancellable *cancellable_notifier, + GCancellable *cancellable_favorite); As mentioned, probably all cancellables could be folded into one. @@ +70,3 @@ + GError **error); + +void nautilus_tag_manager_update_tags (NautilusTagManager *self, Some of this API is unused. Since extending the number of handled tags proves hard with GtkPlacesSidebar, I wonder what should we do about it :(. @@ +103,3 @@ +gchar* parse_color_from_tag_id (const gchar *tag_id); + +TagData* nautilus_tag_data_new (const gchar *id, All this TagData and color API can go too I guess. Since we're not going through that UI concept.
Created attachment 358174 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Created attachment 358180 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Created attachment 358313 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Created attachment 358330 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Comment on attachment 358330 [details] [review] Add favorite files The handling of GET_IDS_FOR_URLS still seems to leak the GTasks after g_task_return_pointer(), feel free to fix in place before pushing. (Setting as accepted-commit_after_freeze in provision of the r-t approval)
Created attachment 358387 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
*** Bug 762928 has been marked as a duplicate of this bug. ***
Created attachment 362018 [details] [review] Add favorite files Add option to make files Favorite, by either toggling a star in the list view, or from the context menu.
Created attachment 365245 [details] [review] meson.build: bump GTK+ version requirement Starred locations in GtkPlacesSidebar are only available in 3.22.26 onward.
Attachment 365245 [details] pushed as ddca5f5 - meson.build: bump GTK+ version requirement