-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Make Editor tabs movable #3946
Conversation
This is great! Thanks a lot for it :-) @goanpeca, please review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small comments plus some questions
spyder/plugins/editor.py
Outdated
|
||
def move_editorstack_data(self, start, end): | ||
""" | ||
Move editorstack.data to be synchronized when tabs are moved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single line is shorter than 79?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I will change it
spyder/plugins/editor.py
Outdated
editorstack.blockSignals(True) | ||
|
||
for i in range(start, end, sign*1): | ||
data[i], data[i+sign] = data[i+sign], data[i] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pace around +
sign ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be probably better to invert the position of the for so that we move all editors at the same time
for i in range(start, end, sign * 1):
for editorstack in self.editorstacks :
data = editorstack.data
editorstack.blockSignals(True)
data[i], data[i + sign] = data[i + sign], data[i]
editorstack.blockSignals(False)
editorstack.refresh()
It feels that in case there is an error it would leave the UI in a better state.
Thoughts @rlaverde, @ccordoba12 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that space is really needed
and I think that inverting the order maybe could cause some undesired effect, because signals won't be blocked all the time while modifying an editorstack
also It will be a little overhead to call editorstack.blockSignals()
and editorstack.refresh()
so many times.
spyder/plugins/editor.py
Outdated
return | ||
else: | ||
steps = abs(end - start) | ||
sign = (end - start) // steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe sign is not the best name hmmm
direction
or step
maybe? and adding a comment saying +1 for right, -1 for left?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think direction
is better.
@@ -1005,16 +1005,8 @@ def get_tab_tip(self, filename, is_modified=None, is_readonly=None): | |||
else: | |||
return text % (osp.basename(filename), osp.dirname(filename)) | |||
|
|||
def __get_sorting_func(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a method we would like to preserve in case the user want to sort (by adding an option in the editor menu?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so, the less options we have the better.
This have a glitch related with #1170, It have to be solved first. |
You found the culprit @rlaverde for the moving tab glitch? |
Yes I fixed it, this line is causing the glitch https://github.com/spyder-ide/spyder/blob/master/spyder/widgets/editor.py#L1460 I implement an workaround in the last commit, but I'm not sure if it's the best solution. |
TODO: This is causing strange behaviors with changing files shortcuts (ctrl+tab) and closing opening files, because these implementation assume an strict order in |
I was wrong, that was the behavior previous my changes, Ctrl+Tab, Ctrl+Shit+Tab have a really strange behavior, I think this is ready, and I'll implement MRU(#43) in another PR |
@rlaverde thanks! |
@rlaverde, please do a rebase to see if that fixes the errors in Travis. |
Done 😄 |
This should have been done as part of PR spyder-ide#3946
Fixes: #564
Fixes: #1170
I made it really simple, just reused some code from #2372