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

PR: Add a tab switcher dialog to the Editor #4302

Merged
merged 11 commits into from
Apr 20, 2017
6 changes: 3 additions & 3 deletions spyder/config/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -405,8 +405,8 @@
'editor/conditional breakpoint': 'Shift+F12',
'editor/run selection': "F9",
'editor/go to line': 'Ctrl+L',
'editor/go to previous file': 'Ctrl+Tab',
'editor/go to next file': 'Ctrl+Shift+Tab',
'editor/go to previous file': 'Ctrl+Shift+Tab',
'editor/go to next file': 'Ctrl+Tab',
'editor/new file': "Ctrl+N",
'editor/open last closed':"Ctrl+Shift+T",
'editor/open file': "Ctrl+O",
Expand Down Expand Up @@ -658,7 +658,7 @@
# or if you want to *rename* options, then you need to do a MAJOR update in
# version, e.g. from 3.0.0 to 4.0.0
# 3. You don't need to touch this value if you're just adding a new option
CONF_VERSION = '33.0.0'
CONF_VERSION = '33.1.0'

# Main configuration instance
try:
Expand Down
120 changes: 97 additions & 23 deletions spyder/widgets/editor.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
from qtpy.QtGui import QFont
from qtpy.QtWidgets import (QAction, QApplication, QHBoxLayout, QMainWindow,
QMessageBox, QMenu, QSplitter, QVBoxLayout,
QWidget)
QWidget, QListWidget, QListWidgetItem)

# Local imports
from spyder.config.base import _, DEBUG, STDERR, STDOUT
Expand Down Expand Up @@ -274,6 +274,70 @@ def breakpoints_changed(self):
self.save_breakpoints.emit(self.filename, repr(breakpoints))


class TabSwitcherWidget(QListWidget):
"""Show tabs in mru order and change between them."""
Copy link
Member

Choose a reason for hiding this comment

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

one space here

def __init__(self, parent, stack_history, tabs):
QListWidget.__init__(self, parent)
self.setWindowFlags(Qt.SubWindow | Qt.FramelessWindowHint)

self.editor = parent
self.stack_history = stack_history
self.tabs = tabs

self.setSelectionMode(QListWidget.SingleSelection)
self.itemActivated.connect(self.item_selected)

self.id_list = []
self.load_data()
size = CONF.get('main', 'completion/size')
Copy link
Member Author

Choose a reason for hiding this comment

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

I reused completion widget size widget, is that ok? should I add a setting? or leave a fixed size

Copy link
Member

@ccordoba12 ccordoba12 Mar 28, 2017

Choose a reason for hiding this comment

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

What if you use the same size and positioning of the file switcher?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think It's better in the center of the editor (I take inspiration from Kate), at the center you will have to move less the eyes than if it's at the top.

Or maybe something like 1/4(editor height) from top?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to have both widgets (the file switcher and this one) placed at the same location to avoid breaking expectations from users (i.e. where to expect each dialog in the screen). Since the file switcher came first, this one has to follow it.

Copy link
Member

Choose a reason for hiding this comment

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

Besides, that's what VSCode also does, i.e. place both widgets at the same location.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, on the top is better for consistency. But we can evaluate the UX of all those dialogs in a separate issue

self.resize(*size)
self.set_dialog_position()
self.setCurrentRow(0)

def load_data(self):
"""Fill ListWidget with the tabs texts.

Add elements in inverse order of stack_history.
"""
# Fill a list to track ids and corresponding tab index
self.id_list = [id(self.tabs.widget(i))
for i in range(self.tabs.count())]

for index in self.stack_history[::-1]:
text = self.tabs.tabText(self.id_list.index(index))
item = QListWidgetItem(ima.icon('FileIcon'), text)
self.addItem(item)
Copy link
Member

Choose a reason for hiding this comment

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

As discussed @rlaverde lets move all the stack history logic into a separate class (StackHistory)


def item_selected(self, item=None):
"""Change to the selected document and hide this widget."""
if item is None:
item = self.currentItem()

# stack history is in inverse order
id_ = self.stack_history[-(self.currentRow()+1)]

index = self.id_list.index(id_)
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove this logic and put inside the StackHistory widget

self.editor.set_stack_index(index)
self.editor.current_changed(index)
self.hide()

def select_row(self, steps):
"""Move selected row a number of steps.

Iterates in a cyclic behaviour.
"""
row = (self.currentRow() + steps) % self.count()
self.setCurrentRow(row)

def set_dialog_position(self):
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way we can generalize this method, so that it can be used by the fileswitcher and this one, and any other pop up dialog we use?

