From 75e57b03e990a6dd02046acc07dd1f293f4ead8e Mon Sep 17 00:00:00 2001 From: Jeremy Wootten Date: Mon, 6 Mar 2023 05:22:12 +0000 Subject: [PATCH] Fix tab label disambiguation regression (#2081) Co-authored-by: Ryan Kornheisl --- libcore/FileUtils.vala | 76 ++++++++++++++++++ libcore/tests/UtilTests/UtilTests.vala | 58 ++++++++++++++ src/View/ViewContainer.vala | 24 ++---- src/View/Window.vala | 107 ++++++++++++------------- 4 files changed, 190 insertions(+), 75 deletions(-) diff --git a/libcore/FileUtils.vala b/libcore/FileUtils.vala index 89553b72e8..c1211622e4 100644 --- a/libcore/FileUtils.vala +++ b/libcore/FileUtils.vala @@ -1234,6 +1234,82 @@ namespace Files.FileUtils { return false; } } + + // Return enough of @path to distinguish it from @conflict_path + // Currently, differences in some parts of uri are ignored, only scheme and path are used. + public string disambiguate_uri (string uri, string conflict_uri) { + string? uri_scheme, uri_userinfo, uri_host, uri_path, uri_query, uri_fragment; + string? conflict_scheme, conflict_userinfo, conflict_host, conflict_path, conflict_query, conflict_fragment; + int uri_port, conflict_port; + try { + Uri.split (uri, UriFlags.NONE, out uri_scheme, out uri_userinfo, out uri_host, out uri_port, out uri_path, out uri_query, out uri_fragment); + Uri.split (conflict_uri, UriFlags.NONE, out conflict_scheme, out conflict_userinfo, out conflict_host, out conflict_port, out conflict_path, out conflict_query, out conflict_fragment); + } catch { + return Path.get_basename (uri); + } + + var prefix = ""; + var conflict_prefix = ""; + var temp_path = uri_path; + var temp_conflict_path = conflict_path; + string temp_basename = "", temp_conflict_basename = ""; + var basename = Path.get_basename (temp_path); + + if (basename == "") { + return (uri_scheme ?? "file") + "://"; + } + + // This function should be called with an actually conflicting path but + // we deal with some unexpected values in case. + if (basename != Path.get_basename (conflict_path)) { + return basename; + } + + // Deal with same path possibly with different schemes + if (temp_path == temp_conflict_path) { + if ((uri_scheme ?? "file") == (conflict_scheme ?? "file")) { + return basename; + } else { + return uri; + } + } + + // Add parent directories until path and conflict path differ + // Protect from possible infinite loops + uint count = 0; + while (prefix == conflict_prefix && + temp_basename != Path.DIR_SEPARATOR_S && + count < 10 + ) { + var parent_temp_path = FileUtils.get_parent_path_from_path (temp_path, false); + var parent_temp_confict_path = FileUtils.get_parent_path_from_path (temp_conflict_path, false); + temp_path = parent_temp_path; + temp_conflict_path = parent_temp_confict_path; + temp_basename = Path.get_basename (parent_temp_path); + temp_conflict_basename = Path.get_basename (parent_temp_confict_path); + + if (temp_basename != Path.DIR_SEPARATOR_S) { + prefix = temp_basename + Path.DIR_SEPARATOR_S + prefix; + } else { + prefix = temp_basename + prefix; + } + + if (temp_conflict_basename != Path.DIR_SEPARATOR_S) { + conflict_prefix = temp_conflict_basename + Path.DIR_SEPARATOR_S + conflict_prefix; + } else { + conflict_prefix = temp_conflict_basename + conflict_prefix; + } + + count++; + } + + if (count > 5) { + warning ("disambiguate_oath: too many loops"); + return basename; + } + + return (prefix + basename); + } } namespace Files { diff --git a/libcore/tests/UtilTests/UtilTests.vala b/libcore/tests/UtilTests/UtilTests.vala index 99b2f33f7c..0c435ddc60 100644 --- a/libcore/tests/UtilTests/UtilTests.vala +++ b/libcore/tests/UtilTests/UtilTests.vala @@ -346,6 +346,64 @@ void add_file_utils_tests () { assert (!result.contains (Files.FileUtils.COPY_TAG)); assert (result.contains ("2")); }); + + Test.add_func ("/FileUtils/disambiguate_path/conflicting", () => { + var result = Files.FileUtils.disambiguate_uri ( + "/A/B/folder", + "/A/C/folder" + ); + assert (result == "B/folder"); + }); + + Test.add_func ("/FileUtils/disambiguate_path/different_length", () => { + var result = Files.FileUtils.disambiguate_uri ( + "/A/B/folder", + "X/A/B/folder" + ); + assert (result == "/A/B/folder"); + + result = Files.FileUtils.disambiguate_uri ( + "X/A/B/folder", + "/A/B/folder" + ); + assert (result == "X/A/B/folder"); + }); + + Test.add_func ("/FileUtils/disambiguate_path/not_conflicting", () => { + var result = Files.FileUtils.disambiguate_uri ( + "/A/B/folder", + "/A/B/folder2" + ); + assert (result == "folder"); + }); + + Test.add_func ("/FileUtils/disambiguate_path/equivalent", () => { + var result = Files.FileUtils.disambiguate_uri ( + "/A/B/folder", + "file:///A/B/folder" + ); + assert (result == "folder"); + + result = Files.FileUtils.disambiguate_uri ( + "file:///A/B/folder", + "/A/B/folder" + ); + assert (result == "folder"); + }); + + Test.add_func ("/FileUtils/disambiguate_path/different_scheme", () => { + var result = Files.FileUtils.disambiguate_uri ( + "/A/B/folder", + "smb:///A/B/folder" + ); + assert (result == "/A/B/folder"); + + result = Files.FileUtils.disambiguate_uri ( + "smb:///A/B/folder", + "/A/B/folder" + ); + assert (result == "smb:///A/B/folder"); + }); } int main (string[] args) { diff --git a/src/View/ViewContainer.vala b/src/View/ViewContainer.vala index 46c1e23515..360175acbf 100644 --- a/src/View/ViewContainer.vala +++ b/src/View/ViewContainer.vala @@ -25,17 +25,6 @@ namespace Files.View { public class ViewContainer : Gtk.Box { - private static int container_id; - - protected static int get_next_container_id () { - return ++container_id; - } - - static construct { - container_id = -1; - } - - public int id {get; construct;} public Gtk.Widget? content_item; public bool can_show_folder { get; private set; default = false; } private View.Window? _window = null; @@ -113,11 +102,6 @@ namespace Files.View { public signal void tab_name_changed (string tab_name); public signal void loading (bool is_loading); public signal void active (); - /* path-changed signal no longer used */ - - construct { - id = ViewContainer.get_next_container_id (); - } /* Initial location now set by Window.make_tab after connecting signals */ public ViewContainer (View.Window win) { @@ -189,6 +173,8 @@ namespace Files.View { } } + // Either the path or a special name or fallback if invalid + // Window will use as little as possible to distinguish tabs private string label = ""; public string tab_name { private set { @@ -358,16 +344,16 @@ namespace Files.View { } private void update_tab_name () { - string tab_name = Files.INVALID_TAB_NAME; + var tab_name = Files.INVALID_TAB_NAME; string protocol, path; - FileUtils.split_protocol_from_path (this.uri, out protocol, out path); //path is escaped + FileUtils.split_protocol_from_path (this.uri, out protocol, out path); if (path == "" || path == Path.DIR_SEPARATOR_S) { tab_name = Files.protocol_to_name (protocol); } else if (protocol == "" && path == Environment.get_home_dir ()) { tab_name = _("Home"); } else { - tab_name = Path.get_basename (Uri.unescape_string (path)); + tab_name = Uri.unescape_string (path); } this.tab_name = tab_name; diff --git a/src/View/Window.vala b/src/View/Window.vala index eabb2ce299..0aa97c0ee4 100644 --- a/src/View/Window.vala +++ b/src/View/Window.vala @@ -503,11 +503,9 @@ namespace Files.View { change_tab ((int)tabs.insert_tab (tab, -1)); tabs.current = tab; - /* Capturing ViewContainer object reference in closure prevents its proper destruction - * so capture its unique id instead */ - var id = content.id; + content.tab_name_changed.connect ((tab_name) => { - set_tab_label (check_for_tab_with_same_name (id, tab_name), tab, tab_name); + check_for_tabs_with_same_name (); // Also sets tab_label. }); content.loading.connect ((is_loading) => { @@ -566,47 +564,64 @@ namespace Files.View { return -1; } - private string check_for_tab_with_same_name (int id, string path) { - if (path == Files.INVALID_TAB_NAME) { - return path; - } - - var new_label = Path.get_basename (path); - foreach (Granite.Widgets.Tab tab in tabs.tabs) { + /** Compare every tab label with every other and resolve ambiguities **/ + private void check_for_tabs_with_same_name () { + // Take list copy so foreach clauses can be nested safely + var copy_tabs = tabs.tabs.copy (); + foreach (unowned var tab in tabs.tabs) { var content = (ViewContainer)(tab.page); - if (content.id != id) { - string content_path = content.tab_name; - string content_label = Path.get_basename (content_path); - if (tab.label == new_label) { - if (content_path != path) { - new_label = disambiguate_name (new_label, path, content_path); /*Relabel calling tab */ - if (content_label == tab.label) { - /* Also relabel conflicting tab (but not before this function finishes) */ - Idle.add_full (GLib.Priority.LOW, () => { - var unique_name = disambiguate_name (content_label, content_path, path); - set_tab_label (unique_name, tab, content_path); - return GLib.Source.REMOVE; - }); - } - } - } else if (content_label == new_label && - content_path == path && - content_label != tab.label) { - - /* Revert to short label when possible */ - Idle.add_full (GLib.Priority.LOW, () => { - set_tab_label (content_label, tab, content_path); - return GLib.Source.REMOVE; - }); + if (content.tab_name == Files.INVALID_TAB_NAME) { + set_tab_label (content.tab_name, tab, content.tab_name); + continue; + } + + var path = content.location.get_path (); + if (path == null) { // e.g. for uris like network:// + set_tab_label (content.tab_name, tab, content.tab_name); + continue; + } + var basename = Path.get_basename (path); + + // Ignore content not named from the path + if (!content.tab_name.has_suffix (basename)) { + set_tab_label (content.tab_name, tab, content.tab_name); + continue; + } + + // Tab label defaults to the basename. + set_tab_label (basename, tab, content.tab_name); + + // Compare with every other tab for same label + foreach (unowned var tab2 in copy_tabs) { + var content2 = (ViewContainer)(tab2.page); + if (content2 == content || content2.tab_name == Files.INVALID_TAB_NAME) { + continue; + } + + var path2 = content2.location.get_path (); + if (path2 == null) { // e.g. for uris like network:// + continue; + } + var basename2 = Path.get_basename (path2); + + // Ignore content not named from the path + if (!content2.tab_name.has_suffix (basename2)) { + continue; + } + + if (basename2 == basename && path2 != path) { + set_tab_label (FileUtils.disambiguate_uri (path2, path), tab2, content2.tab_name); + set_tab_label (FileUtils.disambiguate_uri (path, path2), tab, content.tab_name); } } } - return new_label; + return; } /* Just to append "as Administrator" when appropriate */ private void set_tab_label (string label, Granite.Widgets.Tab tab, string? tooltip = null) { + string lab = label; if (Files.is_admin ()) { lab += (" " + _("(as Administrator)")); @@ -626,25 +641,6 @@ namespace Files.View { } } - private string disambiguate_name (string name, string path, string conflict_path) { - string prefix = ""; - string prefix_conflict = ""; - string path_temp = path; - string conflict_path_temp = conflict_path; - - /* Add parent directories until path and conflict path differ */ - while (prefix == prefix_conflict) { - var parent_path= FileUtils.get_parent_path_from_path (path_temp); - var parent_conflict_path = FileUtils.get_parent_path_from_path (conflict_path_temp); - prefix = Path.get_basename (parent_path) + Path.DIR_SEPARATOR_S + prefix; - prefix_conflict = Path.get_basename (parent_conflict_path) + Path.DIR_SEPARATOR_S + prefix_conflict; - path_temp= parent_path; - conflict_path_temp = parent_conflict_path; - } - - return prefix + name; - } - public void bookmark_uri (string uri, string custom_name = "") { sidebar.add_favorite_uri (uri, custom_name); } @@ -653,7 +649,6 @@ namespace Files.View { return !sidebar.has_favorite_uri (uri); } - public void remove_content (ViewContainer view_container) { remove_tab (tabs.get_tab_by_widget ((Gtk.Widget)view_container)); }