Fixes #2306: Add a restart method #2312

goanpeca · 2015-04-09T13:57:52Z

Description

Add a restart method. Used the restart image as the ones used by the consoles. The shortcut for restarting is set as Shift+Alt+R.

Tasks

Windows (Py2 & Py3) (Tested on Windows 7)
Linux (Py2 & Py3) (Tested on Ubuntu 14.04)
OSX (Py2 & Py3)

Nodd · 2015-04-09T19:29:29Z

spyderlib/spyder_restart.py

+
+    if any([not spyder_args, not pid, not is_bootstrap, not spyder_folder]):
+        error = "This script can only be called from within a Spyder instance"
+        raise Exception(error)


RuntimeException is maybe cleaner

goanpeca · 2015-04-10T08:08:07Z

@Nodd, I included your suggestions and completed support on Linux and Windows. Still need Mac check and python3 check.

There is no way around the shell = True on Linux, at least the way I did this PR, but it works as expected, you do not see any terminals when restarting from the installed app, and if running from a terminal on bootstrap mode, the new instance gets run in that same terminal (which is what I would expect)

Nodd · 2015-04-10T08:28:56Z

shell=True is not a problem per se, but it's usually better to avoid it when possible.

goanpeca · 2015-04-10T08:35:00Z

@Nodd, indeed I understand

The docs say:

"Warning Using shell=True can be a security hazard. See the warning under Frequently Used Arguments for details."

But I do not think the example they give as a security hazard applies to this case in particular.

Nodd · 2015-04-10T12:28:19Z

It's also a bit slower but it doesn't apply here either.

blink1073 · 2015-04-10T20:54:01Z

spyderlib/spyder.py

+        system = platform.system().lower()
+
+        startupinfo = None
+        if 'win' in system:


This picks up darwin, you can use win32 here or put darwin first.

blink1073 · 2015-04-10T20:55:07Z

I tested this on OSX. It works after I made the two changes highlighted above.

goanpeca · 2015-04-11T07:25:56Z

Thanks @blink1073 😄

Did you check with python3 as well? I will push the changes shortly

goanpeca · 2015-04-11T09:12:00Z

Ok so I checked with Py3 on linux as well and made corrections as suggested by @blink1073.

Thanks @blink1073, and @Nodd for comments 😸

Any suggestions/comments/concerns @ccordoba12?

blink1073 · 2015-04-11T13:27:25Z

@goanpeca, I had used Python 3. Python 2 works now as well on OSX.

goanpeca · 2015-04-11T13:31:35Z

@blink1073 Ok great, then I guess is up to @ccordoba12 and @SylvainCorlay now ;-) to give some feedback

goanpeca · 2015-04-16T21:11:32Z

@ccordoba12, have you had time to check this?

ccordoba12 · 2015-04-16T23:51:57Z

spyderlib/config.py

@@ -477,6 +477,7 @@ def is_ubuntu():
              '_/save current layout': "Shift+Alt+S",
              '_/toggle default layout': "Shift+Alt+Home",
              '_/layout preferences': "Shift+Alt+P",
+              '_/restart': "Shift+Ctrl+R",


Shift+Ctrl+<foo> is used to focus plugins. Could you think of a different shortcut?

Hmm I can try... @Nodd, ideas?

Shift+Ctrl+Alt+AltGr+Meta+R ?
A bit more seriously, the other shortcuts use Shift+Alt, so it should be OK.

Ctrl+Shift+Alt+R ?

Shift+Alt+R only ?

Sure.. why not, @ccordoba12 ?

That's ok for me

ccordoba12 · 2015-04-17T00:09:10Z

Ok, first (quick) review is over :-)

goanpeca · 2015-04-17T23:22:01Z

spyderlib/spyder.py

+        try:
+            subprocess.Popen(command, shell=shell, env=env,
+                             startupinfo=startupinfo)
+            self.console.quit()


@ccordoba12 here is where it is closed

Ok, thanks for pointing it out! ;-)

But why not call self.closing() instead of self.console.quit()? That's the method I was looking for in my review :-)

Oops... Got it

Hmm @ccordoba12, self.closing() return True, or False... but does not close... so how does it work?

goanpeca · 2015-04-19T17:35:00Z

@ccordoba12, made suggested corrections

ccordoba12 · 2015-04-19T18:24:25Z

spyderlib/spyder_restart.py

+
+    # Maybe --new-instance should always be included to cope with corner cases
+    # where someone has one running version without and and another with
+    # If the user the one without (--new...) it would not start


Do you plan to address this comment?

Also, could reworded a bit? It's not easy to understand what it means :-)

Do you think it is a good idea to enforce upon restart the use of new instance? to avoid any weird cases? For most use cases it wont make a difference, but if for some reason people are using that option, then it would help.

Thoughts?

ccordoba12 · 2015-04-19T18:31:16Z

New review done :-) This is shaping nicely!

goanpeca · 2015-04-19T19:19:22Z

@ccordoba12 added latest suggestions ;-)

ccordoba12 · 2015-04-20T01:30:22Z

spyderlib/spyder.py

+        env['SPYDER_ARGS'] = spyder_args
+        env['SPYDER_PID'] = str(pid)
+        env['SPYDER_IS_BOOTSTRAP'] = str(is_bootstrap)
+        env['SPYDER_FOLDER'] = spyder_folder


It seems to me there is no need to pass SPYDER_FOLDER as an env var because it can be computed from the __file__ attribute of spyder_restart.py.

The other env vars are fine by the way :-)

goanpeca · 2015-04-20T07:22:31Z

@ccordoba12 added latest suggestions.

ccordoba12 · 2015-04-20T23:13:18Z

spyderlib/restart_app.py

+import sys
+import time
+
+from spyderlib.py3compat import to_text_string


I just tested this PR and it's not working with bootstrap because of this line.

The thing is this assumes Spyder is installed in your virtualenv or globally. But that's almost always not true (at least for me :-), when you're using bootstrap.

The only alternative I see is to copy/paste to_text_string here and use it below. It's a simple function, so I don't see any problem with that :-)

goanpeca · 2015-04-21T08:04:20Z

@ccordoba12 can you check again :-) ?

ccordoba12 · 2015-04-21T19:35:06Z

Great! This is working for me now :-)

Add restart functionality to the application

goanpeca changed the title ~~Add restart method and shortcut to Spyder~~ Add restart method and shortcut to Spyder. Solves #2306 Apr 9, 2015

goanpeca changed the title ~~Add restart method and shortcut to Spyder. Solves #2306~~ Add restart method and shortcut to Spyder. Apr 9, 2015

goanpeca added type:Enhancement User Experience (UX) labels Apr 9, 2015

goanpeca added this to the v3.0 milestone Apr 9, 2015

goanpeca added the User Interface (UI) label Apr 9, 2015

Nodd reviewed Apr 9, 2015
View reviewed changes

goanpeca changed the title ~~Add restart method and shortcut to Spyder.~~ Implement #2306: Add restart method and shortcut to Spyder Apr 10, 2015

goanpeca changed the title ~~Implement #2306: Add restart method and shortcut to Spyder~~ Implement #2306: Add a restart method to Spyder Apr 10, 2015

blink1073 reviewed Apr 10, 2015
View reviewed changes

goanpeca mentioned this pull request Apr 11, 2015

Enhancement: Add restart functionality to spyder. #2306

Closed

Add restart method and shortcut

4ded801

goanpeca changed the title ~~Implement #2306: Add a restart method to Spyder~~ Implements #2306: Add a restart method to Spyder Apr 11, 2015

goanpeca changed the title ~~Implements #2306: Add a restart method to Spyder~~ Implements #2306: Add a restart method Apr 11, 2015

ccordoba12 reviewed Apr 16, 2015
View reviewed changes

goanpeca reviewed Apr 17, 2015
View reviewed changes

goanpeca changed the title ~~Implements #2306: Add a restart method~~ Fixes #2306: Add a restart method Apr 19, 2015

goanpeca self-assigned this Apr 19, 2015

ccordoba12 reviewed Apr 19, 2015
View reviewed changes

Clean up methods

e580c08

ccordoba12 reviewed Apr 20, 2015
View reviewed changes

goanpeca added 2 commits April 20, 2015 09:19

Renamed restarter script

2ecefcc

Remove unnecessary variables, clean code

03603af

goanpeca mentioned this pull request Apr 20, 2015

How to change UI language #1001

Closed

ccordoba12 reviewed Apr 20, 2015
View reviewed changes

Remove dependency on spyderlib, added to_text_string function

aa66feb

ccordoba12 added a commit that referenced this pull request Apr 21, 2015

Merge pull request #2312 from goanpeca/restart

3e619c4

Add restart functionality to the application

ccordoba12 merged commit 3e619c4 into spyder-ide:master Apr 21, 2015

goanpeca deleted the restart branch April 21, 2015 21:36

Fixes #2306: Add a restart method #2312

Fixes #2306: Add a restart method #2312

Conversation

goanpeca commented Apr 9, 2015

Description

Tasks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Apr 10, 2015

Nodd commented Apr 10, 2015

goanpeca commented Apr 10, 2015

Nodd commented Apr 10, 2015

Choose a reason for hiding this comment

blink1073 commented Apr 10, 2015

goanpeca commented Apr 11, 2015

goanpeca commented Apr 11, 2015

blink1073 commented Apr 11, 2015

goanpeca commented Apr 11, 2015

goanpeca commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Apr 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Apr 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccordoba12 commented Apr 19, 2015

goanpeca commented Apr 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Apr 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Apr 21, 2015

ccordoba12 commented Apr 21, 2015