Reset spyder and restart from within running application #2423

goanpeca · 2015-05-07T09:48:52Z

Description

Adds a new entry on the tools menu for reseting spyder from within the application

Or as an option in preferences... decision pending

After clicking a message box will ask to confirm, probably the message can be more explanatory so the user knows what he\she is about to do.

Todo

Agree on location of option
Agree on text of the new option

Nodd · 2015-05-07T09:50:47Z

I would put it in the general tab in the preferences window.

goanpeca · 2015-05-07T09:53:12Z

That feels weird, cause the preferences tab defines behaviors not actions.

This is an action, and it would be nice if it has a straightforward place for it.

Nodd · 2015-05-07T09:54:47Z

Yeah, but it is an action applied to the behaviors !
It could be added as another pushbutton along with Apply, OK and Cancel.

goanpeca · 2015-05-07T09:57:04Z

In that case I prefer adding the extra button.

Opinions @spyder-ide/core-developers?

goanpeca · 2015-05-27T17:53:39Z

@blink1073, @SylvainCorlay, @ccordoba12 any comments on this one?

blink1073 · 2015-05-27T18:25:51Z

I like it as part of the Tools menu, as it is a global action.

goanpeca · 2015-06-05T06:35:15Z

@SylvainCorlay? @ccordoba12 comments?

Nodd · 2015-06-05T07:00:49Z

I really think that resetting the preferences should be done from the preference window. That's the same reason why resetting shortcuts is done from the shortcuts tab.

goanpeca · 2015-06-05T08:46:38Z

I understand my dear, that is why I said, if it was going to be there, then it should be a button next to apply, ok, cancel, etc....

But I would like to hear from @SylvainCorlay and @ccordoba12

goanpeca · 2015-06-05T08:58:33Z

@Nodd there might be a reason to keep the restart option outside. Sometimes if there is an error or conflict the preferences dialog might not open, in which case restarting and reseting could not be done from within the gui.

Nodd · 2015-06-05T09:03:28Z

#2412 should lower the probability of this happening. Also it's still possible to use the commandline switch or remove the directory by hand (of course it's less user-friendly).

goanpeca · 2015-06-11T14:00:44Z

Ok @spyder-ide/core-developers this is the other option for the button.

This is a very small PR I would like to get off my back so please take a look at:

Location. Tools menu or Preferences dialog?, or both.
Text of message
Any comments on code?

Cheers

Nodd · 2015-06-11T14:33:32Z

spyderlib/plugins/configdialog.py

@@ -943,4 +950,4 @@ def apply_settings(self, options):
        if self.main.historylog is not None:
            self.main.historylog.apply_plugin_settings(['color_scheme_name'])
        if self.main.inspector is not None:
-            self.main.inspector.apply_plugin_settings(['color_scheme_name'])
+            self.main.inspector.apply_plugin_settings(['color_scheme_name'])


Missing a newline here

goanpeca · 2015-06-11T14:35:43Z

@Nodd Fixed

Nodd · 2015-06-11T14:38:33Z

spyderlib/restart_app.py

+            pid_reset = p.pid
+        except Exception as error:
+            print(command)
+            print(error)


Using the traceback module would be nicer here, since it prints the full traceback as if the Exception wasn't catched. You can use traceback.print_exc() with no arguments, it prints the right thing at the right place.

Great, I will do :)

goanpeca · 2015-06-11T14:48:20Z

@Nodd so you suggest something regarding the "infinite" loop?... or should we just let it roll and see how it works?

Nodd · 2015-06-11T15:13:21Z

spyderlib/spyder.py

@@ -713,6 +713,9 @@ def create_edit_action(text, tr_text, icon):
                                                  module_completion.reset(),
                                        tip=_("Refresh list of module names "
                                              "available in PYTHONPATH"))
+            reset_spyder_action = create_action(
+                self, _("Reset Spyder to factory defaults"),
+                triggered=lambda: self.reset_spyder())


You can avoid the lambda here I think, using self.reset_spyder directly

Nodd · 2015-06-11T15:19:05Z

I think the correct behaviour is to wait something like 30s, and if it's not finished, kill the process, stop the restart and display a QMessageBox with an error.
I'm afraid that if we let the infinite loop it'll bite us sometimes in the future for whatever reason...

Nodd · 2015-06-11T15:20:53Z

spyderlib/restart_app.py

+        except Exception as error:
+            print(command)
+            traceback.print_exc()
+            time.sleep(15)


If there is a problem here, the restart should be stopped instead of waiting 15s. This way the error will show up in the internal console.
Actually that means that you don't need to use traceback.print_exc(), just reraise the exception!

I am not sure I get you... by internal console you mean? at this point we already left the initial spyder instance and we are inside the restart script. I will add an extra flag so that the restart stops and a QMessageBox displays something.

goanpeca · 2015-06-11T16:44:30Z

Ok @Nodd I added a series of checks for better error handling which include a display of a message box in case errors occur, including close, reset and restart errors. Errors catched in the try,except part will also display the actual error. Errors based on too long running time will not display the error, but a friendly message....

Nodd · 2015-06-12T11:14:40Z

spyderlib/restart_app.py

-    while True:
+    max_counter = 1500  # Max loops to perform before aborting
+    counter = 0
+    while counter < max_counter:  # This is equivalent to ~ 5 minute (1500*0.2)


A for loop maybe ? ;)

ccordoba12 · 2015-07-11T16:42:50Z

spyderlib/restart_app.py

+    # Before launching a new Spyder instance we need to make sure that the
+    # previous one has closed. We wait for a fixed and "reasonable" amount of
+    # time and check, otherwise an error is launched
+    wait_time = 60*5 if IS_WINDOWS else 60*3 # Seconds


So we discussed this with @blink1073 and @SylvainCorlay and all of us agree that these waiting times are way too big. We think nobody is going to wait 3 min, much less 5 min, for Spyder to start :-)

So our proposal for this is: 30 sec for Linux/Mac (perhaps even 20), and 75 sec (90 tops) for Windows.

Ok Done...

30 and 90

goanpeca · 2015-07-11T16:51:32Z

Any my mental health @ccordoba12 ?

goanpeca · 2015-07-14T16:48:54Z

Anything holding this @ccordoba12?

ccordoba12 · 2015-07-15T04:21:08Z

spyderlib/restart_app.py

+                  RESTART_ERROR: _("Spyder restart error")}
+
+        if error:
+            e = error.__repr__()


Why is this necessary?

ccordoba12 · 2015-07-15T04:28:50Z

I think waiting 60 secs for --reset to work is too much because that would be added to the restart time, right?

Also, I'd like to discuss how the logic for reset + restart works. So restart_app calls spyder --reset first and then it closes the app, or is the other way around?

goanpeca · 2015-07-15T07:04:38Z

Ok. When you restart first we wait for spyder to close. If that happenned before the max wait time then we launch the reset process, if that went fine and took less than the stated time then the new spyder instance is launched.

ccordoba12 · 2015-07-15T14:34:31Z

So this means a user could have to wait up to (30 or 90) + 60 secs for a reset (plus the time it take for a new instance to appear).

Can we reduce the time to reset to 20 secs or so? It usually doesn't take that long ;-)

goanpeca · 2015-07-15T16:46:42Z

Yes, you are correct.

Sure, 20 secs is fine (I guess... we will know otherwise!)

goanpeca · 2015-07-15T16:46:47Z

Anything besides that ?

goanpeca · 2015-07-15T17:57:37Z

Anything else @ccordoba12 ?

ccordoba12 · 2015-07-17T19:51:32Z

Ok, merging this with @goanpeca (in person) for his own mental health :-P

Reset spyder and restart from within running application

blink1073 · 2015-07-17T20:02:51Z

woot!

goanpeca added User Interface (UI) User Experience (UX) labels May 7, 2015

goanpeca self-assigned this May 7, 2015

goanpeca added this to the v3.0 milestone May 7, 2015

Nodd reviewed Jun 11, 2015
View reviewed changes

Nodd reviewed Jun 12, 2015
View reviewed changes

goanpeca and others added 12 commits July 11, 2015 17:44

Fix duplicate range

4003c37

Rework exception handling

28fc618

Update error codes

eb8b585

Update error codes

d341b5c

Improve comments and variable names

4e9b789

Reorganize code in config dialog

a6f8646

Add splash screen

a0d7a5f

Using current sys.path to restart app

3f4b398

Using current sys.path to restart app

a738d31

Fix check logic on restarter script

4931b49

Fix typo in docstring

363853c

Modify path in restart method only in bootstrap mode

63ca7be

ccordoba12 reviewed Jul 11, 2015
View reviewed changes

reduce waiting times on all OSs

73eab81

ccordoba12 reviewed Jul 15, 2015
View reviewed changes

spyderlib/restart_app.py

RESTART_ERROR: _("Spyder restart error")}

if error:

e = error.__repr__()

Copy link

Member

ccordoba12 Jul 15, 2015

Choose a reason for hiding this comment

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

Why is this necessary?

Reduce max waiting time when resetting

37e0535

Remove helper function adn use to_text_string from spyderlib.py3compat

abdb8f9

ccordoba12 added a commit that referenced this pull request Jul 17, 2015

Merge pull request #2423 from goanpeca/reset

c759852

Reset spyder and restart from within running application

ccordoba12 merged commit c759852 into spyder-ide:master Jul 17, 2015

goanpeca deleted the reset branch August 3, 2015 04:35

Reset spyder and restart from within running application #2423

Reset spyder and restart from within running application #2423

Conversation

goanpeca commented May 7, 2015

Description

Todo

Nodd commented May 7, 2015

goanpeca commented May 7, 2015

Nodd commented May 7, 2015

goanpeca commented May 7, 2015

goanpeca commented May 27, 2015

blink1073 commented May 27, 2015

goanpeca commented Jun 5, 2015

Nodd commented Jun 5, 2015

goanpeca commented Jun 5, 2015

goanpeca commented Jun 5, 2015

Nodd commented Jun 5, 2015

goanpeca commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nodd commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jun 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goanpeca commented Jul 11, 2015

goanpeca commented Jul 14, 2015

Choose a reason for hiding this comment

ccordoba12 commented Jul 15, 2015

goanpeca commented Jul 15, 2015

ccordoba12 commented Jul 15, 2015

goanpeca commented Jul 15, 2015

goanpeca commented Jul 15, 2015

goanpeca commented Jul 15, 2015

ccordoba12 commented Jul 17, 2015

blink1073 commented Jul 17, 2015