PR: Add disambiguation functionality for Editor tabs #3715

dalthviz · 2016-11-20T19:49:32Z

Fixes #3160
Fixes #2907

Edit: Include reference to related issue about tabs titles

dalthviz · 2016-11-20T19:56:26Z

Now the editor tabs show a title that disambiguates files with the same name:

ccordoba12 · 2016-11-20T21:40:23Z

This is so nice!! I wanted this for a long time!

@goanpeca, what do you think about the design? We followed the same one as Sublime :-)

ccordoba12 · 2016-11-23T17:24:57Z

@dalthviz, I'd like to see this one finished (or at least ready for reviewing) before this Friday :-)

…c to compress long paths

ccordoba12 · 2016-11-24T13:36:31Z

@dalthviz, you need to merge your PR with master to fix the errors in Travis.

ccordoba12 · 2016-11-28T13:36:34Z

spyder/widgets/editor.py

+   from itertools import izip
+except ImportError:
+    # Python 3
+    izip = zip


Please move this import to py3compat

ccordoba12 · 2016-11-28T13:41:32Z

spyder/widgets/editor.py

+        """Get tab title without ambiguation."""
+        finfo = self.data[index]
+        fname = osp.basename(finfo.filename)
+        for findex, file in enumerate(self.data):


file is a reserved word in Python 2, so you need to use a different name here.

ccordoba12 · 2016-11-28T13:45:12Z

spyder/widgets/editor.py

+                differ_path_length = len(differ_path)
+                if (differ_path_length > 20):
+                    path_components = self.path_components(differ_path)
+                    last_component_index = len(path_components) -1


There is a missing space here after the minus sign:

len(path_components) - 1

ccordoba12 · 2016-11-28T13:51:24Z

spyder/widgets/editor.py

+                break
+        return fname
+
+    def path_components(self, path):


Let's move this function to utils/sourcecode.py and add a test for it. For that you need to move the tests at the end of that file to the tests folder in utils, in a file called test_sourcecode.py.

ccordoba12 · 2016-11-28T13:53:39Z

spyder/widgets/editor.py

+        """Return the individual components of the given file path
+        string (for the local operating system). Taked from 
+        http://stackoverflow.com/questions/21498939/how-to-circumvent
+        -the-fallacy-of-pythons-os-path-commonprefix."""


This docstring should be formatted like this

""" Return the individual components of a given file path string (for the local operating system). Taken from http://stackoverflow.com/q/21498939/438386 """

That link is the short link to the SO question you referenced. You can get those links by pressing the share links that are placed below every SO question or answer.

ccordoba12 · 2016-11-28T13:55:26Z

spyder/widgets/editor.py

+        components.reverse()  # First component first 
+        return components
+
+    def differ_prefix(self, path_components0, path_components1):


Let's also move this function to utils/sourcecode.py and a test for it.

differ is a verb. Did you mean different here? Or maybe differentiate?

ccordoba12 · 2016-11-28T13:57:29Z

spyder/widgets/editor.py

+    def differ_prefix(self, path_components0, path_components1):
+        """Return the differ prefix of the given two iterables. Based
+        on longest_prefix in http://stackoverflow.com/questions/21498939/how-to-
+        circumvent-the-fallacy-of-pythons-os-path-commonprefix."""


Please reformat this docstring as I said above for path_components.

ccordoba12 · 2016-11-29T19:24:22Z

Some of the tests you introduced are failing on Travis (i.e. Linux). Please review them.

…validation

ccordoba12 · 2016-12-01T02:14:59Z

spyder/py3compat.py

@@ -63,6 +63,7 @@
    import thread as _thread
    import repr as reprlib
    import Queue
+    from itertools import izip


Let's change this to

from itertools import izip as zip

ccordoba12 · 2016-12-01T02:15:23Z

spyder/py3compat.py

@@ -78,6 +79,7 @@
    import _thread
    import reprlib
    import queue as Queue
+    izip = zip


With my previous suggestion, there's no need for this line now ;-)

ccordoba12 · 2016-12-01T03:00:06Z

spyder/utils/sourcecode.py

@@ -9,7 +9,10 @@
 """

 import re
-
+import os
+from spyder.py3compat import PY2


Please add a blank line before this one, to separate standard library imports from Spyder ones.

ccordoba12 · 2016-12-01T23:56:55Z

spyder/utils/sourcecode.py

+
+def differentiate_prefix(path_components0, path_components1):
+    """
+    Return the differ prefix of the given two iterables. 


Please change this to

Return the differentiated prefix ...

ccordoba12 · 2016-12-01T23:58:19Z

spyder/utils/sourcecode.py

+            common_elmt = elmt0
+        longest_prefix.append(elmt0)
+    file_name_length = len(path_components0[len(path_components0)-1])
+    path_0 = os.path.join(*path_components0)[:-file_name_length-1]


Add spaces around the minus sign in these two lines, like this

file_name_length = len(path_components0[len(path_components0) - 1])

ccordoba12 · 2016-12-01T23:58:39Z

spyder/utils/sourcecode.py

+        longest_prefix.append(elmt0)
+    file_name_length = len(path_components0[len(path_components0)-1])
+    path_0 = os.path.join(*path_components0)[:-file_name_length-1]
+    if(len(longest_prefix)>3):


Please add spaces around >

ccordoba12 · 2016-12-01T23:58:52Z

spyder/utils/sourcecode.py

+    path_0 = os.path.join(*path_components0)[:-file_name_length-1]
+    if(len(longest_prefix)>3):
+        longest_path_prefix = os.path.join(*longest_prefix)
+        length_to_delete = len(longest_path_prefix)-len(common_elmt) 


Spaces around -

ccordoba12 · 2016-12-02T00:00:57Z

spyder/utils/tests/test_sourcecode.py

+from spyder.utils import sourcecode
+
+
+def test_get_primary_at(tmpdir, qtbot):


None of these tests seems to require tmpdir nor qtbot, so please remove them

ccordoba12 · 2016-12-02T00:04:07Z

spyder/utils/tests/test_sourcecode.py

+def test_get_identifiers(tmpdir, qtbot):
+    code = 'import functools\nfunctools.partial'
+    assert set(sourcecode.get_identifiers(code)) == set(['import', 'functools',
+                                              'functools.partial'])


Please align this line like this

.. == set(['import', 'functools', 'functools.partial'])

ccordoba12 · 2016-12-05T01:00:59Z

That's what I was expecting: almost the same information is present in the tab's title and flie switcher's second row of each entry.

So we need to remove from the tabs title the disambiguation text when showing it in the file switcher. Please also do that as part of this PR :-)

dalthviz · 2016-12-05T01:37:23Z

This is the way the file switcher and titles in tabs will be shown:

…path.

…th for the file is his path.

… subdirectory.

dalthviz · 2016-12-11T03:09:40Z

A preview in Windows:

A preview in Linux:

ccordoba12 · 2016-12-11T20:46:24Z

Thanks @dalthviz for keep working on this! There's a simple style fix reported in CircleCI.

Also, please don't forget to merge with 3.x to fix the problems we were seeing yesterday in CircleCI :-)

dalthviz · 2016-12-12T00:10:12Z

spyder/widgets/editor.py

@@ -965,11 +965,36 @@ def __modified_readonly_title(self, title, is_modified, is_readonly):
            title = "(%s)" % title
        return title

-    def get_tab_text(self, filename, is_modified=None, is_readonly=None):
+    def get_tab_text(self, index, is_modified=None, is_readonly=None):


Hi @ccordoba12, CircleCi (pydocstyle) is giving me an error of docstring (D400), however the line that is pointing out (968 in widgets/editor.py) can't finish in a period (is the signature of a method) . In this case what can I do to fix the code style issue?

I fixed this in the latest 3.x branch. Please merge it here again :-)

I finally understood what's this error is all about: you just need to add a dot to the docstring below this line, so that it reads like this

"""Return tab title."""

… Added tests for the new methods.

ccordoba12 · 2016-12-14T03:28:12Z

spyder/utils/sourcecode.py

+            path_0 = '/'
+    return path_0
+
+def get_file_title(data, index):


The Python console doesn't have the same structure for tabs as the Editor, so this is not useful.

So you need to make this function to receive a filename instead of data and index, and call it differently in the Editor.

ccordoba12 · 2016-12-14T03:29:42Z

spyder/utils/sourcecode.py

+        fname = fname + " - " + diff_path    
+    return fname
+
+def get_same_name_files(data, fname):


Please also make this function independent of data (or move it to the Editor :-)

ccordoba12 · 2016-12-14T03:30:10Z

spyder/utils/sourcecode.py

+                shortest_path = path_elmts
+        return os.path.join(*shortest_path)
+
+


Please remove these spurious blank lines

…n the tests accordingly.

ccordoba12 · 2016-12-14T15:04:40Z

This is great!! The use case we talked about is working now the same as in Sublime

But there's some work to do for this one:

In this case, your algorithm is not detecting carlos as the parent of widgets/editor.py and plugins/editor.py.

Please fix it :-)

… shrinking.

ccordoba12 · 2016-12-17T20:01:01Z

I left one last comment to fix the error reported by pydocstyle, and then I'll merge :-)

ccordoba12 · 2016-12-17T20:35:57Z

All green now!! Thanks a lot @dalthviz!

This is a great addition :-)

Fixes #3160 Fixes #2907

Added disambiguation functionality for Editor tabs

e99a311

ccordoba12 added this to the v3.1 milestone Nov 20, 2016

Enhanced logic to generate titles for tabs. Added validation and logi…

69ced28

…c to compress long paths

Merge remote-tracking branch 'upstream/3.x' into fixes_issue_3160

5f62542

ccordoba12 reviewed Nov 28, 2016

View reviewed changes

Changes following the review. Added test_sourcecode.py

c65c066

dalthviz added 2 commits November 30, 2016 20:52

Changes in the test for sourcecode.py to include an operative system …

f302a7b

…validation

Changes in the test for sourcecode.py to include an operative system …

42a4052

…validation

ccordoba12 reviewed Dec 1, 2016

View reviewed changes

Changes in the way to import izip

c9d4d38

ccordoba12 reviewed Dec 1, 2016

View reviewed changes

Added blank line to separate standard library imports from Spyder ones

4295fa8

ccordoba12 reviewed Dec 1, 2016

View reviewed changes

ccordoba12 reviewed Dec 2, 2016

View reviewed changes

Changes tabs titles in the file switcher to only show the file name

81f98ba

dalthviz added 7 commits December 5, 2016 20:09

Changes for not take into account the last common directory from the …

f4d596f

…path.

Improved validation for taking into account when the common prefix pa…

606e4d0

…th for the file is his path.

Fixed problem when comparing with root directory.

2cc4574

Merge for test.

3aea355

Fixed code style problems.

75ddace

Added case for comparison between file in root directory and one in a…

7cbbdd1

… subdirectory.

Added support for linux.

2d9acbe

Merge for tests with CircleCI.

1473091

dalthviz commented Dec 12, 2016

View reviewed changes

Improvements in the logic. Pass all the logic to utils/sourcecode.py.…

858414e

… Added tests for the new methods.

ccordoba12 reviewed Dec 14, 2016

View reviewed changes

dalthviz added 2 commits December 13, 2016 23:44

Change of data structure from data of the editor to a list. Changes i…

b8b8b7a

…n the tests accordingly.

Merge for tests.

a4d1c73

dalthviz added 2 commits December 14, 2016 12:23

Added validation to detect common parent directory when applying path…

4e0bdb0

… shrinking.

Merge for tests.

92a7b7a

ccordoba12 changed the title ~~PR: Added disambiguation functionality for Editor tabs~~ PR: Add disambiguation functionality for Editor tabs Dec 17, 2016

Fixes pydocstyle D400.

0163546

ccordoba12 merged commit 1432024 into spyder-ide:3.x Dec 17, 2016

ccordoba12 added a commit that referenced this pull request Dec 17, 2016

Merge from 3.x: PR #3715

9ba2932

Fixes #3160 Fixes #2907

goanpeca assigned dalthviz Jan 13, 2017

		from spyder.utils import sourcecode


		def test_get_primary_at(tmpdir, qtbot):

		shortest_path = path_elmts
		return os.path.join(*shortest_path)

PR: Add disambiguation functionality for Editor tabs #3715

PR: Add disambiguation functionality for Editor tabs #3715

Conversation

dalthviz commented Nov 20, 2016 • edited Loading

dalthviz commented Nov 20, 2016

ccordoba12 commented Nov 20, 2016

ccordoba12 commented Nov 23, 2016

ccordoba12 commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Dec 5, 2016

dalthviz commented Dec 5, 2016

dalthviz commented Dec 11, 2016

ccordoba12 commented Dec 11, 2016

Choose a reason for hiding this comment

ccordoba12 Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

ccordoba12 commented Dec 14, 2016

ccordoba12 commented Dec 17, 2016 • edited Loading

ccordoba12 commented Dec 17, 2016

dalthviz commented Nov 20, 2016 •

edited

Loading

ccordoba12 commented Nov 29, 2016 •

edited

Loading

ccordoba12 Dec 14, 2016 •

edited

Loading

ccordoba12 Dec 14, 2016 •

edited

Loading

ccordoba12 commented Dec 17, 2016 •

edited

Loading