classification
Title: IDLE, configdialog: Factor out FontTab class from ConfigDialog
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: 30853 Superseder:
Assigned To: terry.reedy Nosy List: cheryl.sabella, terry.reedy, vstinner
Priority: normal Keywords:

Created on 2017-07-24 08:03 by terry.reedy, last changed 2017-08-07 15:26 by terry.reedy. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2905 merged cheryl.sabella, 2017-07-27 00:40
PR 2950 merged terry.reedy, 2017-07-30 17:38
Messages (15)
msg298933 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-24 08:03
I want to follow the safe way to refactor (from a blog post), rather than the hacker way of refactoring 'in place'.
Step 0: Test code to be refactored.  Finished in 30993.

Step 1: Copy code to be refactored and modify as needed to isolate it from the working original.  For configdialog, add 'class FontPage' below ConfigDialog and copy font/tab methods, now collected together, below that.  For the test, copy class FontTest as FontPageTest and IndentTest as IndentOptionTest. The copies should pass as they will still be testing the original code.  Commit.

Step 2: Modify code as desired.  Modify test copy to test code copy.  For FontPage, this will require new tracer manager (#30853) to attach (activate) callbacks so tests will pass.  Commit.

Step 3: Switch from using original code to using modified code.  For this issue, calling FontPage instead of create_page_font_tab may be enough.  Skip original test; modify new test as needed to pass.  Commit.

Step 4: Once we are sure that we do not need the original code that has been replaced, delete it.  Rerun tests. Commit.

The separate commits will make review easier.  Create or update PRs as desired.
msg299279 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python triager) Date: 2017-07-27 00:42
PR is for step 1 since VarTrace is needed for step 2.
msg299362 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-28 05:05
I may have changed the Font group since the copy was made.  So unless merges since are carefully checked, the configdialog part of Step 1 should be redone.  I think the latter would be easier.

Since #30853 might be merged tomorrow, I will 'freeze' the font/indent code unless I hear otherwise, or there is an unexpected delay.  When this is merged, I will do the same for the general part.  And so on.

In Step 2, delete 'Modify code as desired.' I must have forgotten the synchonization problem.  We don't want to modify the copy until it can be htested.  As soon as the copy *is* changed, the original is no longer a complete backup.

I thought of saying that FontPage should subclass Frame, like we have done before.  But TabbedPageSet creates a Frame for each page name.  So ConfigDialog, the only user, is expected to pass names, then retrieve the blank frames to fill in.  We could change that, but not now unless we really need to.  It should be possible to replace the blank frame, but there are at least two references to replace.
msg299367 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-28 06:06
For steps 2 and 3, setUpClass will have to create or retrieve a reverence to the page class instance.  For page functions, 'dialog' will have to be changed to 'self.page'.  For hightlight and key pages, I will write the tests using 'self.page', which would start as  a synonym for 'dialog' (set in setUpClass).  I might convert GeneralTest before you copy it.
msg299414 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python triager) Date: 2017-07-28 17:33
OK, once 30853 is merged, I'll recopy the font code into the class.

On Fri, Jul 28, 2017 at 2:06 AM, Terry J. Reedy <report@bugs.python.org>
wrote:

>
> Terry J. Reedy added the comment:
>
> For steps 2 and 3, setUpClass will have to create or retrieve a reverence
> to the page class instance.  For page functions, 'dialog' will have to be
> changed to 'self.page'.  For hightlight and key pages, I will write the
> tests using 'self.page', which would start as  a synonym for 'dialog' (set
> in setUpClass).  I might convert GeneralTest before you copy it.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue31004>
> _______________________________________
>
msg299432 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-28 19:51
When replying by email, snip the quote.
msg299452 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-29 04:30
I will push Notebook when CI passes.  We can then do Step 2 for class FontPage(Frame).  Consider the font code frozen for now.  Pass 'note', not the dialog, as the parent.  This makes tab switching work better.

Create the highlight frame first and pass it as a second argument to FontPage.  Adding set_samples to load_font_config should solve the initial font problem.
msg299489 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python triager) Date: 2017-07-30 00:15
I'm pushing step 2 with an error in the test.  It's on the keydown in test_fontlist_key.  I just didn't want to hold you up from looking at the rest of it because of one test.

