bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. (#2612)

* In config, put dump test code in a function; run it and unittest in 'if __name__ == '__main__'.
* Add class config.ConfigChanges based on changes_class_v4.py on bpo issue.
* Add class test_config.ChangesTest, partly based on configdialog_tests_v1.py on bpo issue.
* Revise configdialog to use ConfigChanges, mostly as specified in tracker msg297804.
* Revise test_configdialog to match configdialog changes.  All tests pass in both files.
* Remove configdialog functions unused or moved to ConfigChanges.
Cheryl Sabella contributed parts of the patch.

Lib/idlelib/config.py

@@ -44,7 +44,7 @@ class IdleConfParser(ConfigParser):
         """
         cfgFile - string, fully specified configuration file name
         """
-        self.file = cfgFile
+        self.file = cfgFile  # This is currently '' when testing.
         ConfigParser.__init__(self, defaults=cfgDefaults, strict=False)
 
     def Get(self, section, option, type=None, default=None, raw=False):
@@ -73,7 +73,8 @@ class IdleConfParser(ConfigParser):
 
     def Load(self):
         "Load the configuration file from disk."
-        self.read(self.file)
+        if self.file:
+            self.read(self.file)
 
 class IdleUserConfParser(IdleConfParser):
     """
@@ -130,21 +131,22 @@ class IdleUserConfParser(IdleConfParser):
     def Save(self):
         """Update user configuration file.
 
-        Remove empty sections. If resulting config isn't empty, write the file
-        to disk. If config is empty, remove the file from disk if it exists.
+        If self not empty after removing empty sections, write the file
+        to disk. Otherwise, remove the file from disk if it exists.
 
         """
-        if not self.IsEmpty():
-            fname = self.file
-            try:
-                cfgFile = open(fname, 'w')
-            except OSError:
-                os.unlink(fname)
-                cfgFile = open(fname, 'w')
-            with cfgFile:
-                self.write(cfgFile)
-        else:
-            self.RemoveFile()
+        fname = self.file
+        if fname:
+            if not self.IsEmpty():
+                try:
+                    cfgFile = open(fname, 'w')
+                except OSError:
+                    os.unlink(fname)
+                    cfgFile = open(fname, 'w')
+                with cfgFile:
+                    self.write(cfgFile)
+            else:
+                self.RemoveFile()
 
 class IdleConf:
     """Hold config parsers for all idle config files in singleton instance.
@@ -158,7 +160,7 @@ class IdleConf:
         (user home dir)/.idlerc/config-{config-type}.cfg
     """
     def __init__(self):
-        self.config_types = ('main', 'extensions', 'highlight', 'keys')
+        self.config_types = ('main', 'highlight', 'keys', 'extensions')
         self.defaultCfg = {}
         self.userCfg = {}
         self.cfg = {}  # TODO use to select userCfg vs defaultCfg
@@ -766,7 +768,6 @@ class IdleConf:
 
 idleConf = IdleConf()
 
-
 _warned = set()
 def _warn(msg, *key):
     key = (msg,) + key
@@ -778,9 +779,100 @@ def _warn(msg, *key):
         _warned.add(key)
 
 
+class ConfigChanges(dict):
+    """Manage a user's proposed configuration option changes.
+
+    Names used across multiple methods:
+        page -- one of the 4 top-level dicts representing a
+                .idlerc/config-x.cfg file.
+        config_type -- name of a page.
+        section -- a section within a page/file.
+        option -- name of an option within a section.
+        value -- value for the option.
+
+    Methods
+        add_option: Add option and value to changes.
+        save_option: Save option and value to config parser.
+        save_all: Save all the changes to the config parser and file.
+        delete_section: Delete section if it exists.
+        clear: Clear all changes by clearing each page.
+    """
+    def __init__(self):
+        "Create a page for each configuration file"
+        self.pages = []  # List of unhashable dicts.
+        for config_type in idleConf.config_types:
+            self[config_type] = {}
+            self.pages.append(self[config_type])
+
+    def add_option(self, config_type, section, item, value):
+        "Add item/value pair for config_type and section."
+        page = self[config_type]
+        value = str(value)  # Make sure we use a string.
+        if section not in page:
+            page[section] = {}
+        page[section][item] = value
+
+    @staticmethod
+    def save_option(config_type, section, item, value):
+        """Return True if the configuration value was added or changed.
+
+        Helper for save_all.
+        """
+        if idleConf.defaultCfg[config_type].has_option(section, item):
+            if idleConf.defaultCfg[config_type].Get(section, item) == value:
+                # The setting equals a default setting, remove it from user cfg.
+                return idleConf.userCfg[config_type].RemoveOption(section, item)
+        # If we got here, set the option.
+        return idleConf.userCfg[config_type].SetOption(section, item, value)
+
+    def save_all(self):
+        """Save configuration changes to the user config file.
+
+        Then clear self in preparation for additional changes.
+        """
+        idleConf.userCfg['main'].Save()
+        for config_type in self:
+            cfg_type_changed = False
+            page = self[config_type]
+            for section in page:
+                if section == 'HelpFiles':  # Remove it for replacement.
+                    idleConf.userCfg['main'].remove_section('HelpFiles')
+                    cfg_type_changed = True
+                for item, value in page[section].items():
+                    if self.save_option(config_type, section, item, value):
+                        cfg_type_changed = True
+            if cfg_type_changed:
+                idleConf.userCfg[config_type].Save()
+        for config_type in ['keys', 'highlight']:
+            # Save these even if unchanged!
+            idleConf.userCfg[config_type].Save()
+        self.clear()
+        # ConfigDialog caller must add the following call
+        # self.save_all_changed_extensions()  # Uses a different mechanism.
+
+    def delete_section(self, config_type, section):
+        """Delete a section from self, userCfg, and file.
+
+        Used to delete custom themes and keysets.
+        """
+        if section in self[config_type]:
+            del self[config_type][section]
+        configpage = idleConf.userCfg[config_type]
+        configpage.remove_section(section)
+        configpage.Save()
+
+    def clear(self):
+        """Clear all 4 pages.
+
+        Called in save_all after saving to idleConf.
+        XXX Mark window *title* when there are changes; unmark here.
+        """
+        for page in self.pages:
+            page.clear()
+
+
 # TODO Revise test output, write expanded unittest
-#
-if __name__ == '__main__':
+def _dump():  # htest # (not really, but ignore in coverage)
     from zlib import crc32
     line, crc = 0, 0
 
@@ -790,10 +882,10 @@ if __name__ == '__main__':
         line += 1
         crc = crc32(txt.encode(encoding='utf-8'), crc)
         print(txt)
-        #print('***', line, crc, '***')  # uncomment for diagnosis
+        #print('***', line, crc, '***')  # Uncomment for diagnosis.
 
     def dumpCfg(cfg):
-        print('\n', cfg, '\n')  # has variable '0xnnnnnnnn' addresses
+        print('\n', cfg, '\n')  # Cfg has variable '0xnnnnnnnn' address.
         for key in sorted(cfg.keys()):
             sections = cfg[key].sections()
             sprint(key)
@@ -808,3 +900,9 @@ if __name__ == '__main__':
     dumpCfg(idleConf.defaultCfg)
     dumpCfg(idleConf.userCfg)
     print('\nlines = ', line, ', crc = ', crc, sep='')
+
+if __name__ == '__main__':
+    import unittest
+    unittest.main('idlelib.idle_test.test_config',
+                  verbosity=2, exit=False)
+    #_dump()

Lib/idlelib/idle_test/test_config.py

@@ -9,12 +9,13 @@ from idlelib import config
 
 # Tests should not depend on fortuitous user configurations.
 # They must not affect actual user .cfg files.
-# Replace user parsers with empty parsers that cannot be saved.
+# Replace user parsers with empty parsers that cannot be saved
+# due to getting '' as the filename when created.
 
 idleConf = config.idleConf
 usercfg = idleConf.userCfg
 testcfg = {}
-usermain = testcfg['main'] = config.IdleUserConfParser('')  # filename
+usermain = testcfg['main'] = config.IdleUserConfParser('')
 userhigh = testcfg['highlight'] = config.IdleUserConfParser('')
 userkeys = testcfg['keys'] = config.IdleUserConfParser('')
 
@@ -136,6 +137,87 @@ class CurrentColorKeysTest(unittest.TestCase):
         userkeys.remove_section('Custom Keys')
 
 
+class ChangesTest(unittest.TestCase):
+
+    empty = {'main':{}, 'highlight':{}, 'keys':{}, 'extensions':{}}
+
+    def load(self):  # Test_add_option verifies that this works.
+        changes = self.changes
+        changes.add_option('main', 'Msec', 'mitem', 'mval')
+        changes.add_option('highlight', 'Hsec', 'hitem', 'hval')
+        changes.add_option('keys', 'Ksec', 'kitem', 'kval')
+        return changes
+
+    loaded = {'main': {'Msec': {'mitem': 'mval'}},
+              'highlight': {'Hsec': {'hitem': 'hval'}},
+              'keys': {'Ksec': {'kitem':'kval'}},
+              'extensions': {}}
+
+    def setUp(self):
+        self.changes = config.ConfigChanges()
+
+    def test_init(self):
+        self.assertEqual(self.changes, self.empty)
+
+    def test_add_option(self):
+        changes = self.load()
+        self.assertEqual(changes, self.loaded)
+        changes.add_option('main', 'Msec', 'mitem', 'mval')
+        self.assertEqual(changes, self.loaded)
+
+    def test_save_option(self):  # Static function does not touch changes.
+        save_option = self.changes.save_option
+        self.assertTrue(save_option('main', 'Indent', 'what', '0'))
+        self.assertFalse(save_option('main', 'Indent', 'what', '0'))
+        self.assertEqual(usermain['Indent']['what'], '0')
+
+        self.assertTrue(save_option('main', 'Indent', 'use-spaces', '0'))
+        self.assertEqual(usermain['Indent']['use-spaces'], '0')
+        self.assertTrue(save_option('main', 'Indent', 'use-spaces', '1'))
+        self.assertFalse(usermain.has_option('Indent', 'use-spaces'))
+        usermain.remove_section('Indent')
+
+    def test_save_added(self):
+        changes = self.load()
+        changes.save_all()
+        self.assertEqual(usermain['Msec']['mitem'], 'mval')
+        self.assertEqual(userhigh['Hsec']['hitem'], 'hval')
+        self.assertEqual(userkeys['Ksec']['kitem'], 'kval')
+        usermain.remove_section('Msec')
+        userhigh.remove_section('Hsec')
+        userkeys.remove_section('Ksec')
+
+    def test_save_help(self):
+        changes = self.changes
+        changes.save_option('main', 'HelpFiles', 'IDLE', 'idledoc')
+        changes.add_option('main', 'HelpFiles', 'ELDI', 'codeldi')
+        changes.save_all()
+        self.assertFalse(usermain.has_option('HelpFiles', 'IDLE'))
+        self.assertTrue(usermain.has_option('HelpFiles', 'ELDI'))
+
+    def test_save_default(self):  # Cover 2nd and 3rd false branches.
+        changes = self.changes
+        changes.add_option('main', 'Indent', 'use-spaces', '1')
+        # save_option returns False; cfg_type_changed remains False.
+
+    # TODO: test that save_all calls usercfg Saves.
+
+    def test_delete_section(self):
+        changes = self.load()
+        changes.delete_section('main', 'fake')  # Test no exception.
+        self.assertEqual(changes, self.loaded)  # Test nothing deleted.
+        for cfgtype, section in (('main', 'Msec'), ('keys', 'Ksec')):
+            changes.delete_section(cfgtype, section)
+            with self.assertRaises(KeyError):
+                changes[cfgtype][section]  # Test section gone.
+        # TODO Test change to userkeys and maybe save call.
+
+    def test_clear(self):
+        changes = self.load()
+        changes.clear()
+        self.assertEqual(changes, self.empty)
+
+
 class WarningTest(unittest.TestCase):
 
     def test_warn(self):

Lib/idlelib/idle_test/test_configdialog.py

@@ -3,7 +3,7 @@
 Half the class creates dialog, half works with user customizations.
 Coverage: 46% just by creating dialog, 56% with current tests.
 """
-from idlelib.configdialog import ConfigDialog, idleConf  # test import
+from idlelib.configdialog import ConfigDialog, idleConf, changes
 from test.support import requires
 requires('gui')
 from tkinter import Tk
@@ -21,17 +21,13 @@ testcfg = {
     'extensions': config.IdleUserConfParser(''),
 }
 
-# ConfigDialog.changed_items is a 3-level hierarchical dictionary of
-# pending changes that mirrors the multilevel user config dict.
-# For testing, record args in a list for comparison with expected.
-changes = []
 root = None
 configure = None
+mainpage = changes['main']
+highpage = changes['highlight']
+keyspage = changes['keys']
 
-
-class TestDialog(ConfigDialog):
-    def add_changed_item(self, *args):
-        changes.append(args)
+class TestDialog(ConfigDialog): pass  # Delete?
 
 
 def setUpModule():
@@ -63,31 +59,28 @@ class FontTabTest(unittest.TestCase):
         default_size = str(default_font[1])
         default_bold = default_font[2] == 'bold'
         configure.font_name.set('Test Font')
-        expected = [
-            ('main', 'EditorWindow', 'font', 'Test Font'),
-            ('main', 'EditorWindow', 'font-size', default_size),
-            ('main', 'EditorWindow', 'font-bold', default_bold)]
-        self.assertEqual(changes, expected)
+        expected = {'EditorWindow': {'font': 'Test Font',
+                                     'font-size': default_size,
+                                     'font-bold': str(default_bold)}}
+        self.assertEqual(mainpage, expected)
         changes.clear()
         configure.font_size.set(20)
-        expected = [
-            ('main', 'EditorWindow', 'font', 'Test Font'),
-            ('main', 'EditorWindow', 'font-size', '20'),
-            ('main', 'EditorWindow', 'font-bold', default_bold)]
-        self.assertEqual(changes, expected)
+        expected = {'EditorWindow': {'font': 'Test Font',
+                                     'font-size': '20',
+                                     'font-bold': str(default_bold)}}
+        self.assertEqual(mainpage, expected)
         changes.clear()
         configure.font_bold.set(not default_bold)
-        expected = [
-            ('main', 'EditorWindow', 'font', 'Test Font'),
-            ('main', 'EditorWindow', 'font-size', '20'),
-            ('main', 'EditorWindow', 'font-bold', not default_bold)]
-        self.assertEqual(changes, expected)
+        expected = {'EditorWindow': {'font': 'Test Font',
+                                     'font-size': '20',
+                                     'font-bold': str(not default_bold)}}
+        self.assertEqual(mainpage, expected)
 
     #def test_sample(self): pass  # TODO
 
     def test_tabspace(self):
         configure.space_num.set(6)
-        self.assertEqual(changes, [('main', 'Indent', 'num-spaces', 6)])
+        self.assertEqual(mainpage, {'Indent': {'num-spaces': '6'}})
 
 
 class HighlightTest(unittest.TestCase):
@@ -111,19 +104,19 @@ class GeneralTest(unittest.TestCase):
 
     def test_startup(self):
         configure.radio_startup_edit.invoke()
-        self.assertEqual(changes,
-                         [('main', 'General', 'editor-on-startup', 1)])
+        self.assertEqual(mainpage,
+                         {'General': {'editor-on-startup': '1'}})
 
     def test_autosave(self):
         configure.radio_save_auto.invoke()
-        self.assertEqual(changes, [('main', 'General', 'autosave', 1)])
+        self.assertEqual(mainpage, {'General': {'autosave': '1'}})
 
     def test_editor_size(self):
         configure.entry_win_height.insert(0, '1')
-        self.assertEqual(changes, [('main', 'EditorWindow', 'height', '140')])
+        self.assertEqual(mainpage, {'EditorWindow': {'height': '140'}})
         changes.clear()
         configure.entry_win_width.insert(0, '1')
-        self.assertEqual(changes, [('main', 'EditorWindow', 'width', '180')])
+        self.assertEqual(mainpage, {'EditorWindow': {'width': '180'}})
 
     #def test_help_sources(self): pass  # TODO