Skip to content
This repository has been archived by the owner on Jun 25, 2020. It is now read-only.

notebook tabs shows first uncommon subdirectory if filename is the same #365

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

zalox
Copy link
Member

@zalox zalox commented Feb 11, 2018

No description provided.

@eidheim
Copy link
Member

eidheim commented Feb 12, 2018

I had a look at this, and there were some minor issues:

  • The labels got the /.../-prefix even though only the filenames were equal
  • When closing a tab with a path prefix, the tabs did not get updated.
  • update_tab_label should be called when opening a new buffer, just in case the buffer is empty and on_change is never called.
  • Maybe slightly more complex code than needed.

I did a fix proposal here that you can look at:

diff --git a/src/notebook.cc b/src/notebook.cc
index 0cbdf75..4f4c654 100644
--- a/src/notebook.cc
+++ b/src/notebook.cc
@@ -10,14 +10,6 @@
 #include "source_language_protocol.h"
 #include "gtksourceview-3.0/gtksourceview/gtksourcemap.h"
 
-template <class firstIt, class secondIt>
-  auto mismatch(firstIt first1, firstIt last1, secondIt first2,secondIt last2){
-  while (first1 != last1 && first2 != last2 && *first1 == *first2) {
-      ++first1, ++first2;
-  }
-  return std::make_pair(*first1, *first2);
-};
-
 Notebook::TabLabel::TabLabel(const boost::filesystem::path &path, std::function<void()> on_close) {
   set_can_focus(false);
   set_tooltip_text(path.string());
@@ -26,7 +18,6 @@ Notebook::TabLabel::TabLabel(const boost::filesystem::path &path, std::function<
   auto hbox=Gtk::manage(new Gtk::Box());
   
   hbox->set_can_focus(false);
-  label.set_text(path.filename().string()+' ');
   label.set_can_focus(false);
   button->set_image_from_icon_name("window-close-symbolic", Gtk::ICON_SIZE_MENU);
   button->set_can_focus(false);
@@ -203,12 +194,10 @@ void Notebook::open(const boost::filesystem::path &file_path_, size_t notebook_i
     }
   };
   source_views.back()->update_tab_label=[this](Source::View *view) {
-    const auto update_label = [&](const boost::filesystem::path &path, size_t index){
+    const auto update_label = [this](size_t index, const std::string &prepend) {
       const auto current_view = source_views[index];
-      std::string title = current_view->file_path.filename().string();
-      if(!path.empty())
-        title = path.string() + "/.../" + title;
-      if(view->get_buffer()->get_modified())
+      auto title = prepend+current_view->file_path.filename().string();
+      if(current_view->get_buffer()->get_modified())
         title+='*';
       else
         title+=' ';
@@ -216,32 +205,32 @@ void Notebook::open(const boost::filesystem::path &file_path_, size_t notebook_i
       tab_label->label.set_text(title);
       tab_label->set_tooltip_text(filesystem::get_short_path(current_view->file_path).string());
     };
-    std::unordered_map<int, std::pair<boost::filesystem::path, boost::filesystem::path>> uncommon_paths;
     const auto file_name=view->file_path.filename();
     size_t current_view_index = 0;
+    std::string prepend_current_view;
     for(size_t c=0;c<size();++c) {
       if(source_views[c]==view) {
         current_view_index = c;
         continue;
       }
       if (source_views[c]->file_path.filename() == file_name) {
-        auto const first_uncommon_path = mismatch(
-          view->file_path.parent_path().rbegin(),
-          view->file_path.parent_path().rend(),
-          source_views[c]->file_path.parent_path().rbegin(),
-          source_views[c]->file_path.parent_path().rend()
-        );
-        uncommon_paths.emplace(std::make_pair(c, first_uncommon_path));
+        int diff=0;
+        auto parent_path1=view->file_path.parent_path();
+        auto parent_path2=source_views[c]->file_path.parent_path();
+        auto it1=parent_path1.rbegin();
+        auto it2=parent_path2.rbegin();
+        while (it1 != parent_path1.rend() &&
+               it2 != parent_path2.rend() && *it1 == *it2) {
+          ++it1;
+          ++it2;
+          ++diff;
+        }
+        if(prepend_current_view.empty())
+          prepend_current_view=it1->string()+(diff>0?"/.../":"/");
+        update_label(c, it2->string()+(diff>0?"/.../":"/"));
       }
     }
-    for (const auto &uncommon_path : uncommon_paths) {
-      update_label(uncommon_path.second.second, uncommon_path.first);
-    }
-    if (uncommon_paths.empty()) {
-      update_label("", current_view_index);
-    } else {
-      update_label(uncommon_paths.begin()->second.first, current_view_index);
-    }
+    update_label(current_view_index, prepend_current_view);
     update_status(view);
   };
   source_views.back()->update_status_diagnostics=[this](Source::View* view) {
@@ -327,6 +316,8 @@ void Notebook::open(const boost::filesystem::path &file_path_, size_t notebook_i
     if(index!=static_cast<size_t>(-1))
       close(index);
   }));
+  if(source_views.back()->update_tab_label)
+    source_views.back()->update_tab_label(source_views.back());
   
   //Add star on tab label when the page is not saved:
   source_view->get_buffer()->signal_modified_changed().connect([this, source_view]() {
@@ -540,6 +531,11 @@ bool Notebook::close(size_t index) {
     hboxes.erase(hboxes.begin()+index);
     tab_labels.erase(tab_labels.begin()+index);
   }
+  
+  for(auto view: get_views()) { // Update all view tabs in case one clicks cross to close a buffer
+    if(view->update_tab_label)
+      view->update_tab_label(view);
+  }
   return true;
 }

@zalox zalox force-pushed the notebook_tabs branch 2 times, most recently from 7e962d4 to 3420b50 Compare February 13, 2018 22:18
src/notebook.cc Outdated
tab_label->set_tooltip_text(filesystem::get_short_path(view->file_path).string());
update_status(view);
return;
update_label(c, prepend_current_view);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepend_current_view might not have been set here yet.

src/notebook.cc Outdated
while (it1 != parent_path1.rend() && it2 != parent_path2.rend() && *it1 == *it2) {
++it1;
++it2;
++diff;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long indentation, also feel free to keep these at one line like you did before. I changed it by accident to multiple lines while testing.

@eidheim
Copy link
Member

eidheim commented Feb 24, 2018

Here are some suggestion for changes that also fixes double / when root path is shown:

diff --git a/src/notebook.cc b/src/notebook.cc
index c4dd4c1..4d7c5ef 100644
--- a/src/notebook.cc
+++ b/src/notebook.cc
@@ -209,6 +209,7 @@ void Notebook::open(const boost::filesystem::path &file_path_, size_t notebook_i
     const auto file_name = view->file_path.filename();
     std::string prepend_current_view;
     size_t current_view_index = 0;
+    // Extends tab label with a parent folder if two tab labels show the same filename
     for (size_t c = 0; c < size(); ++c) {
       if (source_views[c] == view) {
         current_view_index = c;
@@ -218,16 +219,26 @@ void Notebook::open(const boost::filesystem::path &file_path_, size_t notebook_i
         int diff = 0;
         const auto parent_path1 = view->file_path.parent_path();
         const auto parent_path2 = source_views[c]->file_path.parent_path();
-        auto it1 = parent_path1.rbegin();
-        auto it2 = parent_path2.rbegin();
-        while (it1 != parent_path1.rend() && it2 != parent_path2.rend() && *it1 == *it2) {
+        // Ubuntu 16.04 workaround:
+        auto it1 = std::reverse_iterator<boost::filesystem::path::iterator>(parent_path1.end());
+        auto it1_end = std::reverse_iterator<boost::filesystem::path::iterator>(parent_path1.begin());
+        auto it2 = std::reverse_iterator<boost::filesystem::path::iterator>(parent_path2.end());
+        auto it2_end = std::reverse_iterator<boost::filesystem::path::iterator>(parent_path2.begin());
+        while (it1 != it1_end && it2 != it2_end && *it1 == *it2) {
           ++it1;
           ++it2;
           ++diff;
         }
-        if (prepend_current_view.empty())
-          prepend_current_view = it1->string() + (diff > 0 ? "/.../" : "/");
-        update_label(c, it2->string() + (diff > 0 ? "/.../" : "/"));
+        if (prepend_current_view.empty()) {
+          auto it1_str = it1->string();
+          if(it1_str=="/") // Fix for root directory
+            it1_str="";
+          prepend_current_view = it1_str + (diff > 0 ? "/.../" : "/");
+        }
+        auto it2_str = it2->string();
+        if(it2_str=="/") // Fix for root directory
+          it2_str="";
+        update_label(c, it2_str + (diff > 0 ? "/.../" : "/"));
       }
     }
     update_label(current_view_index, prepend_current_view);

@eidheim
Copy link
Member

eidheim commented Feb 24, 2018

One problem though is that if you have a directory structure like this:

  • a/
    • b/
      • 1.txt
    • 1.txt
  • b/
    • 1.txt

and you open all the 1.txt files, then if you modify them, their labels will change depending on which file you modify.

@zalox
Copy link
Member Author

zalox commented Feb 28, 2018

I will have time to look at this this today or this weekend. Feel free to fix it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants