gh-112898: Fix double close dialog with warning about unsafed files #113513
Conversation
…iles Without this changeset I get two consecutive dialogs asking if I want to save an unsafed file, when choosing "Cancel" in the first one.
|
I've added "skip news" because the relevant news item is in #112898 |
ronaldoussoren
added
needs backport to 3.11
Lib/idlelib/macosx.py
Outdated
| @@ -221,7 +221,7 @@ def help_dialog(event=None): | |||
| # The binding above doesn't reliably work on all versions of Tk | |||
| # on macOS. Adding command definition below does seem to do the | |||
| # right thing for now. | |||
| root.createcommand('::tk::mac::Quit', flist.close_all_callback) | |||
| root.createcommand('::tk::mac::Quit', lambda: "break") | |||
There was a problem hiding this comment.
And what happens when remove this line at all?
Please try different ways to quit the application: not only via menu or closing the main window, but via the dock or when try to shutdown the computer. Try with several modified not saved files, are any changes lost when you cancel save for one of them?
There was a problem hiding this comment.
On macOS there is no "main" window in IDLE, the shell en editor windows are treated equaly and IDLE will quit when you close all of them. Thus the ways to quit IDLE:
- Manually close all windows
- Use the quit menu
- Use the quit keyboard shortcut
- Log off (or shutdown).
The last one should result in a quit event, and is something I haven't tested yet (too many open windows...), lets first get the normal behaviour correct, especially since retesting shows that the PR doesn't do what I say...
The behaviour with multiple unsaved windows is not yet good enough:
- Quiting using the menu (IDLE -> Quit IDLE):
all is fine, I get warnings for all unsaved files and choosing Cancel in one of them aborts quoting the app, that is IDLE keeps running and no more windows are closed.I'm convinced that the menu did work earlier, but now it doesn't, the menu selection is completely ignored. - Quiting using the keyboard shortcut: not quite there yet, I do get warnings for all unsaved files, but choosing Cancel still results in a warning for all other unsaved files.
We also don't quite match system behaviour, although that's hard to be sure about these days due to auto-saving in most system apps. I did compare with two other apps:
- Terminal.app: This app shows a warning for windows with active command-line apps, and does this before closing idle windows, whereas IDLE first closes windows that don't have unsaved state (including the shell window) and then warns about unsaved content
- Microsoft Word: This shows a pop-up with a summary of unsaved changes ("you have 2 documents with unsaved changes"), again before closing windows with unsaved state. When you do review changes you can cancel at any of the open files, but already closed files will stay closed.
It would be nice to try to close windows with unsaved state before closing the rest, but that's not essential. Same for Word's summary dialog before trying to close any window.
I now have something that works for me, but is not quite ready.
The cleaned up patch (on top of this PR):
diff --git a/Lib/idlelib/config.py b/Lib/idlelib/config.py
index 92992fd9cc..c6f09335d6 100644
--- a/Lib/idlelib/config.py
+++ b/Lib/idlelib/config.py
@@ -31,6 +31,7 @@
from tkinter.font import Font
import idlelib
+from idlelib import macosx
class InvalidConfigType(Exception): pass
class InvalidConfigSet(Exception): pass
@@ -660,6 +661,10 @@ def GetCoreKeys(self, keySetName=None):
'<<zoom-height>>': ['<Alt-Key-2>'],
}
+ if macosx.isAquaTk():
+ assert '<<close-all-windows>>' in keyBindings
+ del keyBindings['<<close-all-windows>>']
+
if keySetName:
if not (self.userCfg['keys'].has_section(keySetName) or
self.defaultCfg['keys'].has_section(keySetName)):
diff --git a/Lib/idlelib/macosx.py b/Lib/idlelib/macosx.py
index 683817d1ae..aca5c5f31c 100644
--- a/Lib/idlelib/macosx.py
+++ b/Lib/idlelib/macosx.py
@@ -216,12 +216,10 @@ def help_dialog(event=None):
root.bind('<<open-config-dialog>>', config_dialog)
root.createcommand('::tk::mac::ShowPreferences', config_dialog)
if flist:
- root.bind('<<close-all-windows>>', flist.close_all_callback)
-
# The binding above doesn't reliably work on all versions of Tk
# on macOS. Adding command definition below does seem to do the
# right thing for now.
- root.createcommand('::tk::mac::Quit', lambda: "break")
+ root.createcommand('::tk::mac::Quit', flist.close_all_callback)
if isCarbonTk():
# for Carbon AquaTk, replace the default Tk apple menuThe change to macosx.py reverts this PR, and the other change removes the default key binding for <<close-all-windows>>``. That change doesn't work for me though, the code as written results in getting no pop-ups for unsaved windows at all. The alternative change to config.py` does work, but would break other platforms.
diff --git a/Lib/idlelib/config.py b/Lib/idlelib/config.py
index 92992fd9cc..46c39b3718 100644
--- a/Lib/idlelib/config.py
+++ b/Lib/idlelib/config.py
@@ -31,6 +31,7 @@
from tkinter.font import Font
import idlelib
+from idlelib import macosx
class InvalidConfigType(Exception): pass
class InvalidConfigSet(Exception): pass
@@ -605,7 +606,7 @@ def GetCoreKeys(self, keySetName=None):
'<<paste>>': ['<Control-v>', '<Control-V>'],
'<<beginning-of-line>>': ['<Control-a>', '<Home>'],
'<<center-insert>>': ['<Control-l>'],
- '<<close-all-windows>>': ['<Control-q>'],
+ #'<<close-all-windows>>': ['<Control-q>'],
'<<close-window>>': ['<Alt-F4>'],
'<<do-nothing>>': ['<Control-x>'],
'<<end-of-file>>': ['<Control-d>'],I'll return to this in the morning, I'm must be overlooking something that explains why the clean patch for config.py doesn't have the expected behaviour.
There was a problem hiding this comment.
I'll return to this in the morning, I'm must be overlooking something that explains why the clean patch for config.py doesn't have the expected behaviour.
I couldn't let this go just yet...
For reasons I don't understand the call to macosx.isAquaTk breaks things, if I replace that by a check for sys.platform == "darwin" the code works as expected. That does break using the X11 bindings for Tk on macOS though, although I expected the number of users for that is negligible.
|
Please test with #112994. It shows different dialog windows (with and without the "Cancel" button) depending on the way the application is closed, so it can help you to understand what action is triggered. I think that |
But as I wrote above: the added if statement in |
Lib/idlelib/macosx.py
Outdated
| root.bind('<<close-all-windows>>', flist.close_all_callback) | ||
|
|
||
| # The binding above doesn't reliably work on all versions of Tk | ||
| # on macOS. Adding command definition below does seem to do the | ||
| # right thing for now. |
There was a problem hiding this comment.
The comment “The binding above…” should likely also be adjusted so that it makes sense after removing the binding it refers to.
I agree that the windowing system should be checked instead, so I would like to understand why ASan report:==76471==ERROR: AddressSanitizer: heap-use-after-free on address 0x61a0000a5ca8 at pc 0x00010bd99942 bp 0x7ff7bd8860f0 sp 0x7ff7bd8860e8
READ of size 8 at 0x61a0000a5ca8 thread T0
#0 0x10bd99941 in Tcl_FindCommand tclNamesp.c:2553
#1 0x108e3ac6b in ReallyKillMe tkMacOSXHLEvents.c:594
#2 0x10bdc097b in Tcl_ServiceEvent tclNotify.c:670
#3 0x10bdc355b in Tcl_DoOneEvent tclNotify.c:967
#4 0x106d13c6c in _tkinter_tkapp_mainloop _tkinter.c.h:534
#5 0x1028f93b4 in method_vectorcall_FASTCALL descrobject.c:393
#6 0x1028b929e in PyObject_Vectorcall call.c:327
#7 0x102e953f3 in _PyEval_EvalFrameDefault generated_cases.c.h:815
#8 0x102e2c2a8 in PyEval_EvalCode ceval.c:592
#9 0x102e1c2c8 in builtin_exec bltinmodule.c.h:540
#10 0x102a7e4db in cfunction_vectorcall_FASTCALL_KEYWORDS methodobject.c:441
#11 0x1028b929e in PyObject_Vectorcall call.c:327
#12 0x102e953f3 in _PyEval_EvalFrameDefault generated_cases.c.h:815
#13 0x1028b904b in _PyVectorcall_Call call.c:273
#14 0x1031b53e2 in pymain_run_module main.c:297
#15 0x1031b29ce in Py_RunMain main.c:707
#16 0x1031b4a6a in pymain_main main.c:737
#17 0x1031b4ea8 in Py_BytesMain main.c:761
#18 0x113cdd52d in start+0x1cd (dyld:x86_64+0x552d)
|
|
In Tk Aqua, the first Tk interpreter (i.e. the first Tcl interpreter to load Tk) is the one that will receive Apple high-level events, such as quitting. (The use-after-free bug I mentioned has to do with how Tk Aqua keeps a reference to the first Tk interpreter.) This means that defining
|
|
If specifying bindings in config-keys.def is mandatory as I described in #112898 (comment) then a hacky workaround would be to specify a key which is nonexistent in practice but still recognized by Tk, such as F35: --- a/Lib/idlelib/config-keys.def
+++ b/Lib/idlelib/config-keys.def
@@ -264,7 +264,7 @@ redo = <Shift-Command-Key-Z>
close-window = <Command-Key-w>
restart-shell = <Control-Key-F6>
save-window-as-file = <Shift-Command-Key-S>
-close-all-windows = <Command-Key-q>
+close-all-windows = <Key-F35>
view-restart = <Key-F6>
tabify-region = <Control-Key-5>
find-again = <Command-Key-g> <Key-F3>Then there would be no need to modify idlelib/config.py. |
I haven't tried to debug this yet, other than noticing that IDLE quits without asking about unsafed data if I test using
:-( |
That sadly doesn't work, IDLE just quits when quitting with unsaved files (both using the menu and Cmd-Q shortcut). This is definitely related to And that in turn is to be triggered by creating a |
Removing the <<close-all-windows>> bindings is still necessary to avoid double close pop-ups
|
That's it for tonight. The current PR works for me (TM), but contains code in macosx.py that I don't particularly like and still removes the |
I should clarify that I meant for the change I suggested to be used without the changes in this PR; were there other changes present when you tested it? Also note the line number of the change: it should apply to the binding for |
There was a problem hiding this comment.
I have verified that 4cefbea works as expected.
It also does not trigger the use-after-free bug, which can only occur once the first Tk interpreter is deleted. Calling root.createcommand(…) prevents root from deallocating, which in turn prevents the first Tk interpreter from being deleted.
Lib/idlelib/macosx.py
Outdated
| @@ -158,6 +168,7 @@ def overrideRootMenu(root, flist): | |||
| from idlelib import mainmenu | |||
| from idlelib import window | |||
|
|
|||
|
|
|||
``macosx.isAquaTk()`` needs access to Tk to query it. Before this changeset it created a temporary Tk instance. It turns out that the *first* instance of Tk created on macOS is special and must to be used to set up the commands to handle mac specific system events (file open, quit, ...). By restructuring the code a little we can avoid creating a temporary Tk object.
|
My latest commits to the PR clean up the code by tweaking the code a little to avoid having to create a temporary The code in The current code looks mergeable to me now. |
|
@terryjreedy, could you please review this PR? This fixes a real issue for macOS users (work may be lost when quitting IDLE, for example during system shutdown). |
|
[Started before the last commit] To fix the tests, there are multiple options.
|
TBH I'm not a fan of the
This change would be fine with me, I'd be surprised if there's anyone left that would want to run (let alone test) the X11 based Tk backend on macOS (let alone the Carbon backend, that might not even exist in Tk 8.6) That would IMHO require adding a test that asserts that the GUI type is "cocoa" on macOS to make it abundantly clear that this is the only supported setup.
Currently all tests pass on macOS with this PR (alternate framework name to not affect a python.org install): You'll quickly notice if a test reorg requires changes because
As @chrstphrchvz noted we should try to avoid more than one root, and that IMHO should include tests. We tend to run into edge cases in Tk when using multiple root's because Tcl users generally don't do this and also some functionality only works in the first root. |
Change the tests to ensure that the new invariants for macosx are met. This does assume that we're only interested in testing CocoaTk on macOS (which is IMHO a reasonable assumption)
|
@terryjreedy, I've updated the tests as well they now pass on all platforms. I chose to mock The tests assume that the GUI variant on macOS will be "cocoa", which is IMHO a safe assumption to make. The Carbon back-end to Tk is AFAIK long gone and X11 will be tested on other platforms. Sorry about pushing, but what's needed to bring this PR over the finish line? |
|
@terryjreedy, I intent to merge this tomorrow afternoon (CET timezone). |
Lib/idlelib/pyshell.py
Outdated
| @@ -1611,6 +1611,12 @@ def main(): | |||
| from idlelib.run import fix_scaling | |||
| fix_scaling(root) | |||
|
|
|||
| # start editor and/or shell windows: | |||
There was a problem hiding this comment.
Move this comment back where is was.
Lib/idlelib/config.py
Outdated
| # to the key bindings an rely the default binding. | ||
| # | ||
| # Without this IDLE will prompt twice about closing a file with | ||
| # unsaved # changes when the user quits IDLE using the keyboard |
There was a problem hiding this comment.
| # unsaved # changes when the user quits IDLE using the keyboard | |
| # unsaved changes when the user quits IDLE using the keyboard |
| if _idle_root is None: | ||
| _tk_type = "cocoa" | ||
| return |
There was a problem hiding this comment.
I believe everything from the 'testing' import to here can be deleted and these 3 lines dedented twice. IDLE will set _idle_root, at least when on mac and if a test on mac needs an accurate _tk_type, it should check requires('gui') and set _idle_root. Checking it here is redundant. Do as you wish for now.
There was a problem hiding this comment.
I'm leaving this as is for now, I can check later if it is save to remove the testing import.
|
When you're done making the requested changes, leave the comment: |
|
Manual checks on Windows of saving, saving as, and closing with unsaved changes seems to behave the same without and with this patch. |
…oren/cpython into pythongh-112898-double-dialog
|
I have made the requested changes; please review again. |
|
Sorry about the slow response, I didn't have time to work on Python last weekend after all. |
|
Ronald, I am leaving this for you to commit as I cannot test the macOS-specific changes except that they do not affect behavior on Windows. And that the cosmetic changes I requested are correct ;-). |
serhiy-storchaka
added
needs backport to 3.13
Without this changeset I get two consecutive dialogs asking if I want to save an unsafed file, when choosing "Cancel" in the first one.