GNOME Bugzilla – Bug 741377
Open containing folder doesn't select the document file in nautilus
Last modified: 2017-05-12 14:01:30 UTC
Currently Evince offers to open the file containing the current document. However, when there are many documents the user has to manually search where it is. The user can be tricked with the document title (bold text in the GtkHeaderBar), which might not be the same as the document name, the secondary entry. In my case, even if know the distinction, my brain gets fooled over and over, and usually I have to search twice. In Firefox, when you ask "Open in containing folder" (for a given download), it opens the file manager and select the file, which feels right. The way Firefox solves the issue by using the File Manager D-BUS interface. If it fails, it fallback to gvfs/gnome-vfs. See: http://www.freedesktop.org/wiki/Specifications/file-manager-interface/ The Firefox commit that solves it: https://hg.mozilla.org/integration/mozilla-inbound/rev/5fc52820456d More discussion on this: https://bugzilla.mozilla.org/show_bug.cgi?id=417952#c12 It seems to me that is the right approach.
The part of the code that needs some love is: https://git.gnome.org/browse/evince/tree/shell/ev-window.c#n2979
Created attachment 293887 [details] [review] Select file in file manager when opening containing folder.
Review of attachment 293887 [details] [review]: Thanks for the patch. It works, but I think it could be improved. ::: shell/ev-window.c @@ +3021,3 @@ + NULL, NULL); + + if (proxy) { If the function were to return TRUE or FALSE, then we could use an early return like: if (!proxy) return FALSE; ... @@ +3054,3 @@ + + g_free (uri); + g_object_unref (proxy); ... and this could be freed before the condition for result. Then, depending of result, we could return TRUE or FALSE, in case of success or failure. @@ +3079,2 @@ + show_file_in_filemanager (file, + gtk_widget_get_screen (ev_window_widget)); And here, we could have something like: if (!open_containing_folder_dbus (...)) open_containing_folder_fallback (...); which seems easier to read/follow.
Created attachment 294180 [details] [review] Select file in file manager when opening containing folder.
Patch updated based on review.
Review of attachment 294180 [details] [review]: Thanks for the update. Some stylistic comments. ::: shell/ev-window.c @@ +3026,3 @@ + gchar *startup_id; + GVariant *params, *result; + GVariantBuilder builder; The declaration should be at the beginning. Previously made sense when it was inside the if, but not anymore. @@ +3081,1 @@ + /* If showing the file via the FileManager dbus interface didn't work */ If the function name has dbus in its name, then you don't need this comment. @@ +3081,3 @@ + /* If showing the file via the FileManager dbus interface didn't work */ + if (!show_file_in_filemanager (file, screen)) + /* Fallback to gtk_show_uri() if launch over DBus is not possible */ I think this comment is redundant considering the function name is clear enough. Mentioning gtk_show_uri is an implementation detail.
Created attachment 294184 [details] [review] Select file in file manager when opening containing folder.
Review of attachment 294184 [details] [review]: Much better, thanks. Let's wait for Carlos review. He has the last word.
Review of attachment 294184 [details] [review]: Thanks for the patch, I have a few comments. ::: shell/ev-window.c @@ +2978,2 @@ static void +show_file_in_filemanager_fallback (GFile *file, GdkScreen *screen) This doesn't show the file, since it's the current code mainly, so I would call this open_containing_folder @@ +3006,3 @@ + +static gboolean +show_file_in_filemanager_dbus (GFile *file, GdkScreen *screen) And this select_file_in_containing_folder(), for example @@ +3016,3 @@ + g_return_val_if_fail (file != NULL, FALSE); + + proxy = g_dbus_proxy_new_for_bus_sync (G_BUS_TYPE_SESSION, Do not use sync API in the main thread, use the async version instead. @@ +3039,3 @@ + + /* params is floating! */ + params = g_variant_new ("(ass)", &builder, startup_id); I think you could do use "(@ass)" and g_variant_builder_end() and you don't need to call clear later. @@ +3045,3 @@ + + /* Floating params-GVariant is consumed here */ + result = g_dbus_proxy_call_sync (proxy, "ShowItems", Use the async version. @@ +3046,3 @@ + /* Floating params-GVariant is consumed here */ + result = g_dbus_proxy_call_sync (proxy, "ShowItems", + params, G_DBUS_CALL_FLAGS_NONE, Don't use a local variable, use g_variant_new directly here since the floating reference will be consumed. @@ +3078,1 @@ + g_return_if_fail (file != NULL); Do not use g_return macros in private functions. Use an early return, or an assert
Created attachment 350610 [details] [review] shell: Let Open in Containing Folder select the active file Use dbus async calls to select a file when opening in containing folder.
Created attachment 350611 [details] [review] shell: Let Open in Containing Folder select the active file Use dbus async calls to select a file when opening in containing folder. Previous format-patch had mangled some headers.
Review of attachment 350611 [details] [review]: Are you sure you need all this? This works in epiphany with the downloads and we simply use g_app_info_launch(). gtk_show_uri() uses g_app_info_launch_default_for_uri) that I assume it's pretty much the same.
Created attachment 350744 [details] [review] shell: Select active file when open containing folder Update patch. Simpler solution as suggested.
Review of attachment 350744 [details] [review]: Yes, much simpler. Please, fix at least the leak before pushing. ::: shell/ev-window.c @@ +2939,1 @@ + app = g_app_info_get_default_for_type ("inode/directory", FALSE); You have to unref the app. @@ +2945,2 @@ + file = g_file_new_for_uri (window->priv->uri); + uri = g_file_get_uri (file); This is only used in case of error, you can move it inside the if (error != NULL) @@ +2951,2 @@ + timestamp = gtk_get_current_event_time (); + list = g_list_append (list, file); Since we know there will always be a list with a single item, we can make it stack allocated. GList list; list.next = list.prev = NULL; list.data = file; g_app_info_launch (app, &list, ...); @@ +2967,3 @@ + g_free (uri); + g_object_unref (context); + g_list_free (list); And you wouldn't need this.
Created attachment 350996 [details] [review] Updated patch with the comments addressed
Germán, please just commit it :) as Carlos already gave the accept modulo the latest comments.
Review of attachment 350996 [details] [review]: Pushed into master