Merge lp:~gue5t/midori/suggestion-regression into lp:midori

[gue5t gue5t]

Fix Midori.Download.get_filename_suggestion_for_uri regression since r6498

Midori.Download.get_filename_suggestion_for_uri accepts arbitrary web URIs, not WebKitDownload-style file:// "destination" URIs (which is what Midori.URI.get_basename_for_display accepts). This was confused in r6498. Fix the confusion and move Midori.URI.get_basename_for_display to Midori.Download.get_basename_for_display to make it clearer that it only works on file:// URIs.

[Cris Dywan (kalikiana)]

What "arbitrary web URI" does get_basename_for_display not support that get_filename_suggestion_for_uri does handle? I don't see an obvious reason why get_basename_for_display would not take all URLs - originally it even was proposed to be implemented exactly as done here.

review: Needs Information

[gue5t gue5t (gue5t)]

Midori.URI.get_basename_for_display was introduced to be used with the file:// URIs that the WebKitDownload API uses for its "destinations". It uses "Filename.from_uri", which fails on non-"file://" URIs: g_filename_from_uri("http://google.com/foo/bar/baz.txt", NULL , NULL) == NULL. Hence, it was only intended to be used with these WebKitDownload-provided uris and it was a mistake to have used it to implement get_filename_suggestion_for_uri which takes an arbitrary web (http/https) URI and returns a filesystem path. This restriction on the argument to Midori.URI.get_basename_for_display is documented, but we could also change its name to clarify so this kind of bug doesn't recur.

[Cris Dywan (kalikiana)]

Nice! Totally like your docs tweaks also!

review: Approve

[Cris Dywan (kalikiana)]

Nice! Totally like your docs tweaks also!

review: Approve

=== modified file 'extensions/transfers.vala'
--- extensions/transfers.vala	2013-11-22 19:54:24 +0000
+++ extensions/transfers.vala	2014-01-06 21:54:39 +0000
@@ -325,7 +325,7 @@
             progress.show_text = true;
 #endif
             progress.ellipsize = Pango.EllipsizeMode.MIDDLE;
-            string filename = Midori.URI.get_basename_for_display (transfer.destination);
+            string filename = Midori.Download.get_basename_for_display (transfer.destination);
             progress.text = filename;
             int width;
             Sokoke.widget_get_text_size (progress, "M", out width, null);
@@ -469,7 +469,7 @@
                 }
 
                 string uri = transfer.destination;
-                string filename = Midori.URI.get_basename_for_display (uri);
+                string filename = Midori.Download.get_basename_for_display (uri);
                 var item = new Katze.Item ();
                 item.uri = uri;
                 item.name = filename;

=== modified file 'katze/midori-uri.vala'
--- katze/midori-uri.vala	2013-11-22 19:54:24 +0000
+++ katze/midori-uri.vala	2014-01-06 21:54:39 +0000
@@ -178,25 +178,6 @@
             return null;
         }
 
-        /**
-         * Returns a string showing a file:// URI's intended filename on
-         * disk, suited for displaying to a user.
-         * 
-         * The string returned is the basename (final path segment) of the
-         * filename of the uri. If the uri is invalid, not file://, or has no
-         * basename, the uri itself is returned.
-         * 
-         * Since: 0.5.7
-         **/
-        public static string get_basename_for_display (string uri) {
-            try {
-                string filename = Filename.from_uri (uri);
-                if(filename != null && filename != "")
-                    return Path.get_basename (filename);
-            } catch (Error error) { }
-            return uri;
-        }
-
         public static GLib.ChecksumType get_fingerprint (string uri,
             out string checksum, out string label) {
 

=== modified file 'midori/midori-download.vala'
--- midori/midori-download.vala	2013-11-22 19:54:24 +0000
+++ midori/midori-download.vala	2014-01-06 21:54:39 +0000
@@ -54,7 +54,7 @@
 
         public static string get_tooltip (WebKit.Download download) {
 #if !HAVE_WEBKIT2
-            string filename = Midori.URI.get_basename_for_display (download.destination_uri);
+            string filename = Midori.Download.get_basename_for_display (download.destination_uri);
             /* i18n: Download tooltip (size): 4KB of 43MB */
             string size = _("%s of %s").printf (
                 format_size (download.current_size),
@@ -256,9 +256,13 @@
 #endif
         }
 
+        /**
+         * Returns a filename of the form "name.ext" to use as a suggested name for
+         * a download of the given uri
+         */
         public string get_filename_suggestion_for_uri (string mime_type, string uri) {
             return_val_if_fail (Midori.URI.is_location (uri), uri);
-            string filename = Midori.URI.get_basename_for_display (uri);
+            string filename = File.new_for_uri (uri).get_basename ();
             if (uri.index_of_char ('.') == -1)
                 return Path.build_filename (filename, fallback_extension (null, mime_type));
             return filename;
@@ -297,6 +301,25 @@
             return filename;
         }
 
+        /**
+         * Returns a string showing a file:// URI's intended filename on
+         * disk, suited for displaying to a user.
+         * 
+         * The string returned is the basename (final path segment) of the
+         * filename of the uri. If the uri is invalid, not file://, or has no
+         * basename, the uri itself is returned.
+         * 
+         * Since: 0.5.7
+         **/
+        public static string get_basename_for_display (string uri) {
+            try {
+                string filename = Filename.from_uri (uri);
+                if(filename != null && filename != "")
+                    return Path.get_basename (filename);
+            } catch (Error error) { }
+            return uri;
+        }
+
         public string prepare_destination_uri (WebKit.Download download, string? folder) {
             string suggested_filename = get_suggested_filename (download);
             string basename = Path.get_basename (suggested_filename);
@@ -316,9 +339,13 @@
             }
         }
 
-        public static bool has_enough_space (WebKit.Download download, string uri) {
+        /**
+         * Returns whether it seems possible to save @download to the path specified by
+         * @destination_uri, considering space on disk and permissions
+         */
+        public static bool has_enough_space (WebKit.Download download, string destination_uri) {
 #if !HAVE_WEBKIT2
-            var folder = File.new_for_uri (uri).get_parent ();
+            var folder = File.new_for_uri (destination_uri).get_parent ();
             bool can_write;
             uint64 free_space;
             try {
@@ -337,12 +364,12 @@
                 string detailed_message;
                 if (!can_write) {
                     message = _("The file \"%s\" can't be saved in this folder.").printf (
-                        Midori.URI.get_basename_for_display (uri));
+                        Midori.Download.get_basename_for_display (destination_uri));
                     detailed_message = _("You don't have permission to write in this location.");
                 }
                 else if (free_space < download.total_size) {
                     message = _("There is not enough free space to download \"%s\".").printf (
-                        Midori.URI.get_basename_for_display (uri));
+                        Midori.Download.get_basename_for_display (destination_uri));
                     detailed_message = _("The file needs %s but only %s are left.").printf (
                         format_size (download.total_size), format_size (free_space));
                 }