Title: IDLE: modernize textview module
Type: enhancement Stage: patch review
Components: IDLE Versions: Python 3.7, Python 3.6
Status: open Resolution:
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: csabella, terry.reedy
Priority: normal Keywords: patch

Created on 2017-05-28 03:22 by terry.reedy, last changed 2017-06-19 22:22 by csabella.

File name Uploaded Description Edit
textview.patch csabella, 2017-05-29 13:26
textview.patch csabella, 2017-05-30 23:49
Pull Requests
URL Status Linked Edit
PR 1839 merged csabella, 2017-05-28 03:23
PR 2102 merged terry.reedy, 2017-06-11 07:37
PR 2283 open csabella, 2017-06-19 22:16
Messages (17)
msg294622 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 03:22
First get issue #.
msg294623 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 04:18
The initial patch,, adds missing docstrings to textview and changes names to conform to PEP 8.  It was originally attached to #30290. I changed the bpo # in the title to 30495.  We will see if Belvedere bot corrects the linkage.

Cheryl: I forgot to mention something very important.  When we change externally accessible names  -- those of functions, classes, and attributes -- in module xyz (here textview), we must grep idlelib *recursively* to find modules and test modules that import xyz and then check all accesses to names within xyz to discover and fix any that we have broken.  (If everything were perfectly tested, discovery would be trivial: run test_idlelib.  But it is not, so we must grep.)

In this case, the change from textview.TextViewer.textView to textview.TextViewer.textview broke the  two copies of
       dialog._current_textview.textView.get('1.0', '1.end'))
in test_help_about.  Running test_idle, which should always be done before IDLE commits, would have caught this.  When Appveyor ran test_idle, it properly failed.  I am mystified the it did not in the Travis run.

I will change the attribute name to 'text' both to avoid confusion with the module name and since the value is a Text instance.  I will also change all the new uses of 'textview' as an attribute.
msg294624 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 04:23
Grepping shows that other references to textview objects (other than in test_textview) are to unchanged class and function names.
msg294626 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 04:43
I plan to merge when checks pass.

Not reusing module names avoids noise when grepping for a module name ;-).  A final search for 'textview', match case, whole word, gave 18 hits, which were easy to check.
msg294636 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-28 10:08
Thanks Terry!

I had done a search in Spyder using "textView" and only found those files that I changed, but now I see what I did wrong.  If I do it within a different git branch that isn't up to update with the master, it won't show up, but if I do it in the proper branch, it does show up.  I'm learning git, so now I know to be more careful when searching.  I should grep it too, just to see how git and grep work together.

I ran the test on the one file I changed, but now I know to run it on the whole test suite.  I should have realized that.  