Maybe have a base PopUpDialog class that handles this based on the parent?

Copy link
Member Author

@rlaverde rlaverde Apr 17, 2017

Choose a reason for hiding this comment

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

hmm yes, maybe reusing some of the Modes logic introduced with editor panels

class PopUpDialog(QWidget):
    ...
    def set_dialog_position(self, top, left):

top and left will be numbers from 0 to 1 than indicate the position i.e.:

(0.5, 0.5) will be the center
(0, 0.5) will be the top-center

Copy link
Member

Choose a reason for hiding this comment

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

Yeah something like that would be vey useful... probably out of the scope of this PR, but you could do it afterwards? please create an issue for it :-)

"""Positions the tab switcher in the center of the editor."""
parent = self.parent()
left = parent.geometry().width()/2 - self.width()/2
top = parent.geometry().height()/2 - self.height()/2

self.move(left, top + self.tabs.tabBar().geometry().height())


class EditorStack(QWidget):
reset_statusbar = Signal()
readonly_changed = Signal(bool)
Expand Down Expand Up @@ -335,6 +399,7 @@ def __init__(self, parent, actions):
# self.previous_btn = None
# self.next_btn = None
self.tabs = None
self.tabs_switcher = None

self.stack_history = []

Expand Down Expand Up @@ -445,9 +510,10 @@ def create_shortcuts(self):
parent=self)
gotoline = config_shortcut(self.go_to_line, context='Editor',
name='Go to line', parent=self)
tab = config_shortcut(self.go_to_previous_file, context='Editor',
tab = config_shortcut(lambda: self.tab_navigation_mru(forward=False),
context='Editor',
name='Go to previous file', parent=self)
tabshift = config_shortcut(self.go_to_next_file, context='Editor',
tabshift = config_shortcut(self.tab_navigation_mru, context='Editor',
name='Go to next file', parent=self)
run_selection = config_shortcut(self.run_selection, context='Editor',
name='Run selection', parent=self)
Expand Down Expand Up @@ -1492,27 +1558,23 @@ def _get_previous_file_index(self):
if id(self.tabs.widget(_i)) == last_id:
return _i

def go_to_previous_file(self):
"""Ctrl+Tab"""
prev_index = self._get_previous_file_index()
if prev_index is not None:
self.set_stack_index(prev_index)
elif len(self.stack_history) == 0 and self.get_stack_count():
self.stack_history = [id(self.tabs.currentWidget())]
def tab_navigation_mru(self, forward=True):
"""
Tab navigation with "most recently used" behaviour.

def go_to_next_file(self):
"""Ctrl+Shift+Tab"""
if len(self.stack_history) > 1:
last = len(self.stack_history)-1
w_id = self.stack_history.pop(0)
self.stack_history.append(w_id)
last_id = self.stack_history[last]
for _i in range(self.tabs.count()):
if id(self.tabs.widget(_i)) == last_id:
self.set_stack_index(_i)
break
elif len(self.stack_history) == 0 and self.get_stack_count():
self.stack_history = [id(self.tabs.currentWidget())]
It's fired when pressing 'go to previous file' or 'go to next file'
shortcuts.

forward:
True: move to next file
False: move to previous file
"""
if self.tabs_switcher is None or not self.tabs_switcher.isVisible():
self.tabs_switcher = TabSwitcherWidget(self, self.stack_history,
self.tabs)
self.tabs_switcher.show()

self.tabs_switcher.select_row(1 if forward else -1)

def focus_changed(self):
"""Editor focus has changed"""
Expand Down Expand Up @@ -1979,6 +2041,18 @@ def dropEvent(self, event):
editor.insert_text( source.text() )
event.acceptProposedAction()

def keyReleaseEvent(self, event):
"""Reimplement Qt method.

Handle "most recent used" tab behavior,
When ctrl is released and tab_switcher is visible, tab will be changed.
"""
if (event.key() == Qt.Key_Control and self.tabs_switcher is not None
Copy link
Member Author

Choose a reason for hiding this comment

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

This will break if It's set a shortcut without "Ctrl" (Although I think that will be a really rare use case)

should I add a validation when configuring the shortcut?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please add the validation to prevent this possibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll add an option for also considering Alt, I think the shortcuts validation could be addressed in other Issue in a more general and reusable way

Copy link
Member

Choose a reason for hiding this comment

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

Agreed!

and self.tabs_switcher.isVisible()):
self.tabs_switcher.item_selected()
else:
super(EditorStack, self).keyPressEvent(event)


class EditorSplitter(QSplitter):
def __init__(self, parent, plugin, menu_actions, first=False,
Expand Down