I did run into a bunch of other issues/questions while doing this.  I didn't add tracers.attach() to the FontPage class.  So, I had to call it in the FontPageTest.  Well, I then also added a detach so it wouldn't carry to the other tests and of course all the other tests broke.  So, I added setup/teardown of tracers in all the test classes.  I don't know if that was the right way to go or if adding tracers.attach to FontPage and then just leaving everything attached would be ok.

Also, I tried to minimize the change from `d=dialog` to `d = self.page`, so I kept the name `d`.

Adding set_samples to load_font_cfg upped the called count to 4 in test_load_font_cfg because the tracers are attached when it's called.  I think you mentioned that test needs to be changed with them detached, but I wasn't sure if you wanted that changed now or later.

In FontPage, I tried not to change code yet, so I keep the name `parent` even though `note` will be passed in.
msg299490 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-30 02:03
I am going to work on this now.  Tracers should be on for the duration of the test class except for the load test.  d = self.page is what I planned initially.

Argument note is bound to parameter parent and that will remain the name.

The one error I see in PageFont before testing is adding or leaving "frame = Frame(self.parent)".  Now that FontPage is a frame, 'frame = self' (we can later replace 'frame' with 'self' as the parent argument in calls that follow).
msg299503 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-30 17:34
New changeset 9397e2a87ed6e0e724ad71a0c751553620cb775e by Terry Jan Reedy (csabella) in branch 'master':
bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (#2905)
https://github.com/python/cpython/commit/9397e2a87ed6e0e724ad71a0c751553620cb775e
msg299508 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-30 19:00
New changeset 7582226a92481ca63dedbfe14ef465d1349d66a9 by Terry Jan Reedy in branch '3.6':
[3.6] bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (GH-2905) (#2950)
https://github.com/python/cpython/commit/7582226a92481ca63dedbfe14ef465d1349d66a9
msg299514 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-30 19:35
At last! On to the next one.
msg299835 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-08-07 09:25
> New changeset 9397e2a87ed6e0e724ad71a0c751553620cb775e by Terry Jan Reedy (csabella) in branch 'master':
> bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) (#2905)

This change introduced reference leaks in test_idle: see bpo-31130.
msg299836 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-08-07 09:30
By the way, I suggest to not backport refactoring changes to stable branches like Python 3.6. Any refactoring is a risk of regressions. Well, it's just an advice, Terry knows IDLE much better than me ;-) It's up to you to decide to backport or not such change.
msg299851 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-08-07 15:26
Idlelib is a bit exceptional: see PEP 434.  Except for one test and the news files, I keep idlelib identical in 3.6 and 3.7.  This makes all backports, *including new tests*, trivial and overall reduces the risk of regression (unless IDLE 3.6 were abandoned completely).  The risk of regression is why I have only touched 3.6/7 since last Fall, why I am increasingly strict about testing, and why I will manually test everything at least a few days before the next release candidate.
History
Date User Action Args
2017-08-07 15:26:32terry.reedysetmessages: + msg299851
2017-08-07 09:30:49vstinnersetmessages: + msg299836
2017-08-07 09:25:28vstinnersetnosy: + vstinner
messages: + msg299835
2017-07-30 19:35:34terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg299514

stage: needs patch -> resolved
2017-07-30 19:00:53terry.reedysetmessages: + msg299508
2017-07-30 17:38:24terry.reedysetpull_requests: + pull_request2999
2017-07-30 17:34:27terry.reedysetmessages: + msg299503
2017-07-30 05:55:02terry.reedylinkissue31050 dependencies
2017-07-30 02:03:17terry.reedysetmessages: + msg299490
2017-07-30 00:15:29cheryl.sabellasetmessages: + msg299489
2017-07-29 04:30:31terry.reedysetmessages: + msg299452
2017-07-28 19:51:54terry.reedysetmessages: + msg299432
2017-07-28 17:33:21cheryl.sabellasetmessages: + msg299414
2017-07-28 06:06:44terry.reedysetmessages: + msg299367
2017-07-28 05:05:16terry.reedysetmessages: + msg299362
2017-07-27 00:42:07cheryl.sabellasetmessages: + msg299279
2017-07-27 00:40:55cheryl.sabellasetpull_requests: + pull_request2956
2017-07-26 18:59:01terry.reedysetnosy: + cheryl.sabella
2017-07-26 18:58:42terry.reedysetdependencies: + IDLE: configdialog -- factor out VarTrace class
2017-07-24 08:03:45terry.reedycreate