Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign file switcher (a la Sublime Text) #2590

Merged
merged 61 commits into from
Aug 20, 2015
Merged

Redesign file switcher (a la Sublime Text) #2590

merged 61 commits into from
Aug 20, 2015

Conversation

goanpeca
Copy link
Member

@goanpeca goanpeca commented Aug 3, 2015

Fixes #2340
Fixes #2141


Description

Continues where PR #2343 left.

The file switcher now looks like this:

File list

switcher-symbol

Symbol list

switcher-symbol2

Original Features

  • matching text is highlighted in bold
  • up/down keys can be used to navigate list when text field has focus
  • navigating the list changes the active tab in the editor, but will revert if dialog is canceled
  • text field has focus when opened
  • the dialog is more compact and is positioned at the top, centre, of the editor
  • global context shortcut - I couldn't work out how to change this and keep Spyder happy. I'd suggest Ctrl-P as default, but the main thing is to make it global rather than Editor-specific.
  • path info in the list in addition to file names (I think this is relatively easy now that there's an HTMLdelegate involved, but I could be wrong).
  • goto line - there is currently a separate dialog for this, but it might be nice to combine the two as in Sublime.

New Features

  • Code leanup and refactoring
  • fuzzy matching
  • dialog resize based on contents
  • goto symbol - this would basically use the same data as the Outline tool. Again, this is something provided by Sublime.
  • Fix line number displayed
  • Restore original line numbers on cancel when looking for symbols
  • Fill-in missing docstring

Todo

  • Fix py3 compatibility
  • Fix bug with cells in outline data

For another PR

  • Improve contrast on OSX
  • Fix Oxygen display in Kubuntu
  • Remove icons?
  • Increase icon size?
  • Add parameters?
  • Remove def, class ?

@goanpeca goanpeca mentioned this pull request Aug 3, 2015
11 tasks
@goanpeca goanpeca added this to the v3.0 milestone Aug 3, 2015
@goanpeca goanpeca self-assigned this Aug 3, 2015
@ccordoba12
Copy link
Member

This needs a rebase now :-) Also waiting badly for it (just saying ;-)

@ccordoba12 ccordoba12 changed the title File Switcher Continuation... Redesign file switcher (a la Sublime Text) Aug 8, 2015
@goanpeca
Copy link
Member Author

goanpeca commented Aug 8, 2015

I want to implement symbol lookup, it is not that hard from what is already in place... but to have perhaps some testing, we could leave symbol lookup for another one?

@ccordoba12
Copy link
Member

I think there's a lot here, so yes, symbol lookup can wait for another PR.

@ccordoba12
Copy link
Member

One last thing: is the new file switcher centered on the Editor when shown? Or on the entire application?

@goanpeca
Copy link
Member Author

goanpeca commented Aug 8, 2015

At this moment, editor

@ccordoba12
Copy link
Member

Mmmh, I think it'd look better to center it on the entire application (like the "Go to line" dialog). But I'm not sure

@blink1073
Copy link
Contributor

For what its worth, Sublime centers on the active editor pane.

@ccordoba12
Copy link
Member

I thought that was the case. The good thing about Sublime is that (by default) it only has two panes: a (small) file explorer to the left and an editor to the right.

But since our editor takes half of the screen, centering the new switcher just on that area seems a bit awkward to me (specially on small screens).

@goanpeca, what do you think?

@blink1073
Copy link
Contributor

When you have a two-column view open in Sublime, it is still centered on the active column. This feels more natural as it lets you know where you are taking action.

@goanpeca
Copy link
Member Author

goanpeca commented Aug 8, 2015

I agree with blinkie

@ccordoba12
Copy link
Member

Ok, let's leave it like that for now :-) We can revisit this decision later.

Then, please rebase to give this a final review ;-)

@goanpeca
Copy link
Member Author

So, it was so close that I wanted to give it a try... here is symbol search :-), round 1

file-switcher

@goanpeca
Copy link
Member Author

SPOILER: there seems to be a bug with the Oxygen theme of Kubuntu which does not display the list items at all , it seems to work fine on the rest though

@goanpeca
Copy link
Member Author

@d1manson (if you are curious...)

@ccordoba12
Copy link
Member

What do you mean by the spoiler? Could you post a screenshot please?

@goanpeca
Copy link
Member Author

@blink1073 can you give it a test drive?

def update_filelistdialog(self):
if not self.tabs.count():
return
e = self
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this. Just use self below.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok (leftover code ;- ) not from me)

@ccordoba12
Copy link
Member

I really don't have time to review the new FileSwitcher class. I think it was reviewed throughly by @goanpeca and @Nodd in the previous PR, so I have no problem with that.

So let's wait for @blink1073 approval, after a little bit of live testing (given that he's the one who has used Sublime the most :-)

@blink1073
Copy link
Contributor

Uh oh:

