configdialog.ConfigDialog mixes together two things: a user interface for changing options and a mechanism for storing proposed changes and applying them to the configuration dictionaries managed by config. The change mechanism is based on a 3-level dict that maps user config file, config file section, and section entry to a new values. Most GUI event handlers insert entries into the dict. Cancel clears the dict. Apply and call a method to apply changes (and then clear them).

This issue will factor out the changes structure and all methods that touch it into a new Changes class. ConfigDialog needs to be able to call methods to add changes, clear changes, apply changes, and maybe something else. GUI tests need to be able to query currently stored changes.

Tests of Changes, which are part of this issue and should be 'complete', will accept examples changes from the test functions and send changes to either configparser mocks or even the actual configparser structures, but not to disk.

A complete test of the GUI (not part of this issue) will simulate user interaction with every widget and then query Changes() to see that the proper changes orders have been recorded.

If possible, I'd like to work on this.

Let's do it 'right'.

Step 1: Write class design in the form of a class with methods with APIs and docstrings as the bodies.

class Changes:
   """Manage a user's proposed configuation option changes.

Changes are either cancelled or applied in a single transaction.
Methods used by ConfigDialog:
meth_a does xa
Methods used only for testing (if any)
meth_t does xt for testing
Internal methods not called externally:
meth_b does xb.
"""

   def __init__(self, ???):  # new
   """Create configuration option change manager.

arg: bool, do we like George Boole?
"""

Temporarily comment methods as new, old, renamed from whatever.

Step 2. Test design by writing a test class with at least a few test methods. If methods are not easy to write, perhaps we should change design.

class ChangesTest(unittest.TestCase):
# no gui needed.    

Step 3. Write simple, clear bodies that make tests pass.

However much you submit, I will review in that order: design, tests, implementation.

High, because other issues depend on this.

Here's the beginning design. Since it's a high priority, I didn't want to wait too long to show you something. As you can see, I still had a lot of questions and things I wasn't sure about.

I have some tests, but wanted to have it more complete before submitting it.

When you said 'submit', I didn't know if you wanted a real PR at each step or just files here. If you'd prefer a PR so you could modify it, let me know and I'll prepare that tomorrow.

Thanks!

The question about reusing ConfigParser (or IdleConfigParser) was a good one. I spent at least an hour looking again at the code and configparser doc.

Observations:

1. Dropping 2.7 support allows us to use mapping protocol access:
   https://docs.python.org/3/library/configparser.html#mapping-protocol-access
2. 'ConfigParser is a misleading name for what is essentially a dict of dict that may never use its parser. Sections are parts of a page of text.
3. idleConf manages 8 pages, organized in 2 groups (default and user) of 4. Each group of 4 represents a 'set of pages' stored in a directory. There is no class for this concept, but some idleConf methods should be methods of the missing class.
4. The 8 pages can also be seen as 4 pairs. Some idleConf methods, especially those getting values, operate on 1 of the 4 pairs. Maybe there should also be a Pair class.

Possibly patching config based on these thoughts is for the future, but I decided that Changes needs a Page class. Each tab works with one page, and should call Page methods. Changes is initialized by ConfigDialog proper and its methods are called by its buttons, not by tabs. I am not a fanatic follower of the 'law of demeter' but the idea of direct access when possible is correct.

Next steps: Write class PageTest. Don't overdo it. The code is simple and just needs inputs testing each branch. (I know, I violated write the tests first. Hmmm. Seeing the code simplify so nicely convinced me that the design was good ;-) Write ChangesTest and pull in and adapt the existing code for the Changes methods to make them work.

For the moment, I say stick with uploads, perhaps until we are ready to remove old code and integrate this into configdialog.py. At some point we might learn how to collaborate via git branches without making PRs. In the meanwhile, right click, save, git apply are easy enough for me.

You asked: Move all IdleConf.userCFg and IdleConf.defaultCfg access to Changes to separate that completely from GUI?

I don't think this would work because the data structures are different. It also seems conceptually wrong. Changes embodies the concept 'Pending changes', which constitute a potential transaction.

If there are repeated pattern of interaction with idleConf that have not already been captured in idleConf methods, we could factor out another class (Config? Options?) in another issue. Key questions: would this make testing easier? would this significantly improve ConfigDialog code?

I will try to look at the extension stuff tomorrow or soon thereafter.

Thanks for pointing out mapping protocol access. That's what I was (poorly) trying to describe in the __getitem__ docstring in v1. It's a really concept.

I worked on the tests. I added some more code to the classes, so I uploaded that too (sorry, but I needed help understanding the functions).

Testing your Page class seemed pretty easy, so hopefully I did it right. I have added more methods to the class though, which I didn't add tests for yet, in case it's not the right direction.

I changed the existing test to use the Changes class. I believe I did what you intended, but I didn't expand it to keys and highlights just in case.

Now, my difficulty. I had trouble trying to figure out the test for 'save_as', so I wanted to show you what I had so far. You don't need to fill it out; I just needed to see if I was on the right track. Writing a test without code was difficult since it involved other classes. So, I copied the existing code into the class to see how it would fit. Observations:

1. The testcfg was a IdleUserConfParser dictionary. I made a dummy config parser to override Save instead of mocking it. I think you prefer this way of doing that.
2. Page needed to know it's 'config_type' so that it (or Changes) could reach into idleConf. self.main on its own didn't know which part of the config files to update.
3. Following Rename README to README.rst and enhance formatting #2, I created a 'save' module within Page. My options were idleConf.userCfg[page.name] (in Changes) or idleConf.userCfg[self.name] (in Page). Based on your LoD comment, I thought Page should know about that and not Changes.
4. Split out save_as into another method called changed, also in Page. I thought it made sense for a Page to say if it's changed.
5. set_user_value is the same as before, although I changed the docstring and it's in Page for the page.name vs self.name reason. However, this still looks really clunky to me.

Questions:

1. Any interest on making Page inherit from defaultdict to simplify additem?
2. I'm not sure about the name for Page, but I haven't come up with an alternate. My reason is that in configdialog, all the create functions are create_page_* and the names on the tabs don't match the config (eg, there's a General tab stored in main). Unless you made that connection on purpose? I was thinking of page more as a GUI item, maybe because of terms like webpage. Maybe the solution is to make the functions create_tab_*?

I still haven't looked at extensions. I should have more time tomorrow.

One note, I found a typo in the current test file. In tearDownModule, it sets idleConf.userCfg to testcfg, but I think the intent was to set it back to the saved value, which is userCfg.

Thanks!

To get moving and unblock other issues, I decided to start with the simplest not-bad change that will work.

The factored out code belongs in config.py. This will separate integration with ConfigDialog into 2 steps.

1. Add code and tests to config and test_config, leaving configdialog untouched.
2. Modify configdialog and tests to import and use the new classes, removing the refactored classes.

We should start with ConfigChanges as a subclass of dict, with added methods as unchanged as possible. No Page class, but the page idea appears, and will later be applied elsewhere. No apply method, as the code only accesses changes but does not modify them. No save_new_config, as that code bypasses changes completely. No set_user_value, as that only access idleConf. I guessed at what delete_section should look like as I don't know where the original code is.

Attached is my untested ConfigChanges.

With step 1 done, I believe step 2 would consist of:

A. add ConfigChanges to the config import.

B. make 'changes' a global (like idleConf), after imports, with
changes = ConfigChanges()
[This could go in config, but CD is its only user.]
Remove self.reset_changed_items in CD.__init__.

C. replace names throughout file
/self.changed_items/changes/
/self.reset_changed_items/changes.clear/
/self.add_changed_items/changes.add_item/
/self.save_all_changed_configs/changes.save_all/

C. Removed renamed methods. (Could do before B.)
D. Replace code moved to .delete_section with a call thereto.

E. Change test_configdialog
If you wrote tests for methods not included in ConfigChanges, they could be added here anyway.

PR2 could be prepared before PR1 is merged, but must not be applied before PR1 is merged and backported, and has to be consistent with final applied version of PR1.

Comments on difficulties and questions.

D1. I will look at test changes where there is a PR to run. I wrote what you changed, but might not have considered what you did.
D2. Moot for the moment.
D3. Ditto.
D4. If not needed, skip for now.
D5. I will be more comfortable changing and presumably improving working code when we have good tests. GetDefaultItems (needs renaming) could save references to page and section dicts.

We are in a bit of a chicken and egg situation when we need better tests to improve code and better code to write better tests. After steps 1 and 2, I want to work on tests. Then we can work on more code changes.

Q1. dict.get is a way to avoid the existence check, and I am thinking of trying it out when tests are in place. It may not have existed when this code was written. I don't know what else DefaultDict adds.
Q2. I was aware of the connection to tab pages, but it is not exact. I decided on 'config_type' for the string argument and local name page for the corresponding dict.

Note: Yes, a bug. I save usercfg so I could restore it. The fix should be a PR for bpo-30780.

I am working on this now.

I've made the first pull request for the changes to config.py. I have to apologize because I really couldn't figure out how to do 'save_all' without including 'set_user_value' since 'set_user_value' updates userCfg with the values from ConfigChanges.

Also, in __init__, you had self.pages, but self itself is the same thing, i.e. self['main'], etc is created in the for-loop, so I couldn't figure out the difference between self and self.pages, except in the context of the Page class.

I kept the code as close to the original as possible. One side effect is that sometimes we have self[page] and sometimes self[config_type].

I also commented out the call to self.save_all_changed_extensions in save_all. In configdialog, save_all_changed_extensions iterates over what would be ConfigChanges['extensions'], with the section = ext_name and item = opt. It calls set_extension_value with each ext_name/opt, but the difference between this and the others is that opt (item) itself is a dictionary with 'name', 'default', and 'var' keys, whereas 'main', 'keys', and 'highlight' have items with one value (at least in set_user_value).

So, if set_user_value stays in ConfigChanges, I believe set_extension_value and save_all_changed_extensions can be copied there as intact also. Neither one uses anything from configdialog directly, just things from ConfigChanges and idleConf.

Thanks!

Whoops, we seems to have partly overlapped (and made some of the same changes). I plan to push my PR sometime tomorrow (it also changes configdialog and revised tests) after looking as yours. Then move on to adding config tests, bpo-30780.

The tests for config need the real config parsers, except for save. But to fully test config, a mock save is needed. I opened bpo-30868 for this.

set_value (set_user_value) accesses neither configdialog nor changes. So it is a function rather than a method, and I could have put it in config as such. I added it as a staticmethod just above save_all instead because it is only used by save_all.

Any other of the now 74 methods of ConfigDialog that are not really methods thereof (don't use self) should also be removed and made module functions, in either module, or possibly changes methods. I just want them tested first, if possible.

I *would* like to put all the extension changes in changes instead of some in a separate structure. I believe the 'hard' part is that key changes have to be handled separately from the non-extension keys. The code was contributed by someone else and once I determined that it worked by manual testing, I left it alone. Testing and moving and possibly rewriting the extension stuff would be a coherent issue and PR.

Sorry about that. When I read msg297804, I thought your intention was for me to work on the PRs. I didn't see your other note until now. It will be good for me to learn from your changes.

As far as mocking Save, I sort of did that (very basically) in my PR by using Func(). It wasn't covering everything because I only added it for 'main'. Do you think I should continue with that in bpo-30868 or did you want to do it a different way?

Terry,

Any interest on using gitter or slack (or something like that) while trying to coordinate work between you, Louie, me, and others?

New changeset 349abd9 by terryjreedy in branch 'master':
bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. (bpo-2612)
349abd9

New changeset edc0342 by terryjreedy in branch '3.6':
[3.6] bpo-30779: IDLE -- Factor ConfigChanges class from configdialog, put in config; test. (GH-2612) (bpo-2625)
edc0342

Cheryl, thank you for reviewing. I am answering the question about bpo-30868 on that issue.

I did not want to immediately test user parser Save() calls in the .save_all test for two reasons.

1. I want to first reconsider the general question what to do about Save in the new issue.

2. The only specification for .save_all calls to .Save is the code itself, and I am not convinced it is correct. I want to review before semi-freezing the code by testing that it does what it does.

Even if it is correct, it will will be tricky to test all paths. For instance, save_all unconditionally call usermain.Save before looking at changes. Then, if changes has changes for usermain, usermain.Save is called again. To test that there are two saves, Func could easily be changed to increment .called instead of setting it to 1.

My guess for the double save is that ConfigDialog *might* write some changes directly to userCfg, so the unconditional save makes sure they are written. But why bypass? If the dialog is cancelled, the 'unconditional' save will not happen, and any directly written changes might affect the current session, but might never be saved to disk. That would seem like a bug.

I have similar questions about the unconditional save of highlights and keys. If all three files are always overwritten, why not push any changes to userCfg and then write all three without bothering about keeping check if there are additional changes?

I am leaving this until I/we have reviewed configdialog while writing tests.

New changeset 24f2e19 by terryjreedy in branch 'master':
bpo-30779: News (bpo-2627)
24f2e19

New changeset 9d8abf3 by terryjreedy in branch '3.6':
[3.6] bpo-30779: News (GH-2627) (bpo-2630)
9d8abf3

I had an error when trying to delete a custom theme. Sorry for not catching this before.

Eventually, we will have tests that would have caught this. I took the opportunity to improve the test for the call.

New changeset 6d13b22 by terryjreedy (csabella) in branch 'master':
bpo-30779: IDLE: fix changes.delete_section calls in configdialog (bpo-2667)
6d13b22

New changeset c017948 by terryjreedy in branch '3.6':
[3.6] bpo-30779: IDLE: fix changes.delete_section calls in configdialog (GH-2667) (bpo-2674)
c017948