Thanks again for all your patience.
msg294637 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 10:50
New changeset 0aa0a06e8b719533aefd175a5716f1698f474052 by terryjreedy (csabella) in branch 'master':
bpo-30495: IDLE: Modernize with docstrings and PEP8 names (#1839)
msg294659 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 20:00
What I have learned:
1. As with hg, the local master should be updated at least at the beginning of each work session, and python.exe rebuilt.
2. Patches to master should be developed with all 'other' files updated.  For completely new branches, this is automatic.  Branches created from PRs start out of sync and should be updated.

One can often, perhaps even 'usually', get away with not doing the above, but occasional mysterious glitches can occur.  Example: when I created a branch from your bdb docstring PR, 9 days later, and did not update, 'import re' failed in the new branch.  Problem disappeared after update.

By 'grep', I mean any multi-file search, starting in a specified directory, such as IDLE's 'Find in Files', not the specific unix program.  I develop code and test for x by repeatedly running just 'test_x', but grepping and running test_idle before commit is essential.  As we expand test files, test_idle will become more and more useful.
msg294661 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-28 20:29
I believe you said elsewhere that you would like to try the refactoring that splits a window+frame class into a window class and a frame class. It would be great if you became good at doing this.

If the current class name is used externally, I might consider reusing it for the window class.  But since the external interface for textview is the two wrapper functions, 'TextView' is not used externally.

F:\dev\cpython\lib\idlelib\idle_test\ 3: Since all methods and functions create (or destroy) a TextViewer, which
F:\dev\cpython\lib\idlelib\idle_test\ 31: # If we call TextViewer or wrapper functions with defaults
F:\dev\cpython\lib\idlelib\idle_test\ 36: class TV(tv.TextViewer):  # Used in TextViewTest.
F:\dev\cpython\lib\idlelib\idle_test\ 73: # Call TextViewer with modal=False.
F:\dev\cpython\lib\idlelib\idle_test\ 88:         self.assertIsInstance(view, tv.TextViewer)
F:\dev\cpython\lib\idlelib\idle_test\ 93:         self.assertIsInstance(view, tv.TextViewer)
F:\dev\cpython\lib\idlelib\idle_test\ 113: # Call TextViewer with _utest=True.
F:\dev\cpython\lib\idlelib\ 895:             # XXX KBK 27Dec07 use TextViewer someday, but must work w/o subproc
F:\dev\cpython\lib\idlelib\ 11: class TextViewer(Toplevel):
F:\dev\cpython\lib\idlelib\ 78:     """Create TextViewer for given text.
F:\dev\cpython\lib\idlelib\ 87:     return TextViewer(parent, title, text, modal, _utest=_utest)
F:\dev\cpython\lib\idlelib\ 91:     """Create TextViewer for text in filename.
F:\dev\cpython\lib\idlelib\ 116:     run(TextViewer)

So lets try TextviewWindow and TextviewFrame.  Make the obvious substitutions, except in the PyShell and line 78 and 91 comments, 'TextViewer' should become a generic 'text viewer' that would apply to TextviewFrame if that became the main entry point.

If you want to show me an incomplete patch, you can still upload diffs here.
msg294695 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-29 13:26
I've attached a patch for textview for your review.  Thanks for suggesting that.  I still need to work on test_textview.

Some comments/questions:
1.  I've made the Frame classes as you suggested.  I had also made a class for TextviewText (as has), but it only had an __init__, so I removed it.
2.  I converted some of the tkinter imports to ttk.  Text didn't have a direct replacement, so I left it.  I didn't know if I should change it to NoteBook?
3.  Since Text wasn't converted, I left bg and fg as is. Is it correct to use and parent.fg to refer to the values in the calling class?
4.  I also bind the button to parent.ok in ButtonFrame.  With this and #3, my question is probably more about my use of parent. to access an attribute/function of the calling class.  It doesn't feel like I'm doing it right.
5.  I changed the inheritance calls to use super().  If you prefer not to use that, I can change it back.
6.  In, instead of using widget.config calls for setup, you used the dictionary access, so I changed those here.  For example, text['state'] = 'disabled' instead of self.text.config(state=DISABLED).
7.  And, yes, I also changed the constants.  I did it to match  I can change it back if it's an issue.
8.  I changed the string formatting used on self.geometry.  I had to look up the usage, so I thought this would be more self-documenting.
9. used grid instead of pack.  I kept this as pack, but wanted to ask if that was something else you wanted to be done?
10. used self.wm_title.  wm_title is aliased to title, so I didn't know if you wanted those changed.  In, you also used self.protocal and not self.wm_protocol, so I wasn't sure about it.
msg294729 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-30 00:41
Responses (without yet testing the code):

1. Generally, the only new subclass needed is for the master frame.  Existing Toplevel derivatives may have a single frame in the toplevel, which should become an instance of a new subclass, in this case TextviewFrame.  Some Toplevels contain multiple widgets, which will need to be moved into a new master frame.  The idea is that a single master frame can easily be put elsewhere.  For testing, it can go into the test root window.  For future IDLE, master frames might go on notebook tabs.

So we don't need the new ButtonFrame class.  I suspect that we do not even need the current button frame containing just one button.  But removing that can be deferred.

2. is a good reference for 8.5, with only a few errors I have seen.  The ttk widgets are listed from 28 on.  Notebooks are for the future.

3. Yes, but the following should be moved into TextviewFrame.
         # TODO: get fg/bg from theme. = '#ffffff'
         self.fg = '#000000'

4. 'Containing widget', rather than 'calling class'.  ButtonFrame should go away.

5. My main objection to super(), that the abbreviated form does not work in 2.x, does not matter now that I have mostly stopped backporting to 2.7. 

6&7. I prefer the newer, more Pythonic, subscript access and, as I said before, string literals. There main issue is making isolated changes in untested code versus doing the same in well-tested code as part of refactoring and line-by-line examination.

8. I generally prefer .format to %.  For 3.6+, we now have the option of f-strings.  The expressions for x and especially y are a bit too long to directly include, but temp vars solve that. 

+        x = parent.winfo_rootx() + 10,
+        y = parent.winfo_rooty() + (10 if not _htest else 100)))
+        self.geometry("=750x500+{x}+{y}")

