bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) #2905

csabella · 2017-07-27T00:40:53Z

https://bugs.python.org/issue31004

mlouielu · 2017-07-27T01:48:02Z

Lib/idlelib/configdialog.py

@@ -1849,6 +1849,219 @@ def save_all_changed_extensions(self):
            self.ext_userCfg.Save()


+class FontPage:


class FontPage(object) for new style of class

Good point, but I honestly don't know the preference for this in Python 3. The new style class is for compatibility with Python 2.

https://stackoverflow.com/questions/4015417/python-class-inherits-object#9448136
"In Python 3, apart from compatibility between Python 2 and 3, no. In Python 2, yes."

We are writing 3.6+ code, including using f-strings, so '(object)' is just noise.

terryjreedy

Looks fine so far. My comments are mostly about looking ahead to the next two steps to anticipate possible problems.

terryjreedy · 2017-07-28T04:44:12Z

Lib/idlelib/configdialog.py

@@ -1849,6 +1849,219 @@ def save_all_changed_extensions(self):
            self.ext_userCfg.Save()


+class FontPage:


We are writing 3.6+ code, including using f-strings, so '(object)' is just noise.

terryjreedy · 2017-07-28T05:12:22Z

Lib/idlelib/configdialog.py

@@ -1853,6 +1853,219 @@ def save_all_changed_extensions(self):
            self.ext_userCfg.Save()


+class FontPage:
+
+    def __init__(self, parent, tab_pages, highlight_sample):


At step 2, testing FontPage isolated, passing in Func() when creating an instance of FontPage would work. But the mock could also be set on the instance after creating it. At step 3, testing FontPage live, FontPage will be created by ConfigDialog. highlight_sample will not exist until PageHighlight is called to create and initialize its widgits, first. At that point, it can be retrieved using tab_pages. Lets leave this for now.

Is there anything that would allow us to create highlight_sample outside of both FontPage and ThemePage and let both of them access it? A superclass would seem a bit much, but I don't know if can just be in ConfigDialog and then have self.highlight_sample sent to the 2 classes?

One other thing that I wasn't sure about - whether to send all of tab_pages to FontPage or to just send the frame as the parameter. The main reason is this line in FontPage:

frame = self.tab_pages.pages['Fonts/Tabs'].frame

Since the pages['Fonts/Tabs'] is defined in ConfigDialog.create_widgets invokes TabbedPageSet, I'm not sure if it's correct to reference it within another class.

So, then the init would be def __init__(self, parent, frame, highlight_sample):

and the instantiation would be font_page = FontPage(self.parent, self.tab_pages.pages['Fonts/Tabs'].frame, self.highlight_sample)

Creating a default Text in create_widgets would work for setting the options (in the two pages). However, 'parent' is not a resettable 'option' and it must be the frame the widget is to appear in. So the Text call must be where it is.

I believe you are referencing issues in https://en.wikipedia.org/wiki/Law_of_Demeter.

A ttk notebook is initialized empty. Tabs are added with note.add(someframe, text='tab title'). If we used that, the new classed would be Frame subclasses. I will try a quick experiment with Notebook.

There would still be the issue that the hightlight frame must exist before the font frame is initialized. We can always stick with the current design of calling all the load methods after all the frames are created.

terryjreedy · 2017-07-28T05:12:47Z

Lib/idlelib/configdialog.py

+    def __init__(self, parent, tab_pages, highlight_sample):
+        self.parent = parent
+        self.tab_pages = tab_pages
+        self.highlight_sample = highlight_sample


~~And remove this statement.~~ change my mind for now.

terryjreedy · 2017-07-28T05:33:05Z

Lib/idlelib/configdialog.py

+        self.highlight_sample = highlight_sample
+        self.create_page_font_tab()
+        self.load_font_cfg()
+        self.load_tab_cfg()


Loading immediately instead of waiting until all pages are created should be ok. requires that self.highlight exist, because set_samples is called. Making ConfigDialog pass tab_pages.pages['highlight'].highlight_sample is messy (such a detail should normally be hidden) but makes the creation order requirement obvious. As long as there are no circular dependencies, immediate loading is okay, but I can see why something decided to punt on the issue. (As far as I know, this is the only cross-page dependency.)

terryjreedy · 2017-07-30T02:31:52Z

Lib/idlelib/configdialog.py

-    def var_changed_space_num(self, *params):
-        "Store change to indentation size."
-        value = self.space_num.get()
-        changes.add_option('main', 'Indent', 'num-spaces', value)


This should not have been deleted here or in the original. It was an error in the test that it could still pass.

Sorry about that. I was severely git-challenged yesterday. I thought I had restored everything correctly, but I must have missed this.

with old code disabled.

terryjreedy

Please test on *nix. Otherwise, I believe this is ready to merge.

terryjreedy · 2017-07-30T02:33:48Z

Lib/idlelib/configdialog.py

@@ -1894,7 +1894,8 @@ def create_page_font_tab(self):

        # Create widgets:
        # body and body section frames.
-        frame = self.tab_pages.pages['Fonts/Tabs'].frame
+        # frame = self.tab_pages.pages['Fonts/Tabs'].frame
+        frame = self.parent


frame = self once class is made a frame

terryjreedy · 2017-07-30T02:36:06Z

Lib/idlelib/idle_test/test_configdialog.py

@@ -57,10 +57,12 @@ class FontTest(unittest.TestCase):
    @classmethod
    def setUpClass(cls):
        dialog.set_samples = Func()  # Mask instance method.
+        tracers.attach()


tracers for everything except FontFrame are already attached by ConfigDialog. The new FontPage must leave this true. I am adjusting it to make it do so. All tracers are detached in tearDownModule.

csabella · 2017-07-30T11:50:20Z

Looks on good Ubuntu. Ran the testsuite and the htests.

…tep 1) (pythonGH-2905) The slightly modified tests continue to pass. The General test broken by the switch to Notebook is fixed. Patch mostly by Cheryl Sabella. (cherry picked from commit 9397e2a)

…tep 1) (GH-2905) (#2950) The slightly modified tests continue to pass. The General test broken by the switch to Notebook is fixed. Patch mostly by Cheryl Sabella. (cherry picked from commit 9397e2a)

mlouielu · 2017-08-01T03:25:42Z

Test on MacOS, works well.

terryjreedy · 2017-08-01T04:36:23Z

Thanks for the Mac test. That should tests everything prior to this also.

the-knights-who-say-ni added the CLA signed label Jul 27, 2017

mlouielu reviewed Jul 27, 2017

View reviewed changes

csabella force-pushed the bpo31004 branch from 3a2fd2a to 751e15a Compare July 27, 2017 23:25

terryjreedy reviewed Jul 28, 2017

View reviewed changes

csabella force-pushed the bpo31004 branch from 751e15a to 7681964 Compare July 28, 2017 19:58

csabella added 3 commits July 29, 2017 17:06

bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1)

2257e62

highlight_sample

fdacf6d

New FontPage with changes from 30853

136066b

csabella force-pushed the bpo31004 branch from 7681964 to 136066b Compare July 29, 2017 21:06

csabella added 2 commits July 29, 2017 20:00

Step 2

31a02ad

Step 2 corrections

8aa4bd5

terryjreedy reviewed Jul 30, 2017

View reviewed changes

terryjreedy added 5 commits July 30, 2017 00:16

"Complete Step 2: test new class isolated."

b376e76

"Step 3: Switch to Font Page class, diable test of old code"

382bdbf

Complete Step 3: make sure tests pass, dialog ok,

c14bb1e

with old code disabled.

Step 4: delete old code.

ebe28f8

News blurb.

6b92e75

terryjreedy approved these changes Jul 30, 2017

View reviewed changes

terryjreedy merged commit 9397e2a into python:master Jul 30, 2017

terryjreedy mentioned this pull request Jul 30, 2017

[3.6] bpo-31004: IDLE: Factor out FontPage class from configdialog (s… #2950

Merged

csabella deleted the bpo31004 branch July 30, 2017 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) #2905

bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) #2905

csabella commented Jul 27, 2017 •

edited by bedevere-bot

Loading

mlouielu Jul 27, 2017

csabella Jul 27, 2017

terryjreedy Jul 28, 2017

terryjreedy left a comment

terryjreedy Jul 28, 2017

terryjreedy Jul 28, 2017

csabella Jul 28, 2017

csabella Jul 28, 2017

terryjreedy Jul 28, 2017 •

edited

Loading

terryjreedy Jul 28, 2017

terryjreedy Jul 28, 2017

terryjreedy Jul 30, 2017

csabella Jul 30, 2017

terryjreedy left a comment

terryjreedy Jul 30, 2017

terryjreedy Jul 30, 2017

csabella commented Jul 30, 2017

mlouielu commented Aug 1, 2017

terryjreedy commented Aug 1, 2017

		@@ -1849,6 +1849,219 @@ def save_all_changed_extensions(self):
		self.ext_userCfg.Save()


		class FontPage:

bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) #2905

bpo-31004: IDLE: Factor out FontPage class from configdialog (step 1) #2905

Conversation

csabella commented Jul 27, 2017 • edited by bedevere-bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy Jul 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented Jul 30, 2017

mlouielu commented Aug 1, 2017

terryjreedy commented Aug 1, 2017

csabella commented Jul 27, 2017 •

edited by bedevere-bot

Loading

terryjreedy Jul 28, 2017 •

edited

Loading