Merge lp:~niclasl/pantheon-files/fix-1198334 into lp:~elementary-apps/pantheon-files/trunk

[Niclas Lockner]

Mount a remote share before entering it so that the display name can be properly read when refreshing the tab (lp:1198334)

Mount a remote share before entering it so that the display name can be properly read when refreshing the tab.

[David Gomes (davidgomes)]

Cannot replicate bug ergo cannot review this.

[Niclas Lockner (niclasl)]

To reproduce it, I added a bookmark to a smb share which I then opened in Files. Without this fix, you should see "folder does not exist" as the tab title after the share has been mounted and the directory has been loaded.

[David Gomes (davidgomes)]

Yeah, but I have a couple of SSH entries on my Files sidebar and when I open them the tab title is the name of the folder it opens.

[Cody Garver (codygarver)]

I still get it for some places where I need credentials or where it turns out a share apparently does not exist as advertised: http://i.imgur.com/ycCZ48I.png[

[Jeremy Wootten (jeremywootten)]

It appears that this is a timing issue. I only get the problem when navigating to an unmounted share - this is because it takes longer to mount the share and establish the folder exists than it does to create and activate the slot (at which point the tab name is written). However, once the share is mounted, the information is available before the slot becomes active, even if you close and reopen the program. If you log out and back in the problem recurs (once).

The branch cures the problem to the extent that once the share has been mounted and loaded, the tab name is corrected, although it still shows "Folder does not exist" until then.

A possibly better solution would be to name the tab on the assumption that the folder exists and only change it to "Folder does not exist" once it has been established that the folder really does not exist. It could be argued that it is not necessary to rename the tab even then because the content gives the "Folder does not exist" information.

[Robert Roth (evfool)]

@Niclas Lockner: Could you implement Jeremy's suggestion from his last comment: name the tab with the folder name and change it to "Folder does not exist" if the connection is established and the folder is really missing?

@Jeremy Wooten: I think there's no point in arguing about not renaming the tab even when the folder does not exist, as the user somehow has to be informed that the folder does not exist, and forced to select another folder from the share/ssh. This could be an infobar, showing an edit button to edit the location the bookmark is pointing to, but that should be treated as a separate enhancement.

review: Needs Fixing

[Niclas Lockner (niclasl)]

I've started looking into this again and it seems to be hard to fix this in a clean way.

The problem is that a directory's display name, which is used as the tab name right now, is only accessible through a FileInfo object, but the GOF.File's FileInfo property isn't accessible as long as the share isn't mounted (GLib.File.query_info returns null for unmounted shares).
So what should be shown as the tab label until the share has been properly mounted?

One possibility is to manually extract the basename from the uri and add it as a tab label, e.g.
sftp://foo/bar would get "bar" as the tab label, but that approach doesn't work well on smb shares, since the display name of "smb://foo/bar" is "bar on foo" and not "foo". I.e. if a user moves away from a smb share and then go back to it, the user will see two different labels for the same directory.

Another approach is to let the tab label be empty until it can be properly named by the directory_done_loading method.

[Cody Garver (codygarver)]

Talked to Dan about it, he said "Connecting…" with a gtk.spinner tab icon if possible.

[Niclas Lockner (niclasl)]

I've committed a working version now, but it has an issue with the tab icons which is fixed by lp:~niclasl/granite/tab-icon-visibility

Once that branch has been merged, I'll propose this for merging.

[Cody Garver (codygarver)]

Use a real elipsis …

[Niclas Lockner (niclasl)]

As in I should bring back the Gtk.Label I removed?

[Cody Garver (codygarver)]

Copy and paste the elipsis character in this comment over the three periods on diff line 30 …

[Cody Garver (codygarver)]

Also I notice that if you cancel the authentication dialog it continues to say it's loading

[Niclas Lockner (niclasl)]

I noticed that too. Would it be an acceptable behavior to change directory to the user's home directory when cancel is clicked? If not, what should happen?

[Cody Garver (codygarver)]

What about kicking them up a "level"? e.g.

Entire Network > click Some Host > click Some Share that asks for password > Cancel > Get returned to Some Host

And would that work for everything? I've only been testing this with samba shares

[Cody Garver (codygarver)]

I just tested nautilus 3.10 and it doesn't even enter the share until you successfully authenticate.

Let's see what Dan says.

[Niclas Lockner (niclasl)]

I use bookmarks to smb and ssh shares and there's no "up" if I click on them. Another solution would be to just go back to the previous location (and maybe to home if there's no previous location). You would sort of get the behavior you described, but only if you actually clicked Entire Network > Some host > Some share.

