PR: Add a tab switcher dialog to the Editor #4302
Conversation
|
|
||
| self.id_list = [] | ||
| self.load_data() | ||
| size = CONF.get('main', 'completion/size') |
There was a problem hiding this comment.
I reused completion widget size widget, is that ok? should I add a setting? or leave a fixed size
There was a problem hiding this comment.
What if you use the same size and positioning of the file switcher?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Besides, that's what VSCode also does, i.e. place both widgets at the same location.
There was a problem hiding this comment.
Yes, on the top is better for consistency. But we can evaluate the UX of all those dialogs in a separate issue
spyder/widgets/editor.py
Outdated
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Yes, please add the validation to prevent this possibility.
There was a problem hiding this comment.
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
spyder/widgets/editor.py
Outdated
| @@ -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.""" | |||
|
|
||
| self.id_list = [] | ||
| self.load_data() | ||
| size = CONF.get('main', 'completion/size') |
There was a problem hiding this comment.
Yes, on the top is better for consistency. But we can evaluate the UX of all those dialogs in a separate issue
spyder/widgets/editor.py
Outdated
| 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) |
There was a problem hiding this comment.
As discussed @rlaverde lets move all the stack history logic into a separate class (StackHistory)
spyder/widgets/editor.py
Outdated
| # stack history is in inverse order | ||
| id_ = self.stack_history[-(self.currentRow()+1)] | ||
|
|
||
| index = self.id_list.index(id_) |
There was a problem hiding this comment.
Lets remove this logic and put inside the StackHistory widget
|
@rlaverde, please make my suggested changes so I can merge this one. |
I already moved it to the top, and the shortcuts validation should be address in a more general way in another PR
|
spyder/widgets/editor.py
Outdated
| @@ -274,6 +275,111 @@ def breakpoints_changed(self): | |||
| self.save_breakpoints.emit(self.filename, repr(breakpoints)) | |||
|
|
|||
|
|
|||
| class StackHistory(MutableSequence): | |||
| """Handles editor stack history""" | |||
There was a problem hiding this comment.
period at the end :-), remeber docstrings are sentences
spyder/widgets/editor.py
Outdated
| if _id not in self.id_list: | ||
| self.history.remove(_id) | ||
|
|
||
| def __len__(self): return len(self.history) |
spyder/widgets/editor.py
Outdated
| def __getitem__(self, i): | ||
| return self.id_list.index(self.history[i]) | ||
|
|
||
| def __delitem__(self, i): del self.history[i] |
spyder/widgets/editor.py
Outdated
| return str(list(self)) | ||
|
|
||
| def insert(self, i, v): | ||
| _id = id(self.editor.tabs.widget(v)) |
spyder/widgets/editor.py
Outdated
| _id = id(self.editor.tabs.widget(v)) | ||
| self.history.insert(i, _id) | ||
|
|
||
| def remove(self, index): |
| row = (self.currentRow() + steps) % self.count() | ||
| self.setCurrentRow(row) | ||
|
|
||
| def set_dialog_position(self): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :-)
|
@rlaverde, please merge with |
| @@ -274,6 +275,119 @@ def breakpoints_changed(self): | |||
| self.save_breakpoints.emit(self.filename, repr(breakpoints)) | |||
|
|
|||
|
|
|||
| class StackHistory(MutableSequence): | |||
There was a problem hiding this comment.
Please move this to spyder/utils/editor.py
There was a problem hiding this comment.
Sorry, don't worry about it. We simply need to split this file in multiple files because it's huge!!
But let's do that in master (I'll do it).
There was a problem hiding this comment.
I mean, I'll do it after we release 3.2
|
I found an annoying behavior: some files get an ampersand in their name:
@rlaverde, please fix that and then I'll merge ;-) |
|
@ccordoba12 I'm not able to reproduce that behavior, any hint of what is causing it? |
|
It happens in some versions of Qt5, I think. Don't worry, it was a simple fix and I made it myself ;-) |
Fixes #43
TODO: