PR: Fix unable to split editor #5083
Conversation
When closing a panel, the editors of this panel are still referenced on the Python side in `self.editor_ids` but are deleted on the Qt side. So `self.editor_ids.items()` contains ids of all deleted and existing editors. When we try to get the parent of a deleted editor, this is where the error is raised. Ideally, self.editor_ids.items() would not contains references to deleted editors.
spyder/widgets/editortools.py
Outdated
| if _id == editor_id and editor.parent() is parent: | ||
| self.current_editor = editor | ||
| break | ||
| try: |
There was a problem hiding this comment.
Temporary fix. I am working to improve this.
This reverts commit 3955986.
This was not working because when the signal `destroyed` is fired, it is already too late, the editor is not an editor anymore, it's a simple widget. Therefore, it is not removed from the the outline explorer settings. This needs to be done before the editors are destroyed so we pass the right id to the outline explorer. We do it instead within the `closeEvent` method of the EditorStack.
Fix an error when there is more than one panel split and self.get_last_focus_editorstack(editorwindow) is None. An error was raised if afterward we immediately click on the outline explorer without giving first the focus to an editor.
|
I would like to add a test for splitting the editor. Should this be done in the widget or the plugin? |
|
I think it should be done in widget level. |
|
The error I'm trying to fix here originated from the editor plugin code, so a test on the widget wouldn't have catch that. That is why I asked if I should do it at the plugin level instead. I'm not sure how to tackle this if we do this at the widget level... Do I create a fixture using the EditorPluginExample for this or do I create a fixture similar in test_editor module? Maybe the test can be left for another PR instead, so that the bug can be fixed quicker? |
I don't think we have the infrastructure in place to test at the plugin level. So I would agree on this. I think that the plan with the plugins decoupling into separate modules is that we can start spyder with just the needed modules/plugins and a temp config and run tests at the application level. |
|
@goanpeca thank you. I will then left the test out of this PR. There is also the fact that I'm comfortable to work at the widget level but I do not yet understand very well how things are working at the plugin level. |
| @@ -1238,7 +1238,7 @@ def register_editorstack(self, editorstack): | |||
| self.set_last_focus_editorstack(self, editorstack) | |||
| editorstack.set_closable( len(self.editorstacks) > 1 ) | |||
| if self.outlineexplorer is not None: | |||
| editorstack.set_outlineexplorer(self.outlineexplorer) | |||
| editorstack.set_outlineexplorer(self.outlineexplorer.explorer) | |||
There was a problem hiding this comment.
This was my bad in a refactoring I did some months ago. Thanks for catching it.
| @@ -746,6 +746,12 @@ def add_corner_widgets_to_tabbar(self, widgets): | |||
| def closeEvent(self, event): | |||
| self.threadmanager.close_all_threads() | |||
| self.analysis_timer.timeout.disconnect(self.analyze_script) | |||
|
|
|||
| # Remove editor references from the outline explorer settings | |||
| return self.get_last_focus_editorstack(editorwindow) | ||
| return editorstack | ||
|
|
||
| editorstack = self.get_last_focus_editorstack(editorwindow) |
|
@ccordoba12 thoughts, I think we can merge?, @jnsebgosselin maybe open a new issue to keep track of the pending test with reference to this and the original issue? |
|
@ccordoba12 this PR is ready to be reviewed. |
|
@jnsebgosselin, it looks good to me. Thanks a lot for looking into this tricky bugs! I just have one comment: issues 2 and 3 are also present in |
Fixes 3 issues related to splitting the Editor in master branch :
First issue :
Steps to reproduce the problem:
Traceback:
_Traceback (most recent call last):
File "C:\Users\jsgosselin\spyder\spyder\widgets\editor.py", line 2274, in
lambda: self.split(orientation=Qt.Vertical))
File "C:\Users\jsgosselin\spyder\spyder\widgets\editor.py", line 2335, in split
unregister_editorstack_cb=self.unregister_editorstack_cb)
File "C:\Users\jsgosselin\spyder\spyder\widgets\editor.py", line 2269, in init
self.register_editorstack_cb(self.editorstack)
File "C:\Users\jsgosselin\spyder\spyder\plugins\editor.py", line 1241, in register_editorstack
editorstack.set_outlineexplorer(self.outlineexplorer)
File "C:\Users\jsgosselin\spyder\spyder\widgets\editor.py", line 847, in set_outlineexplorer
self.outlineexplorer.is_visible.connect(self.refresh_outlineexplorer)
AttributeError: 'OutlineExplorer' object has no attribute 'is_visible'
Fix :
Pass
self.outlineexplorer.explorerto theeditorstackinstead ofself.outlineexplorerbecause they don't need the pluginSecond issue :
Steps to reproduce the problem :
Traceback :
_Traceback (most recent call last):
File "C:\Users\jsgosselin\spyder\spyder\widgets\editortools.py", line 491, in clicked
self.activated(item)
File "C:\Users\jsgosselin\spyder\spyder\widgets\editortools.py", line 482, in activated
if id == editor_id and editor.parent() is parent:
RuntimeError: wrapped C/C++ object of type CodeEditor has been deleted
Fix :
This bug occurs because editors are not removed correctly when a panel is close. This was done by connecting the
destroyedsignal to theremove_editormethod of the outline explorer by passing the editor as an argument. However, this was not working because once thedestroyedsignal is fired, the editor is already destroyed, and therefore the wrong id was passed to the outline editor and it was not able to removed its reference to it.Instead, we do this within the
closeEventmethod of the StackEditor, before the editors are destroyed so we can pass the right id to the outline explorer.Third issue :
Steps to reproduce the problem :
Traceback :
Traceback (most recent call last):
File "C:\Users\jsgosselin\spyder\spyder\plugins\editor.py", line 552, in
editorwindow=self))
File "C:\Users\jsgosselin\spyder\spyder\plugins\editor.py", line 1923, in load
focus=focus)
File "C:\Users\jsgosselin\spyder\spyder\plugins\editor.py", line 1506, in set_current_filename
return editorstack.set_current_filename(filename, focus)
AttributeError: 'NoneType' object has no attribute 'set_current_filename'
Traceback (most recent call last):
File "C:\Users\jsgosselin\spyder\spyder\widgets\editortools.py", line 492, in clicked
File "C:\Users\jsgosselin\spyder\spyder\widgets\editortools.py", line 477, in activated
parent = self.current_editor.parent()
AttributeError: 'NoneType' object has no attribute 'parent'
Fix :
To fix the issue, the case when
self.get_last_focus_editorstack(editorwindow)returnsNonehad to be defined in methodget_current_editorstackso that it returnsself.editorstacks[0]instead ofNoneto prevent the error when clicking on the outline editor.