[Danielle Foré (danrabbit)]

It might be better to, like Cody suggested, wait to enter the directory until after we've authenticated.

[Niclas Lockner (niclasl)]

Now it mounts the share before entering it, and if the mount operation fails it stays in the current directory.

[Cody Garver (codygarver)]

Everything with this branch is great, we just need to verify the required dynamicnotebook changes here and in granite are good

review: Approve

[RabbitBot (rabbitbot-a)]

Attempt to merge into lp:pantheon-files failed due to conflicts:

text conflict in src/View/ViewContainer.vala

[lp:~niclasl/pantheon-files/fix-1198334]

1474. By Niclas Lockner on 2014-07-31

Merge with trunk

=== modified file 'libcore/gof-directory-async.vala'
--- libcore/gof-directory-async.vala	2014-06-05 06:52:48 +0000
+++ libcore/gof-directory-async.vala	2014-07-31 20:18:53 +0000
@@ -139,11 +139,6 @@
             if (state == State.LOADING)
                 clear_directory_info ();
 
-            if (!file.is_mounted) {
-                mount_mountable.begin ();
-                return;
-            }
-
             list_directory.begin ();
 
             try {
@@ -204,25 +199,19 @@
         }
     }
 
-    private async void mount_mountable () {
+    public async void mount_mountable () throws Error {
         debug ("mount_mountable %s", file.uri);
 
         /* TODO pass GtkWindow *parent to Gtk.MountOperation */
         var mount_op = new Gtk.MountOperation (null);
 
-        try {
-            if (file.file_type != FileType.MOUNTABLE) {
-                yield location.mount_enclosing_volume (0, mount_op, cancellable);
-            } else {
-                yield location.mount_mountable (0, mount_op, cancellable);
-            }
+        if (file.file_type != FileType.MOUNTABLE)
+            yield location.mount_enclosing_volume (0, mount_op, cancellable);
+        else
+            yield location.mount_mountable (0, mount_op, cancellable);
 
-            file.is_mounted = true;
-            yield query_info_async (file, file_info_available);
-            load ();
-        } catch (Error e) {
-            warning ("mount_mountable failed: %s %s", e.message, file.uri);
-        }
+        file.is_mounted = true;
+        yield query_info_async (file, file_info_available);
     }
 
     private async void list_directory () {
@@ -278,15 +267,11 @@
             if (err is IOError.NOT_FOUND || err is IOError.NOT_DIRECTORY)
                 file.exists = false;
 
-            if (err is IOError.PERMISSION_DENIED)
+            else if (err is IOError.PERMISSION_DENIED)
                 permission_denied = true;
 
-            if (err is IOError.NOT_MOUNTED) {
+            else if (err is IOError.NOT_MOUNTED)
                 file.is_mounted = false;
-                /* try again this time it shoould be mounted */
-                load ();
-                return;
-            }
         }
 
         //TODO send err code

=== modified file 'src/View/ViewContainer.vala'
--- src/View/ViewContainer.vala	2014-07-16 19:21:40 +0000
+++ src/View/ViewContainer.vala	2014-07-31 20:18:53 +0000
@@ -27,7 +27,7 @@
         public Gtk.Widget? content_item;
         public bool content_shown = false;
         public bool can_show_folder = true;
-        public Gtk.Label label;
+        private string label;
         private Marlin.View.Window window;
         public GOF.Window.Slot? slot = null;
         public Marlin.Window.Columns? mwcol = null;
@@ -43,6 +43,7 @@
         public signal void back (int n=1);
         public signal void forward (int n=1);
         public signal void tab_name_changed (string tab_name);
+        public signal void loading (bool is_loading);
 
         public ViewContainer (Marlin.View.Window win, GLib.File location, int _view_mode = 0) {
             window = win;
@@ -51,11 +52,7 @@
 
             /* set active tab */
             browser = new Browser ();
-            label = new Gtk.Label ("Loading...");
-            label.set_ellipsize (Pango.EllipsizeMode.END);
-            label.set_single_line_mode (true);
-            label.set_alignment (0.0f, 0.5f);
-            label.set_padding (0, 0);
+            label = _("Loading…");
             window.button_back.fetcher = get_back_menu;
             window.button_forward.fetcher = get_forward_menu;
 
@@ -116,11 +113,11 @@
 
         public string tab_name {
             set {
-                label.label = value;
+                label = value;
                 tab_name_changed (value);
             }
             get {
-                return label.label;
+                return label;
             }
         }
 
@@ -139,13 +136,14 @@
         }
 
         public void refresh_slot_info () {
+            loading (false);
             var aslot = get_active_slot ();
             var slot_path = aslot.directory.file.location.get_path ();
             if (slot_path == Environment.get_home_dir ())
                 tab_name = _("Home");
             else if (slot_path == "/")
                 tab_name = _("File System");
-            else if (slot.directory.file.exists && (aslot.directory.file.info is FileInfo))
+            else if (aslot.directory.file.exists && (aslot.directory.file.info is FileInfo))
                 tab_name = aslot.directory.file.info.get_attribute_string (FileAttribute.STANDARD_DISPLAY_NAME);
             else {
                 tab_name = _("This folder does not exist");
@@ -188,7 +186,12 @@
                 }
             } catch (Error err) {
                 /* query_info will throw an expception if it cannot find the file */
-                content = new DirectoryNotFound (slot.directory, this);
+
+                if (err is IOError.NOT_MOUNTED) {
+                    reload ();
+                } else {
+                    content = new DirectoryNotFound (slot.directory, this);
+                }
             }
 
             warning ("directory done loading");
@@ -223,26 +226,54 @@
                         select_childs.prepend (slot.directory.file.location);
                 }
             }