9. Grid is newer than pack, and at least some tk experts recommend using it in preference to pack.  (The reference above only covers gridding.  However, change is not necessary for the first refactoring.

Are you aware that running gui code files runs a visual sanity check that brings up an instance of the window?  One can quickly check the effects of changing gui code.

10. Synonyms are a nuisance, especially if one forgets that they are synonyms.  I would prefer to consistently use the attribute names without the 'wm_' prefix.  I will try to remember to fix when editing it next.
msg294800 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-05-30 23:49
Thanks for all the info.

I've made another patch for with removing ButtonFrame.  I've also included and in the patch.

I had worked on help_about yesterday, so if you don't like the button classes, I may need to undo it.  However, since they do a lot of 'stuff' maybe it would be similar to the HelpText class in  It seems more readable to me compared to when I first looked at the file.  Although, the place_widgets is still messy.

2: Need to bookmark and spend time with this.

For #5 and #8, it sounds like using f-strings is OK even though it only is 3.6+, so then super() should be OK too?

9: Yes, I've been doing that to test all my changes.  It's nice to use since at times, I've lost the window entirely.  And I get feedback in the console with the line error when it doesn't work.  I also get to play with the settings to understand what something does.   :-)

New questions:
1.  In help_about, I converted to all ttk widgets, so fg and bg don't work anymore.  Should I try to add ttk.Style?  For me, the dialog looks the same color-wise as it did before.
2.  In help_about, there was an extra frame (frame_background).  It looked the same with it and without it, so I removed it.  Is that OK?
3.  Forgot to ask yesterday, but the code with `self.text = text = ????` (and all the others with double assignment).  I understand that the self.text is for unittest, but is there a reason to have both self.text and text?  Just trying to understand so I know when to apply it and when not to.

msg294815 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-31 06:05
1. I prefer to not deal with ttk styles yet, unless forced, and we don't seem to be here.
2. Yes, I wanted to get rid of that useless frame as part of refactoring.
3. 'self.x = x = expr' is a semi-common idiom.  A local name is nice when there are multiple local references to the object.  Dereferencing a local name is faster than an attribute, though it may not be enough to worry about unless there is a loop.  No having so many 'self.' prefixes makes the code less cluttered and probably easier to read. The attribute is obviously needed to (directly) refer to the widget outside the method, whether in other methods of the class or outside functions such as in tests. You have probably noticed expression statements, like "Widget(parent, opt=val).grid(...), where no Python binding is kept.
msg294816 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-05-31 06:20
Old 2, references; my other main bookmark is the tk reference.
It is harder to read at first, but more complete, accurate, and up to date.  Unfortunately, even it does not specify all the OS-specific divergences.
msg295694 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-11 08:16
New changeset ccccf3156f8f5cfb820c7deac05beba9f51ec51c by terryjreedy in branch '3.6':
bpo-30495: IDLE: Modernize with docstrings and PEP8 names (#1839) (#2102)
msg296329 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-06-19 13:08
Hi Terry,

Were you waiting on me for changes to the patch or would you like me to make a PR?  Thanks!
msg296370 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-06-19 19:30
You were waiting on my review of the revised patch.

1. Remove the help_about changes.  Reworking help_about is #24813.  I just reviewed Mark Roseman's patch.  I do not want the button frames because I would like to get rid of the buttons.  Besides that, I don't think the classes give enough benefit.  If I wanted to keep the buttons, create_py/idle/button functions might be considered.  You can attach to that issue a minimal change patch or PR that splits off AboutFrame from AboutDialog (the window).  Leave the latter name alone for now to minimize changes needed in other files.  We can then consider adding pieces of what Mark did one at a time.

2. I downloaded the patch, deleted help_about changes, applied, and tested.

Test_help_about needs two  insertions of '.textframe'  to solve
ERROR: test_file_buttons (idlelib.idle_test.test_help_about.LiveDialogTest)
Test buttons that display files.
Traceback (most recent call last):
  File "f:\dev\3x\lib\idlelib\idle_test\", line 67, in test_file_buttons
    dialog._current_textview.text.get('1.0', '1.end'))
AttributeError: 'TextviewWindow' object has no attribute 'text'

ERROR: test_printer_buttons (idlelib.idle_test.test_help_about.LiveDialogTest)
Test buttons whose commands use printer function.
Traceback (most recent call last):
  File "f:\dev\3x\lib\idlelib\idle_test\", line 48, in test_printer_buttons
    dialog._current_textview.text.get('1.0', '1.end'))
AttributeError: 'TextviewWindow' object has no attribute 'text'

Make a PR this time so I can review a side-by-side diff and retest.
msg296383 - (view) Author: Cheryl Sabella (csabella) * Date: 2017-06-19 22:22
Thanks, Terry.  I've pulled the textview changes out and made a PR.  I know
you had said there can be more than one PR for a bug, so I did it under
this number.  Hope that was the right thing to do.

I'll take a look at help_about separately.

On Mon, Jun 19, 2017 at 6:16 PM, Cheryl Sabella <>

> Changes by Cheryl Sabella <>:
> ----------
> pull_requests: +2331
> _______________________________________
> Python tracker <>
> <>
> _______________________________________
Date User Action Args
2017-06-19 22:22:16csabellasetmessages: + msg296383
2017-06-19 22:16:58csabellasetpull_requests: + pull_request2331
2017-06-19 19:30:40terry.reedysetmessages: + msg296370
2017-06-19 13:08:33csabellasetmessages: + msg296329
2017-06-11 08:16:57terry.reedysetmessages: + msg295694
2017-06-11 07:37:18terry.reedysetpull_requests: + pull_request2156
2017-05-31 06:20:24terry.reedysetmessages: + msg294816
2017-05-31 06:05:25terry.reedysetmessages: + msg294815
2017-05-30 23:50:00csabellasetfiles: + textview.patch

messages: + msg294800
2017-05-30 00:41:25terry.reedysetmessages: + msg294729
stage: needs patch -> patch review
2017-05-29 13:26:48csabellasetfiles: + textview.patch
keywords: + patch
messages: + msg294695
2017-05-28 20:29:31terry.reedysetmessages: + msg294661
stage: patch review -> needs patch
2017-05-28 20:00:23terry.reedysetmessages: + msg294659
2017-05-28 10:50:57terry.reedysetmessages: + msg294637
2017-05-28 10:08:31csabellasetmessages: + msg294636
2017-05-28 04:43:40terry.reedysetmessages: + msg294626
2017-05-28 04:23:30terry.reedysetmessages: + msg294624
2017-05-28 04:19:00terry.reedysetmessages: + msg294623
2017-05-28 03:23:39csabellasetpull_requests: + pull_request1924
2017-05-28 03:22:20terry.reedycreate