[epiphany/mcatanzaro/file-launching: 7/11] Simplify and fix opening files



commit c91bc72215bd11f86bc3daf5e9966191774c8d1e
Author: Michael Catanzaro <mcatanzaro igalia com>
Date:   Mon Jan 7 20:27:34 2019 -0600

    Simplify and fix opening files
    
    Opening files regressed in 7c8f19af448d42cb498bd729f498f49ebcd554c6.
    EphyDownload always passes NULL for MIME type, so we skip all the code
    for launching applications. Oops.
    
    Instead of reverting this commit, let's delete all the code! We can
    simplify file launching in several ways:
    
    (1) ephy_embed_shell_launch_handler() is no longer needed at all. It's a
    a wrapper around ephy_file_launch_handler() that just checks to ensure
    Epiphany is not launching itself, to avoid recursive launching. This is
    no longer needed because we no longer open files automatically under any
    circumstances, so recursive launching is no longer possible (unless the
    user manually opens the same file again and again for fun or something).
    Remove this function altogether. EphyDownload can use
    ephy_file_launch_handler() directly instead.
    
    (2) Delete the test for ephy_embed_shell_launch_handler(), since it only
    tests to ensure we avoid recursive launching, which is, again, no longer
    a problem.
    
    (3) ephy_file_launch_handler() doesn't actually need the MIME type for
    anything, except for use by ephy_file_browse_to(). So get rid of it from
    all associated functions. Reimplement ephy_file_browse_to() without it.
    (The MIME type used to be important for making sure it's safe to
    automatically open a file, but we never do that anymore.) A mistake
    regarding the MIME types is what introduced the bug that triggered this
    investigation, so eliminating it altogether seems worth it, especially
    when it's only used in the corner case of ephy_file_browse_to().
    
    (4) Make ephy_file_launch_handler() automatically launch via URI handler
    when running in flatpak. This way, higher-level code no longer needs to
    decide between using ephy_file_launch_handler() and
    ephy_file_launch_file_via_uri_handler() based on whether flatpak is in
    use.
    
    (5) ephy_file_launch_file_via_uri_handler() and
    ephy_file_launch_via_uri_handler() no longer need to be public, and can
    be compressed into one function. ephy_file_launch_application also no
    longer needs to be public.
    
    (6) Add assertions to ephy_file_browse_to(),
    ephy_file_launch_desktop_file(), and
    ephy_file_open_uri_in_default_browser() to ensure they are never called
    when running in flatpak, since it's impossible to make them work
    (because the only way to open files in flatpak is via the OpenURI
    portal, which gives the user, not the application, the choice of which
    application to open). It's already impossible for any of these functions
    to be called inside flatpak. (Hopefully! :)
    
    I tried to split this up into separate commits, but it got rather
    complicated, so here it is as an all-in-one.
    
    Note this consequently fixes opening downloaded files in flatpak, albeit
    not in the simplest-possible way.

 embed/ephy-download.c         |   3 +-
 embed/ephy-embed-shell.c      |  47 ----------------
 embed/ephy-embed-shell.h      |   4 --
 lib/ephy-file-helpers.c       | 128 +++++++++++++++++++-----------------------
 lib/ephy-file-helpers.h       |  11 +---
 src/prefs-dialog.c            |   3 +-
 src/window-commands.c         |   4 +-
 tests/ephy-embed-shell-test.c |  21 -------
 8 files changed, 63 insertions(+), 158 deletions(-)
---
diff --git a/embed/ephy-download.c b/embed/ephy-download.c
index d3118822f..e348ec0b9 100644
--- a/embed/ephy-download.c
+++ b/embed/ephy-download.c
@@ -444,8 +444,7 @@ ephy_download_do_download_action (EphyDownload          *download,
       break;
     case EPHY_DOWNLOAD_ACTION_OPEN:
       LOG ("ephy_download_do_download_action: open");
-      ret = ephy_embed_shell_launch_handler (ephy_embed_shell_get_default (),
-                                             destination, NULL, user_time);
+      ret = ephy_file_launch_handler (destination, user_time);
       if (!ret)
         ret = ephy_file_browse_to (destination, user_time);
       break;
diff --git a/embed/ephy-embed-shell.c b/embed/ephy-embed-shell.c
index 02aedc24a..9d2a00578 100644
--- a/embed/ephy-embed-shell.c
+++ b/embed/ephy-embed-shell.c
@@ -1717,53 +1717,6 @@ ephy_embed_shell_get_mode (EphyEmbedShell *shell)
   return priv->mode;
 }
 
-/**
- * ephy_embed_shell_launch_handler:
- * @shell: an #EphyEmbedShell
- * @file: a #GFile to open
- * @mime_type: the mime type of @file or %NULL
- * @user_time: user time to prevent focus stealing
- *
- * Tries to open @file with the right application, making sure we will
- * not call ourselves in the process. This is needed to avoid
- * potential infinite loops when opening unknown file types.
- *
- * Returns: %TRUE on success
- **/
-gboolean
-ephy_embed_shell_launch_handler (EphyEmbedShell *shell,
-                                 GFile          *file,
-                                 const char     *mime_type,
-                                 guint32         user_time)
-{
-  GAppInfo *app;
-  GList *list = NULL;
-  gboolean ret = FALSE;
-
-  g_assert (EPHY_IS_EMBED_SHELL (shell));
-  g_assert (file);
-
-  if (mime_type == NULL)
-    return ret;
-
-  if (ephy_is_running_inside_flatpak ()) {
-    return ephy_file_launch_file_via_uri_handler (file);
-  }
-
-  app = ephy_file_launcher_get_app_info_for_file (file, mime_type);
-
-  /* Do not allow recursive calls into the browser, they can lead to
-   * infinite loops and they should never happen anyway. */
-  if (!app || g_strcmp0 (g_app_info_get_id (app), "org.gnome.Epiphany.desktop") == 0)
-    return ret;
-
-  list = g_list_append (list, file);
-  ret = ephy_file_launch_application (app, list, user_time, NULL);
-  g_list_free (list);
-
-  return ret;
-}
-
 /**
  * ephy_embed_shell_clear_cache:
  * @shell: an #EphyEmbedShell
diff --git a/embed/ephy-embed-shell.h b/embed/ephy-embed-shell.h
index 3319dfcf1..4d9a351b6 100644
--- a/embed/ephy-embed-shell.h
+++ b/embed/ephy-embed-shell.h
@@ -72,10 +72,6 @@ void               ephy_embed_shell_set_print_settings         (EphyEmbedShell
                                                                 GtkPrintSettings *settings);
 GtkPrintSettings  *ephy_embed_shell_get_print_settings         (EphyEmbedShell   *shell);
 EphyEmbedShellMode ephy_embed_shell_get_mode                   (EphyEmbedShell   *shell);
-gboolean           ephy_embed_shell_launch_handler             (EphyEmbedShell   *shell,
-                                                                GFile            *file,
-                                                                const char       *mime_type,
-                                                                guint32           user_time);
 void               ephy_embed_shell_clear_cache                (EphyEmbedShell   *shell);
 void               ephy_embed_shell_set_thumbnail_path         (EphyEmbedShell   *shell,
                                                                 const char       *url,
diff --git a/lib/ephy-file-helpers.c b/lib/ephy-file-helpers.c
index 8784f308f..9536a4a22 100644
--- a/lib/ephy-file-helpers.c
+++ b/lib/ephy-file-helpers.c
@@ -551,7 +551,7 @@ ephy_file_delete_on_exit (GFile *file)
  *
  * Returns: %TRUE if g_app_info_launch() succeeded
  **/
-gboolean
+static gboolean
 ephy_file_launch_application (GAppInfo  *app,
                               GList     *list,
                               guint32    user_time,
@@ -604,6 +604,11 @@ ephy_file_launch_desktop_file (const char *filename,
   GList *list = NULL;
   gboolean ret;
 
+  /* This is impossible to implement inside flatpak. Higher layers must
+   * ensure we don't get here.
+   */
+  g_assert (!ephy_is_running_inside_flatpak ());
+
   app = g_desktop_app_info_new (filename);
   if (parameter) {
     file = g_file_new_for_path (parameter);
@@ -617,44 +622,30 @@ ephy_file_launch_desktop_file (const char *filename,
   return ret;
 }
 
-GAppInfo *
-ephy_file_launcher_get_app_info_for_file (GFile      *file,
-                                          const char *mime_type)
+static gboolean
+launch_via_uri_handler (GFile *file)
 {
-  GAppInfo *app = NULL;
-
-  g_assert (file || mime_type);
+  const char *uri;
+  GdkDisplay *display;
+  GdkAppLaunchContext *context;
+  g_autoptr(GError) error = NULL;
 
-  if (mime_type != NULL) {
-    app = g_app_info_get_default_for_type (mime_type,
-                                           FALSE);
-  } else {
-    GFileInfo *file_info;
-    char *type;
-
-    /* Sniff mime type and check if it's safe to open */
-    file_info = g_file_query_info (file,
-                                   G_FILE_ATTRIBUTE_STANDARD_CONTENT_TYPE,
-                                   0, NULL, NULL);
-    if (file_info == NULL) {
-      return FALSE;
-    }
-    type = g_strdup (g_file_info_get_content_type (file_info));
+  display = gdk_display_get_default ();
+  context = gdk_display_get_app_launch_context (display);
 
-    g_object_unref (file_info);
+  uri = g_file_get_uri (file);
 
-    if (type != NULL && type[0] != '\0') {
-      app = g_app_info_get_default_for_type (type, FALSE);
-    }
-    g_free (type);
+  g_app_info_launch_default_for_uri (uri, G_APP_LAUNCH_CONTEXT (context), &error);
+  if (error) {
+    g_warning ("Failed to launch handler for URI %s: %s", uri, error->message);
+    return FALSE;
   }
 
-  return app;
+  return TRUE;
 }
 
 /**
  * ephy_file_launch_handler:
- * @mime_type: the mime type of @file or %NULL
  * @file: a #GFile to pass as argument
  * @user_time: user time to prevent focus stealing
  *
@@ -664,25 +655,33 @@ ephy_file_launcher_get_app_info_for_file (GFile      *file,
  * Returns: %TRUE on success
  **/
 gboolean
-ephy_file_launch_handler (const char *mime_type,
-                          GFile      *file,
-                          guint32     user_time)
+ephy_file_launch_handler (GFile   *file,
+                          guint32  user_time)
 {
   GAppInfo *app = NULL;
   gboolean ret = FALSE;
+  g_autoptr(GList) list = NULL;
+  g_autoptr(GError) error = NULL;
 
   g_assert (file != NULL);
 
-  app = ephy_file_launcher_get_app_info_for_file (file, mime_type);
-
-  if (app != NULL) {
-    GList *list = NULL;
+  /* Launch via URI handler only under flatpak, because this way loses
+   * focus stealing prevention. There's no other way to open a file
+   * under flatpak, and focus stealing prevention becomes the
+   * responsibility of the portal in this case anyway. */
+  if (ephy_is_running_inside_flatpak ())
+    return launch_via_uri_handler (file);
 
-    list = g_list_append (list, file);
-    ret = ephy_file_launch_application (app, list, user_time, NULL);
-    g_list_free (list);
+  app = g_file_query_default_handler (file, NULL, &error);
+  if (!app) {
+    g_autofree char *path = g_file_get_path (file);
+    g_warning ("No available application to open %s: %s", path, error->message);
+    return FALSE;
   }
 
+  list = g_list_append (list, file);
+  ret = ephy_file_launch_application (app, list, user_time, NULL);
+
   return ret;
 }
 
@@ -697,6 +696,11 @@ ephy_file_open_uri_in_default_browser (const char *uri,
   gboolean retval = TRUE;
   GError *error = NULL;
 
+  /* This is impossible to implement inside flatpak. Higher layers must
+   * ensure we don't get here.
+   */
+  g_assert (!ephy_is_running_inside_flatpak ());
+
   context = gdk_display_get_app_launch_context (screen ? gdk_screen_get_display (screen) : 
gdk_display_get_default ());
   gdk_app_launch_context_set_screen (context, screen);
   gdk_app_launch_context_set_timestamp (context, timestamp);
@@ -732,7 +736,21 @@ gboolean
 ephy_file_browse_to (GFile  *file,
                      guint32 user_time)
 {
-  return ephy_file_launch_handler ("inode/directory", file, user_time);
+  g_autoptr(GFile) parent = NULL;
+
+  /* This is impossible to implement inside flatpak. Higher layers must
+   * ensure we don't get here.
+   */
+  g_assert (!ephy_is_running_inside_flatpak ());
+
+  parent = g_file_get_parent (file);
+
+  /* If parent is NULL, then the file is / and that would be nuts. This
+   * function is not for directories, anyway.
+   */
+  g_assert (parent);
+
+  return ephy_file_launch_handler (parent, user_time);
 }
 
 /**
@@ -966,33 +984,3 @@ ephy_open_incognito_window (const char *uri)
 
   g_free (command);
 }
-
-gboolean
-ephy_file_launch_via_uri_handler (const char *uri)
-{
-  GdkDisplay *display;
-  GdkAppLaunchContext *context;
-  GError *error = NULL;
-
-  display = gdk_display_get_default ();
-  context = gdk_display_get_app_launch_context (display);
-
-  g_app_info_launch_default_for_uri (uri, G_APP_LAUNCH_CONTEXT (context), &error);
-
-  if (error != NULL) {
-    g_warning ("Failed to launch handler for URI %s: %s", uri, error->message);
-    g_error_free (error);
-    return FALSE;
-  }
-
-  return TRUE;
-}
-
-gboolean
-ephy_file_launch_file_via_uri_handler (GFile *file)
-{
-  const char *uri;
-
-  uri = g_file_get_uri (file);
-  return ephy_file_launch_via_uri_handler (uri);
-}
diff --git a/lib/ephy-file-helpers.h b/lib/ephy-file-helpers.h
index eff671d89..641273dba 100644
--- a/lib/ephy-file-helpers.h
+++ b/lib/ephy-file-helpers.h
@@ -71,12 +71,7 @@ gboolean           ephy_file_launch_desktop_file            (const char
                                                              const char            *parameter,
                                                              guint32                user_time,
                                                              GtkWidget             *widget);
-gboolean           ephy_file_launch_application             (GAppInfo              *app,
-                                                             GList                 *files,
-                                                             guint32                user_time,
-                                                             GtkWidget             *widget);
-gboolean           ephy_file_launch_handler                 (const char            *mime_type,
-                                                             GFile                 *file,
+gboolean           ephy_file_launch_handler                 (GFile                 *file,
                                                              guint32                user_time);
 gboolean           ephy_file_open_uri_in_default_browser    (const char            *uri,
                                                              guint32                timestamp,
@@ -91,11 +86,7 @@ gboolean           ephy_file_move_uri                       (const char
 char       *       ephy_file_create_data_uri_for_filename   (const char            *filename,
                                                              const char            *mime_type);
 char       *       ephy_sanitize_filename                   (char                  *filename);
-GAppInfo   *       ephy_file_launcher_get_app_info_for_file (GFile                 *file,
-                                                             const char            *mime_type);
 void               ephy_open_default_instance_window        (void);
 void               ephy_open_incognito_window               (const char            *uri);
-gboolean           ephy_file_launch_via_uri_handler         (const char            *uri);
-gboolean           ephy_file_launch_file_via_uri_handler    (GFile                 *file);
 
 G_END_DECLS
diff --git a/src/prefs-dialog.c b/src/prefs-dialog.c
index 761af6d03..a2438ae11 100644
--- a/src/prefs-dialog.c
+++ b/src/prefs-dialog.c
@@ -1114,8 +1114,7 @@ css_edit_button_clicked_cb (GtkWidget   *button,
   if (ephy_is_running_inside_flatpak ()) {
     g_file_create_async (css_file, G_FILE_CREATE_NONE, G_PRIORITY_DEFAULT, NULL, css_file_created_cb, NULL);
   } else {
-    ephy_file_launch_handler ("text/plain", css_file,
-                              gtk_get_current_event_time ());
+    ephy_file_launch_handler (css_file, gtk_get_current_event_time ());
     g_object_unref (css_file);
   }
 }
diff --git a/src/window-commands.c b/src/window-commands.c
index 5b695294e..dd832346b 100644
--- a/src/window-commands.c
+++ b/src/window-commands.c
@@ -1872,7 +1872,7 @@ save_temp_source_close_cb (GOutputStream *ostream, GAsyncResult *result, gpointe
     goto out;
   }
 
-  if (!ephy_file_launch_handler ("text/plain", file, gtk_get_current_event_time ())) {
+  if (!ephy_file_launch_handler (file, gtk_get_current_event_time ())) {
     /* Fallback to view the source inside the browser */
     EphyEmbed *embed;
 
@@ -2059,7 +2059,7 @@ window_cmd_page_source (GSimpleAction *action,
     GFile *file;
 
     file = g_file_new_for_uri (address);
-    ephy_file_launch_handler ("text/plain", file, user_time);
+    ephy_file_launch_handler (file, user_time);
 
     g_object_unref (file);
   } else {
diff --git a/tests/ephy-embed-shell-test.c b/tests/ephy-embed-shell-test.c
index e305434c2..59252ea87 100644
--- a/tests/ephy-embed-shell-test.c
+++ b/tests/ephy-embed-shell-test.c
@@ -30,24 +30,6 @@
 #include <glib.h>
 #include <gtk/gtk.h>
 
-static void
-test_ephy_embed_shell_launch_handler (void)
-{
-  EphyEmbedShell *embed_shell;
-  gboolean ret;
-  GFile *file;
-
-  embed_shell = ephy_embed_shell_get_default ();
-
-  file = g_file_new_for_path (TEST_DIR "/data/test.html");
-  g_assert (file);
-
-  ret = ephy_embed_shell_launch_handler (embed_shell, file, NULL, 0);
-  g_assert (ret == FALSE);
-
-  g_object_unref (file);
-}
-
 static void
 web_view_created_cb (EphyEmbedShell *shell, EphyWebView *view, gpointer user_data)
 {
@@ -114,9 +96,6 @@ main (int argc, char *argv[])
   _ephy_shell_create_instance (EPHY_EMBED_SHELL_MODE_TEST);
   g_application_register (G_APPLICATION (ephy_embed_shell_get_default ()), NULL, NULL);
 
-  g_test_add_func ("/embed/ephy-embed-shell/launch_handler",
-                   test_ephy_embed_shell_launch_handler);
-
   g_test_add_func ("/embed/ephy-embed-shell/web-view-created",
                    test_ephy_embed_shell_web_view_created);
 


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