Skip to content

gh-112898: Fix double close dialog with warning about unsafed files #113513

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.

Sign up for GitHub

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

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

gh-112898: Fix double close dialog with warning about unsafed files #113513

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a0bb7ab
gh-112898: Fix double close dialog with warning about unsafed files
ronaldoussoren Dec 27, 2023
eda5750
Actually work with various quit events (menu, shortcut, osascript)
ronaldoussoren Dec 27, 2023
3686a65
Remove comment that is no longer needed
ronaldoussoren Dec 27, 2023
b305b36
Restore working code path
ronaldoussoren Dec 27, 2023
a84a2d7
Crude hack to make sure the quit command is called
ronaldoussoren Dec 27, 2023
0ce7d86
Remove unnecesary key setup
ronaldoussoren Dec 27, 2023
ce08ba0
Remove unnecessary import
ronaldoussoren Dec 27, 2023
92c1206
typo
ronaldoussoren Dec 27, 2023
4cefbea
Getting lost ...
ronaldoussoren Dec 27, 2023
bbdee67
Restructure code to avoid creating a temporary ``tkinter.Tk``
ronaldoussoren Dec 28, 2023
ab7800a
Document why the close-all-windows keybinding is removed on macOS
ronaldoussoren Dec 28, 2023
63087e3
Fix test failure and lint warning
ronaldoussoren Dec 28, 2023
4c7ad17
Merge branch 'main' into gh-112898-double-dialog
ronaldoussoren Jan 9, 2024
4794ee7
The changes to macosx.py broke some of the tests
ronaldoussoren Jan 9, 2024
8d9a7f0
Fix copy&paste error
ronaldoussoren Jan 9, 2024
7ccc8fa
Merge branch 'main' into gh-112898-double-dialog
terryjreedy Feb 4, 2024
bf6f348
review comments
ronaldoussoren Feb 10, 2024
60ccabb
Merge branch 'gh-112898-double-dialog' of github.com:ronaldoussoren/c…
ronaldoussoren Feb 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/idlelib/macosx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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:

  1. 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
  2. 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 menu

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


if isCarbonTk():
# for Carbon AquaTk, replace the default Tk apple menu
Expand Down