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 support for other plugins in File Switcher #4290

Merged
merged 2 commits into from
Mar 25, 2017

Conversation

dalthviz
Copy link
Member

@dalthviz dalthviz commented Mar 22, 2017

Changes to make the file switcher work with the notebook plugin spyder-ide/spyder-notebook#45

@dalthviz dalthviz self-assigned this Mar 22, 2017
@dalthviz dalthviz requested review from ccordoba12 and goanpeca March 22, 2017 20:00
@@ -328,7 +328,12 @@ def save_initial_state(self):
for i, editor in enumerate(self.editors):
if editor is self.initial_editor:
self.initial_path = paths[i]
self.initial_cursors[paths[i]] = editor.textCursor()
# This try is needed to make the fileswitcher work with
# plugins that does not have a textCursor like the notebookpuglin.
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 from this line like the notebookpuglin

try:
return self.parent().get_current_editor()
except AttributeError:
return self.parent().get_current_client()
Copy link
Member

@ccordoba12 ccordoba12 Mar 22, 2017

Choose a reason for hiding this comment

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

I don't like this because it would only work for the Notebook plugin.

Please change this whole block to

self.tabs.currentWidget()

because that's what get_current_editor returns, and in NotebookPlugin rename self.tabwidget to self.tabs.

That should work for both the Editor and the Notebook without specializing for any particular plugin.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, there's no need to change anything in NotebookPlugin. Just the first change, i.e.

self.tabs.currentWidget()

is enough ;-)

@ccordoba12
Copy link
Member

What happens when you write @ in the file switcher associated with the Notebook? That probably gives an error so we need to deactivate it in this PR too.

@ccordoba12 ccordoba12 changed the title PR: Add support for the notebookplugin in the fileswitcher PR: Add support for other plugins in the File Switcher Mar 22, 2017
@ccordoba12 ccordoba12 changed the title PR: Add support for other plugins in the File Switcher PR: Add support for other plugins in File Switcher Mar 22, 2017
@dalthviz
Copy link
Member Author

@ccordoba12 About the @ nothing happens, it just shows nothing as an option to select:

fss

@goanpeca
Copy link
Member

goanpeca commented Mar 24, 2017

@ccordoba12 @dalthviz we should change the name of the plugin to just Notebook or Notebooks, having jupyter just takes up space.

@dalthviz
Copy link
Member Author

@goanpeca 👍

@ccordoba12 ccordoba12 added this to the v3.1.4 milestone Mar 24, 2017
@ccordoba12
Copy link
Member

ccordoba12 commented Mar 24, 2017

About the @ nothing happens

Great!! Less work to do ;-)

we should change the name of the plugin to just Notebook or Notebooks

Since we're going to remove the notebook header, I think it's better to leave Jupyter notebook so that people can clearly identify what's the notebook provenance.

@goanpeca
Copy link
Member

goanpeca commented Mar 24, 2017

@ccordoba12

Since we're going to remove the notebook header, I think it's better to leave Jupyter notebook so that people can clearly identify what's the notebook provenance.

I don't see us supporting any other type of notebook so why bother?, it also goes in line with the request I made about making shorter names for all plugins, e.g.

  • Static analysis -> Linter
  • Jupyter notebook -> Notebooks
  • Ipython console -> Console etc

@ccordoba12
Copy link
Member

ccordoba12 commented Mar 25, 2017

I agree it's a good idea to rename Jupyter notebook to Notebook before releasing 0.1. @dalthviz, please do that.

We can do the other renames for Spyder 4.0

@ccordoba12 ccordoba12 merged commit dc33ce4 into spyder-ide:3.1.x Mar 25, 2017
ccordoba12 added a commit that referenced this pull request Mar 25, 2017
ccordoba12 added a commit that referenced this pull request Mar 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants