Issue #27621: Put query response validation error messages in query box

instead of in separate massagebox.  Redo tests to match.
Add Mac OSX refinements.  Original patch by Mark Roseman.

2 files changed, 165 insertions, 194 deletions

diff --git a/Lib/idlelib/idle_test/test_query.py b/Lib/idlelib/idle_test/test_query.py
index ec86868..584cd88 100644
--- a/Lib/idlelib/idle_test/test_query.py
+++ b/Lib/idlelib/idle_test/test_query.py
@@ -16,21 +16,9 @@ from test.support import requires
 from tkinter import Tk
 import unittest
 from unittest import mock
-from idlelib.idle_test.mock_tk import Var, Mbox_func
+from idlelib.idle_test.mock_tk import Var
 from idlelib import query
 
-# Mock entry.showerror messagebox so don't need click to continue
-# when entry_ok and path_ok methods call it to display errors.
-
-orig_showerror = query.showerror
-showerror = Mbox_func()  # Instance has __call__ method.
-
-def setUpModule():
-    query.showerror = showerror
-
-def tearDownModule():
-    query.showerror = orig_showerror
-
 
 # NON-GUI TESTS
 
@@ -42,59 +30,49 @@ class QueryTest(unittest.TestCase):
         entry_ok = query.Query.entry_ok
         ok = query.Query.ok
         cancel = query.Query.cancel
-        # Add attributes needed for the tests.
+        # Add attributes and initialization needed for tests.
         entry = Var()
-        result = None
-        destroyed = False
+        entry_error = {}
+        def __init__(self, dummy_entry):
+            self.entry.set(dummy_entry)
+            self.entry_error['text'] = ''
+            self.result = None
+            self.destroyed = False
+        def showerror(self, message):
+            self.entry_error['text'] = message
         def destroy(self):
             self.destroyed = True
 
-    dialog = Dummy_Query()
-
-    def setUp(self):
-        showerror.title = None
-        self.dialog.result = None
-        self.dialog.destroyed = False
-
     def test_entry_ok_blank(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set(' ')
-        Equal(dialog.entry_ok(), None)
-        Equal((dialog.result, dialog.destroyed), (None, False))
-        Equal(showerror.title, 'Entry Error')
-        self.assertIn('Blank', showerror.message)
+        dialog = self.Dummy_Query(' ')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertEqual((dialog.result, dialog.destroyed), (None, False))
+        self.assertIn('blank line', dialog.entry_error['text'])
 
     def test_entry_ok_good(self):
-        dialog = self.dialog
+        dialog = self.Dummy_Query('  good ')
         Equal = self.assertEqual
-        dialog.entry.set('  good ')
         Equal(dialog.entry_ok(), 'good')
         Equal((dialog.result, dialog.destroyed), (None, False))
-        Equal(showerror.title, None)
+        Equal(dialog.entry_error['text'], '')
 
     def test_ok_blank(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('')
+        dialog = self.Dummy_Query('')
         dialog.entry.focus_set = mock.Mock()
-        Equal(dialog.ok(), None)
+        self.assertEqual(dialog.ok(), None)
         self.assertTrue(dialog.entry.focus_set.called)
         del dialog.entry.focus_set
-        Equal((dialog.result, dialog.destroyed), (None, False))
+        self.assertEqual((dialog.result, dialog.destroyed), (None, False))
 
     def test_ok_good(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('good')
-        Equal(dialog.ok(), None)
-        Equal((dialog.result, dialog.destroyed), ('good', True))
+        dialog = self.Dummy_Query('good')
+        self.assertEqual(dialog.ok(), None)
+        self.assertEqual((dialog.result, dialog.destroyed), ('good', True))
 
     def test_cancel(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        Equal(self.dialog.cancel(), None)
-        Equal((dialog.result, dialog.destroyed), (None, True))
+        dialog = self.Dummy_Query('does not matter')
+        self.assertEqual(dialog.cancel(), None)
+        self.assertEqual((dialog.result, dialog.destroyed), (None, True))
 
 
 class SectionNameTest(unittest.TestCase):
@@ -104,42 +82,32 @@ class SectionNameTest(unittest.TestCase):
         entry_ok = query.SectionName.entry_ok  # Function being tested.
         used_names = ['used']
         entry = Var()
-
-    dialog = Dummy_SectionName()
-
-    def setUp(self):
-        showerror.title = None
+        entry_error = {}
+        def __init__(self, dummy_entry):
+            self.entry.set(dummy_entry)
+            self.entry_error['text'] = ''
+        def showerror(self, message):
+            self.entry_error['text'] = message
 
     def test_blank_section_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set(' ')
-        Equal(dialog.entry_ok(), None)
-        Equal(showerror.title, 'Name Error')
-        self.assertIn('No', showerror.message)
+        dialog = self.Dummy_SectionName(' ')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('no name', dialog.entry_error['text'])
 
     def test_used_section_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('used')
-        Equal(self.dialog.entry_ok(), None)
-        Equal(showerror.title, 'Name Error')
-        self.assertIn('use', showerror.message)
+        dialog = self.Dummy_SectionName('used')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('use', dialog.entry_error['text'])
 
     def test_long_section_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('good'*8)
-        Equal(self.dialog.entry_ok(), None)
-        Equal(showerror.title, 'Name Error')
-        self.assertIn('too long', showerror.message)
+        dialog = self.Dummy_SectionName('good'*8)
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('longer than 30', dialog.entry_error['text'])
 
     def test_good_section_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('  good ')
-        Equal(dialog.entry_ok(), 'good')
-        Equal(showerror.title, None)
+        dialog = self.Dummy_SectionName('  good ')
+        self.assertEqual(dialog.entry_ok(), 'good')
+        self.assertEqual(dialog.entry_error['text'], '')
 
 
 class ModuleNameTest(unittest.TestCase):
@@ -149,42 +117,32 @@ class ModuleNameTest(unittest.TestCase):
         entry_ok = query.ModuleName.entry_ok  # Function being tested.
         text0 = ''
         entry = Var()
-
-    dialog = Dummy_ModuleName()
-
-    def setUp(self):
-        showerror.title = None
+        entry_error = {}
+        def __init__(self, dummy_entry):
+            self.entry.set(dummy_entry)
+            self.entry_error['text'] = ''
+        def showerror(self, message):
+            self.entry_error['text'] = message
 
     def test_blank_module_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set(' ')
-        Equal(dialog.entry_ok(), None)
-        Equal(showerror.title, 'Name Error')
-        self.assertIn('No', showerror.message)
+        dialog = self.Dummy_ModuleName(' ')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('no name', dialog.entry_error['text'])
 
     def test_bogus_module_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('__name_xyz123_should_not_exist__')
-        Equal(self.dialog.entry_ok(), None)
-        Equal(showerror.title, 'Import Error')
-        self.assertIn('not found', showerror.message)
+        dialog = self.Dummy_ModuleName('__name_xyz123_should_not_exist__')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('not found', dialog.entry_error['text'])
 
     def test_c_source_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('itertools')
-        Equal(self.dialog.entry_ok(), None)
-        Equal(showerror.title, 'Import Error')
-        self.assertIn('source-based', showerror.message)
+        dialog = self.Dummy_ModuleName('itertools')
+        self.assertEqual(dialog.entry_ok(), None)
+        self.assertIn('source-based', dialog.entry_error['text'])
 
     def test_good_module_name(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.entry.set('idlelib')
+        dialog = self.Dummy_ModuleName('idlelib')
         self.assertTrue(dialog.entry_ok().endswith('__init__.py'))
-        Equal(showerror.title, None)
+        self.assertEqual(dialog.entry_error['text'], '')
 
 
 # 3 HelpSource test classes each test one function.
@@ -198,13 +156,13 @@ class HelpsourceBrowsefileTest(unittest.TestCase):
         browse_file = query.HelpSource.browse_file
         pathvar = Var()
 
-    dialog = Dummy_HelpSource()
-
     def test_file_replaces_path(self):
-        # Path is widget entry, file is file dialog return.
-        dialog = self.dialog
+        dialog = self.Dummy_HelpSource()
+        # Path is widget entry, either '' or something.
+        # Func return is file dialog return, either '' or something.
+        # Func return should override widget entry.
+        # We need all 4 combination to test all (most) code paths.
         for path, func, result in (
-                # We need all combination to test all (most) code paths.
                 ('', lambda a,b,c:'', ''),
                 ('', lambda a,b,c: __file__, __file__),
                 ('htest', lambda a,b,c:'', 'htest'),
@@ -217,78 +175,72 @@ class HelpsourceBrowsefileTest(unittest.TestCase):
 
 
 class HelpsourcePathokTest(unittest.TestCase):
-    "Test path_ok method of ModuleName subclass of Query."
+    "Test path_ok method of HelpSource subclass of Query."
 
     class Dummy_HelpSource:
         path_ok = query.HelpSource.path_ok
         path = Var()
-
-    dialog = Dummy_HelpSource()
+        path_error = {}
+        def __init__(self, dummy_path):
+            self.path.set(dummy_path)
+            self.path_error['text'] = ''
+        def showerror(self, message, widget=None):
+            self.path_error['text'] = message
 
     @classmethod
     def tearDownClass(cls):
         query.platform = orig_platform
 
-    def setUp(self):
-        showerror.title = None
-
     def test_path_ok_blank(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.path.set(' ')
-        Equal(dialog.path_ok(), None)
-        Equal(showerror.title, 'File Path Error')
-        self.assertIn('No help', showerror.message)
+        dialog = self.Dummy_HelpSource(' ')
+        self.assertEqual(dialog.path_ok(), None)
+        self.assertIn('no help file', dialog.path_error['text'])
 
     def test_path_ok_bad(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
-        dialog.path.set(__file__ + 'bad-bad-bad')
-        Equal(dialog.path_ok(), None)
-        Equal(showerror.title, 'File Path Error')
-        self.assertIn('not exist', showerror.message)
+        dialog = self.Dummy_HelpSource(__file__ + 'bad-bad-bad')
+        self.assertEqual(dialog.path_ok(), None)
+        self.assertIn('not exist', dialog.path_error['text'])
 
     def test_path_ok_web(self):
-        dialog = self.dialog
+        dialog = self.Dummy_HelpSource('')
         Equal = self.assertEqual
         for url in 'www.py.org', 'http://py.org':
             with self.subTest():
                 dialog.path.set(url)
-                Equal(dialog.path_ok(), url)
-                Equal(showerror.title, None)
+                self.assertEqual(dialog.path_ok(), url)
+                self.assertEqual(dialog.path_error['text'], '')
 
     def test_path_ok_file(self):
-        dialog = self.dialog
-        Equal = self.assertEqual
+        dialog = self.Dummy_HelpSource('')
         for platform, prefix in ('darwin', 'file://'), ('other', ''):
             with self.subTest():
                 query.platform = platform
                 dialog.path.set(__file__)
-                Equal(dialog.path_ok(), prefix + __file__)
-                Equal(showerror.title, None)
+                self.assertEqual(dialog.path_ok(), prefix + __file__)
+                self.assertEqual(dialog.path_error['text'], '')
 
 
 class HelpsourceEntryokTest(unittest.TestCase):
-    "Test entry_ok method of ModuleName subclass of Query."
+    "Test entry_ok method of HelpSource subclass of Query."
 
     class Dummy_HelpSource:
         entry_ok = query.HelpSource.entry_ok
+        entry_error = {}
+        path_error = {}
         def item_ok(self):
             return self.name
         def path_ok(self):
             return self.path
 
-    dialog = Dummy_HelpSource()
-
     def test_entry_ok_helpsource(self):
-        dialog = self.dialog
+        dialog = self.Dummy_HelpSource()
         for name, path, result in ((None, None, None),
                                    (None, 'doc.txt', None),
                                    ('doc', None, None),
                                    ('doc', 'doc.txt', ('doc', 'doc.txt'))):
             with self.subTest():
                 dialog.name, dialog.path = name, path
-                self.assertEqual(self.dialog.entry_ok(), result)
+                self.assertEqual(dialog.entry_ok(), result)
 
 
 # GUI TESTS
@@ -344,10 +296,10 @@ class SectionnameGuiTest(unittest.TestCase):
         root = Tk()
         dialog =  query.SectionName(root, 'T', 't', {'abc'}, _utest=True)
         Equal = self.assertEqual
-        Equal(dialog.used_names, {'abc'})
+        self.assertEqual(dialog.used_names, {'abc'})
         dialog.entry.insert(0, 'okay')
         dialog.button_ok.invoke()
-        Equal(dialog.result, 'okay')
+        self.assertEqual(dialog.result, 'okay')
         del dialog
         root.destroy()
         del root
@@ -362,9 +314,8 @@ class ModulenameGuiTest(unittest.TestCase):
     def test_click_module_name(self):
         root = Tk()
         dialog =  query.ModuleName(root, 'T', 't', 'idlelib', _utest=True)
-        Equal = self.assertEqual
-        Equal(dialog.text0, 'idlelib')
-        Equal(dialog.entry.get(), 'idlelib')
+        self.assertEqual(dialog.text0, 'idlelib')
+        self.assertEqual(dialog.entry.get(), 'idlelib')
         dialog.button_ok.invoke()
         self.assertTrue(dialog.result.endswith('__init__.py'))
         del dialog
diff --git a/Lib/idlelib/query.py b/Lib/idlelib/query.py
index c806c6b..a4584df 100644
--- a/Lib/idlelib/query.py
+++ b/Lib/idlelib/query.py
@@ -10,6 +10,8 @@ The 'return value' is .result set to either a valid answer or None.
 
 Subclass SectionName gets a name for a new config file section.
 Configdialog uses it for new highlight theme and keybinding set names.
+Subclass ModuleName gets a name for File => Open Module.
+Subclass HelpSource gets menu item and path for additions to Help menu.
 """
 # Query and Section name result from splitting GetCfgSectionNameDialog
 # of configSectionNameDialog.py (temporarily config_sec.py) into
@@ -21,10 +23,10 @@ Configdialog uses it for new highlight theme and keybinding set names.
 import importlib
 import os
 from sys import executable, platform  # Platform is set for one test.
-from tkinter import Toplevel, StringVar
+from tkinter import Toplevel, StringVar, W, E, N, S
 from tkinter import filedialog
-from tkinter.messagebox import showerror
 from tkinter.ttk import Frame, Button, Entry, Label
+from tkinter.font import Font
 
 class Query(Toplevel):
     """Base class for getting verified answer from a user.
@@ -47,18 +49,26 @@ class Query(Toplevel):
         """
         Toplevel.__init__(self, parent)
         self.withdraw()  # Hide while configuring, especially geometry.
-        self.configure(borderwidth=5)
-        self.resizable(height=False, width=False)
+        self.parent = parent
         self.title(title)
+        self.message = message
+        self.text0 = text0
+        self.used_names = used_names
         self.transient(parent)
         self.grab_set()
-        self.bind('<Key-Return>', self.ok)
+        windowingsystem = self.tk.call('tk', 'windowingsystem')
+        if windowingsystem == 'aqua':
+            try:
+                self.tk.call('::tk::unsupported::MacWindowStyle', 'style',
+                             self._w, 'moveableModal', '')
+            except:
+                pass
+            self.bind("<Command-.>", self.cancel)
         self.bind('<Key-Escape>', self.cancel)
         self.protocol("WM_DELETE_WINDOW", self.cancel)
-        self.parent = parent
-        self.message = message
-        self.text0 = text0
-        self.used_names = used_names
+        self.bind('<Key-Return>', self.ok)
+        self.bind("<KP_Enter>", self.ok)
+        self.resizable(height=False, width=False)
         self.create_widgets()
         self.update_idletasks()  # Needed here for winfo_reqwidth below.
         self.geometry(  # Center dialog over parent (or below htest box).
@@ -75,32 +85,42 @@ class Query(Toplevel):
 
     def create_widgets(self):  # Call from override, if any.
         # Bind to self widgets needed for entry_ok or unittest.
-        self.frame = frame = Frame(self, borderwidth=2, relief='sunken', )
+        self.frame = frame = Frame(self, padding=10)
+        frame.grid(column=0, row=0, sticky='news')
+        frame.grid_columnconfigure(0, weight=1)
+
         entrylabel = Label(frame, anchor='w', justify='left',
                            text=self.message)
         self.entryvar = StringVar(self, self.text0)
         self.entry = Entry(frame, width=30, textvariable=self.entryvar)
         self.entry.focus_set()
-
-        buttons = Frame(self)
-        self.button_ok = Button(buttons, text='Ok', default='active',
-                width=8, command=self.ok)
-        self.button_cancel = Button(buttons, text='Cancel',
-                width=8, command=self.cancel)
-
-        frame.pack(side='top', expand=True, fill='both')
-        entrylabel.pack(padx=5, pady=5)
-        self.entry.pack(padx=5, pady=5)
-        buttons.pack(side='bottom')
-        self.button_ok.pack(side='left', padx=5)
-        self.button_cancel.pack(side='right', padx=5)
+        self.error_font = Font(name='TkCaptionFont',
+                               exists=True, root=self.parent)
+        self.entry_error = Label(frame, text=' ', foreground='red',
+                                 font=self.error_font)
+        self.button_ok = Button(
+                frame, text='OK', default='active', command=self.ok)
+        self.button_cancel = Button(
+                frame, text='Cancel', command=self.cancel)
+
+        entrylabel.grid(column=0, row=0, columnspan=3, padx=5, sticky=W)
+        self.entry.grid(column=0, row=1, columnspan=3, padx=5, sticky=W+E,
+                        pady=[10,0])
+        self.entry_error.grid(column=0, row=2, columnspan=3, padx=5,
+                              sticky=W+E)
+        self.button_ok.grid(column=1, row=99, padx=5)
+        self.button_cancel.grid(column=2, row=99, padx=5)
+
+    def showerror(self, message, widget=None):
+        #self.bell(displayof=self)
+        (widget or self.entry_error)['text'] = 'ERROR: ' + message
 
     def entry_ok(self):  # Example: usually replace.
         "Return non-blank entry or None."
+        self.entry_error['text'] = ''
         entry = self.entry.get().strip()
         if not entry:
-            showerror(title='Entry Error',
-                    message='Blank line.', parent=self)
+            self.showerror('blank line.')
             return None
         return entry
 
@@ -134,19 +154,16 @@ class SectionName(Query):
 
     def entry_ok(self):
         "Return sensible ConfigParser section name or None."
+        self.entry_error['text'] = ''
         name = self.entry.get().strip()
         if not name:
-            showerror(title='Name Error',
-                    message='No name specified.', parent=self)
+            self.showerror('no name specified.')
             return None
         elif len(name)>30:
-            showerror(title='Name Error',
-                    message='Name too long. It should be no more than '+
-                    '30 characters.', parent=self)
+            self.showerror('name is longer than 30 characters.')
             return None
         elif name in self.used_names:
-            showerror(title='Name Error',
-                    message='This name is already in use.', parent=self)
+            self.showerror('name is already in use.')
             return None
         return name
 
@@ -162,30 +179,27 @@ class ModuleName(Query):
 
     def entry_ok(self):
         "Return entered module name as file path or None."
+        self.entry_error['text'] = ''
         name = self.entry.get().strip()
         if not name:
-            showerror(title='Name Error',
-                    message='No name specified.', parent=self)
+            self.showerror('no name specified.')
             return None
         # XXX Ought to insert current file's directory in front of path.
         try:
             spec = importlib.util.find_spec(name)
         except (ValueError, ImportError) as msg:
-            showerror("Import Error", str(msg), parent=self)
+            self.showerror(str(msg))
             return None
         if spec is None:
-            showerror("Import Error", "module not found",
-                      parent=self)
+            self.showerror("module not found")
             return None
         if not isinstance(spec.loader, importlib.abc.SourceLoader):
-            showerror("Import Error", "not a source-based module",
-                      parent=self)
+            self.showerror("not a source-based module")
             return None
         try:
             file_path = spec.loader.get_filename(name)
         except AttributeError:
-            showerror("Import Error",
-                      "loader does not support get_filename",
+            self.showerror("loader does not support get_filename",
                       parent=self)
             return None
         return file_path
@@ -204,8 +218,9 @@ class HelpSource(Query):
         """
         self.filepath = filepath
         message = 'Name for item on Help menu:'
-        super().__init__(parent, title, message, text0=menuitem,
-                 used_names=used_names, _htest=_htest, _utest=_utest)
+        super().__init__(
+                parent, title, message, text0=menuitem,
+                used_names=used_names, _htest=_htest, _utest=_utest)
 
     def create_widgets(self):
         super().create_widgets()
@@ -216,10 +231,16 @@ class HelpSource(Query):
         self.path = Entry(frame, textvariable=self.pathvar, width=40)
         browse = Button(frame, text='Browse', width=8,
                         command=self.browse_file)
+        self.path_error = Label(frame, text=' ', foreground='red',
+                                font=self.error_font)
 
-        pathlabel.pack(anchor='w', padx=5, pady=3)
-        self.path.pack(anchor='w', padx=5, pady=3)
-        browse.pack(pady=3)
+        pathlabel.grid(column=0, row=10, columnspan=3, padx=5, pady=[10,0],
+                       sticky=W)
+        self.path.grid(column=0, row=11, columnspan=2, padx=5, sticky=W+E,
+                       pady=[10,0])
+        browse.grid(column=2, row=11, padx=5, sticky=W+S)
+        self.path_error.grid(column=0, row=12, columnspan=3, padx=5,
+                             sticky=W+E)
 
     def askfilename(self, filetypes, initdir, initfile):  # htest #
         # Extracted from browse_file so can mock for unittests.
@@ -256,17 +277,14 @@ class HelpSource(Query):
         "Simple validity check for menu file path"
         path = self.path.get().strip()
         if not path: #no path specified
-            showerror(title='File Path Error',
-                      message='No help file path specified.',
-                      parent=self)
+            self.showerror('no help file path specified.', self.path_error)
             return None
         elif not path.startswith(('www.', 'http')):
             if path[:5] == 'file:':
                 path = path[5:]
             if not os.path.exists(path):
-                showerror(title='File Path Error',
-                          message='Help file path does not exist.',
-                          parent=self)
+                self.showerror('help file path does not exist.',
+                               self.path_error)
                 return None
             if platform == 'darwin':  # for Mac Safari
                 path =  "file://" + path
@@ -274,6 +292,8 @@ class HelpSource(Query):
 
     def entry_ok(self):
         "Return apparently valid (name, path) or None"
+        self.entry_error['text'] = ''
+        self.path_error['text'] = ''
         name = self.item_ok()
         path = self.path_ok()
         return None if name is None or path is None else (name, path)