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

Confirm restart (configuration option, and checkbox UI) #1746

Merged
merged 5 commits into from Jun 17, 2012

Conversation

ivanov
Copy link
Member

@ivanov ivanov commented May 17, 2012

Now, one can use the following to not get nagged for confirmation every
kernel restart:

    ipython qtconsole --IPythonWidget.confirm_restart=False

As with all configuration settings, setting this permanently requires editing one's profile.

I also added code to expose confirm_restart via checkbox menu item in the Kernel menu.

Note: There's a limitation to this implementation where the checkbox may not represent the state of confirm_restart for the current tab, but it does get set to the appropriate value if it is ever toggled.

update: This issue was fixed in a41db72

Finally, I did a bunch of typo clean up in the two related files.

update: these were committed directly to master, so now this PR is only the relevant code

@takluyver
Copy link
Member

As with the other PR, I think the idea is useful, but I'm not convinced we should add ever more menu entries.

Also, thanks for the spelling & grammar fixes, but in future, can you do them separately from functional pull requests? It makes the diff much bigger, so it's harder to see what's really changed. For that sort of straightforward clean up, I think it's OK to push straight to master - or if you do a typo-fixing pull request, we'll fast-track it in.

@Carreau
Copy link
Member

Carreau commented Jun 4, 2012

@ivanov,
You track the confirm_restart on a per tab basis, but the menu does not always update correctly when you change tab.
I'm puzzeld on how qt decide to update it, after some test I can't figure out.
I do not quite remember how or where is tracked wether the visible tab has changed, or even if it is tracked right now.

Or we should do it an the whole qtconsole as 1 option, not per tab.

As for the UI, the kernel menu is light now, and I think beeing able to toggle without restarting the qtconsole makes sens. You can also bind a force restart to a much more complicated shortcut that people won't hit by mistake (not ctrl+shift+dot, as dot is accessed by pressing shift on some non-english keyboard, but i'm not against ctrl+alt+del)

@fperez
Copy link
Member

fperez commented Jun 10, 2012

@ivanov, do you think the status mismatch is fixable? I'm leery of committing code we know can potentially expose inconsistencies like this. As always, better not to have a feature than to have a half-working one...

Now, one can use the following to not get nagged for confirmation every
kernel restart:

    ipython qtconsole --IPythonWidget.confirm_restart=False

As with all configuration setting, setting this permanently requires
editing one's profile.
There's a limitation to this implementation where the checkbox may not
represent the state of confirm_restart for the current tab, but it does
get set the appropriate value if it is ever toggled.
@ivanov
Copy link
Member Author

ivanov commented Jun 15, 2012

ok, i pushed the typo commit straight to master, so the non-trivial hanges should be clearer now. I'll work on the status mismatch

The way I originally wrote the code, it was possible for the
"Kernel" -> "Confirm kernel restart" checkbox to get out of sync with
the currently active tab. It turns out that QTabWidget has a
'currentChanged' slot that can be leveraged to update the checkbox to
properly reflect the currently active tab widget.
@ivanov
Copy link
Member Author

ivanov commented Jun 15, 2012

Ok, this PR is now fully ready. @fperez: turns out it was fixable. The checkbox gets updated whenever the active tab is switched, so there will not be any inconsistencies.

@Carreau: it is a per-tab option. the menu will update properly now as you change between tabs.

@takluyver: I don't think it clutters up the menu, since the Kernel menu only had two items before this, and now gets a third, useful item.

@Carreau
Copy link
Member

Carreau commented Jun 15, 2012

got this at startup, but it seem to work though :

$  python ipython.py qtconsole
Traceback (most recent call last):
  File "IPython/frontend/qt/console/mainwindow.py", line 853, in update_restart_checkbox
    self.confirm_restart_kernel_action.setChecked(widget.confirm_restart)
AttributeError: 'MainWindow' object has no attribute 'confirm_restart_kernel_action'

If you suspect this is an IPython bug, please report it at:
    https://github.com/ipython/ipython/issues
or send an email to the mailing list at ipython-dev@scipy.org

You can print a more detailed traceback right now with "%tb", or use "%debug"
to interactively debug it.

Extra-detailed tracebacks for bug-reporting purposes can be enabled via:
    c.Application.verbose_crash=True

[IPKernelApp] To connect another client to this kernel, use:
[IPKernelApp] --existing kernel-32445.json

@ivanov
Copy link
Member Author

ivanov commented Jun 15, 2012

good catch @Carreau ! I was connecting to the signal before the menu was initiated, and didn't notice this caused the error you saw since the console actually started up and was useable. Fixed this in 39728bd

@minrk
Copy link
Member

minrk commented Jun 17, 2012

This works well for multiple kernels, and the UI makes sense, so I am going ahead with merge.

minrk added a commit that referenced this pull request Jun 17, 2012
Confirm kernel restart in QtConsole (configuration option, and checkbox UI)
@minrk minrk merged commit 186e482 into ipython:master Jun 17, 2012
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Confirm kernel restart in QtConsole (configuration option, and checkbox UI)
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.

None yet

5 participants