-            if (slot != null && slot.directory != null && slot.directory.file.exists) {
-                slot.directory.cancel ();
-                slot.directory.track_longest_name = false;
-            }
 
             if (focus_file != null)
                 select_childs.prepend (focus_file);
 
+            Marlin.Window.Columns new_mwcol;
+            GOF.Window.Slot new_slot;
+
             if (nview == ViewMode.MILLER) {
-                mwcol = new Marlin.Window.Columns (location, this);
-                slot = mwcol.active_slot;
+                new_mwcol = new Marlin.Window.Columns (location, this);
+                new_slot = mwcol.active_slot;
             } else {
-                mwcol = null;
-                slot = new GOF.Window.Slot (location, this);
+                new_mwcol = null;
+                new_slot = new GOF.Window.Slot (location, this);
             }
 
             /* automagicly enable icon view for icons keypath */
-            if (!user_change_rq && slot.directory.uri_contain_keypath_icons)
+            if (!user_change_rq && new_slot.directory.uri_contain_keypath_icons)
                 nview = 0; /* icon view */
 
+            /* Mount the directory if it's not mounted */
+            if (!new_slot.directory.file.is_mounted) {
+                tab_name = _("Connecting…");
+                loading (true);
+                
+                new_slot.directory.mount_mountable.begin ((obj,res) => {
+                    try {
+                        new_slot.directory.mount_mountable.end (res);
+                        make_view (nview, new_mwcol, new_slot);
+                    } catch (Error e) {
+                        warning ("mount_mountable failed: %s", e.message);
+                        // Reset the tab label
+                        refresh_slot_info ();
+                    }
+                });
+            } else {
+                make_view (nview, new_mwcol, new_slot);
+            }
+        }
+
+        private void make_view (int nview, Marlin.Window.Columns? new_mwcol, GOF.Window.Slot new_slot) {
+            if (slot != null && slot.directory != null && slot.directory.file.exists) {
+                slot.directory.cancel ();
+                slot.directory.track_longest_name = false;
+            }
+
+            slot = new_slot;
+            mwcol = new_mwcol;
+
             /* Setting up view_mode and its button */
             view_mode = nview;
             if (window.top_menu.view_switcher != null)
@@ -299,8 +330,10 @@
         public string? get_root_uri () {
             if (mwcol != null)
                 return mwcol.get_root_uri ();
-            else
+            else if (slot != null)
                 return slot.location.get_uri ();
+
+            return null;
         }
 
         public string? get_tip_uri () {
@@ -319,7 +352,7 @@
         }
 
         public void update_location_state (bool save_history) {
-            if (!slot.directory.file.exists)
+            if (slot == null || !slot.directory.file.exists)
                 return;
 
             if (save_history)

=== modified file 'src/View/Window.vala'
--- src/View/Window.vala	2014-07-30 02:20:58 +0000
+++ src/View/Window.vala	2014-07-31 20:18:53 +0000
@@ -382,6 +382,10 @@
                 tab.label = tab_name;
             });
 
+            content.loading.connect ((is_loading) => {
+                tab.working = is_loading;
+            });
+
             change_tab ((int)tabs.insert_tab (tab, -1));
             tabs.current = tab;
         }