Traceback (most recent call last):
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/spyder.py", line 2471, in call_file_switcher
    self.editor.get_current_editorstack().open_fileswitcher_dlg()
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/editor.py", line 539, in open_fileswitcher_dlg
    self.fileswitcher_dlg = FileSwitcher(self, self.tabs, self.data)
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/file_switcher.py", line 277, in __init__
    self.setup()
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/file_switcher.py", line 573, in setup
    self.setup_file_list(filter_text, current_path)
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/file_switcher.py", line 459, in setup_file_list
    short_paths = shorten_paths(self.paths, self.save_status)
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/file_switcher.py", line 183, in shorten_paths
    recurse_level({i: pl for i, pl in enumerate(path_list) if pl})
  File "/Users/ssilvester/workspace/spyder-ide/spyderlib/widgets/file_switcher.py", line 125, in recurse_level
    sample_toks = level_idx.values()[0]
TypeError: 'dict_values' object does not support indexing

@goanpeca
Copy link
Member Author

What the f*ck?

Just by pressing Ctrl+P ? or when doing something? your setup?

@goanpeca
Copy link
Member Author

I cannot reproduce this :-( ...

@blink1073
Copy link
Contributor

Are you using Python 2? Here's a diff that makes it work for me on 3.4:

diff --git a/spyderlib/widgets/file_switcher.py b/spyderlib/widgets/file_switcher.py
index c595127..6eab885 100644
--- a/spyderlib/widgets/file_switcher.py
+++ b/spyderlib/widgets/file_switcher.py
@@ -9,6 +9,7 @@ from __future__ import print_function

 import os
 import os.path as osp
+import six

 from spyderlib.qt.QtGui import (QDialog, QHBoxLayout, QIcon, QLabel, QLineEdit,
                                 QListWidget, QListWidgetItem, QRegExpValidator,
@@ -122,7 +123,7 @@ def shorten_paths(path_list, is_unsaved):

         # Firstly, find the longest common prefix for all in the level
         # s = len of longest common prefix
-        sample_toks = level_idx.values()[0]
+        sample_toks = list(level_idx.values())[0]
         if not sample_toks:
             s = 0
         else:
@@ -157,7 +158,7 @@ def shorten_paths(path_list, is_unsaved):
                         group = prospective_group
                     break
                 # Only keep going if all n still match on the kth token
-                _, sample_toks = next(group.iteritems())
+                _, sample_toks = next(six.iteritems(group))
                 prospective_group = {idx: toks for idx, toks
                                      in group.items()
                                      if toks[k] == sample_toks[k]}
@@ -166,7 +167,7 @@ def shorten_paths(path_list, is_unsaved):
                     k += 1
                 else:
                     break
-            _, sample_toks = next(group.iteritems())
+            _, sample_toks = next(six.iteritems(group))
             if k == 0:
                 short_form = ''
             elif k == 1:

@blink1073
Copy link
Contributor

Also, there is poor contrast:

screen shot 2015-08-16 at 11 26 56 am

@blink1073
Copy link
Contributor

Other than those two issues, I like it 😜

@goanpeca
Copy link
Member Author

Sh#@, I forgot to test on py3.... will make the fix thanks!

@goanpeca
Copy link
Member Author

Ok, maybe we need some custom css rules for OSX, it does look funky....

@goanpeca
Copy link
Member Author

In spyder we do not use six... we have our own p3compat.py, so I guess I will add the iteritems fix there to keep it consistent... (although I would prefer to use six PLUS any specific py3compat Qt related stuff....)

@blink1073
Copy link
Contributor

Yeah at some point any large project ends up using six...

@goanpeca
Copy link
Member Author

@blink1073 can you check again?

I will look the contrast issue in a mac this week (I have a mac for a week 😸).

@goanpeca
Copy link
Member Author

@ccordoba12 I updated the description with some screenshots...

But it would be nice if you can test it yourself directly

for key in oedata:
val = oedata[key]
if val and val != 'found_cell_separators':
if val.is_class_or_function():
Copy link
Contributor

Choose a reason for hiding this comment

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

When the file has cells I get the following error here:
AttributeError: 'bool' object has no attribute 'is_class_or_function'
If you simply create an empty file and add a #%% line you get this error.

@d1manson
Copy link
Contributor

@goanpeca thanks for taking over on this! Symbols lookup is a massive help...if it's not too difficult could you show something a bit more like the tree "path" you see in the outline widget, i.e. my_method\n[line 99, in my_class]. Simply trimming whitespace would be nice I think.

@goanpeca
Copy link
Member Author

@d1manson, thanks for the heads up, I will see if we can get a more clean display, this is the first iteration, which I think (when I get the cells error fixed) is good for Beta2!, we need people testing this.

Cheers

goanpeca added a commit that referenced this pull request Aug 20, 2015
Redesign file switcher (a la Sublime Text)
@goanpeca goanpeca merged commit b6532f0 into spyder-ide:master Aug 20, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants