IDLE: modernize textview module

[terryjreedy]

BPO	30495
Nosy	@terryjreedy, @taleinat, @csabella
PRs	bpo-30495: IDLE: Modernize textview.py with docstrings and PEP8 names #1839 [3.6] bpo-30495: IDLE: Modernize textview.py with docstrings & PEP8 names (GH-1839) #2102 bpo-30495: IDLE: refactor textview #2283 [3.6] bpo-30495: IDLE: improve textview with docstrings, PEP8 names, … #2496
Files	textview.patch textview.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/terryjreedy'
closed_at = <Date 2021-05-29.04:04:16.209>
created_at = <Date 2017-05-28.03:22:20.490>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'IDLE: modernize textview module'
updated_at = <Date 2021-05-29.04:04:16.208>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2021-05-29.04:04:16.208>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2021-05-29.04:04:16.209>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2017-05-28.03:22:20.490>
creator = 'terry.reedy'
dependencies = []
files = ['46910', '46916']
hgrepos = []
issue_num = 30495
keywords = ['patch']
message_count = 22.0
messages = ['294622', '294623', '294624', '294626', '294636', '294637', '294659', '294661', '294695', '294729', '294800', '294815', '294816', '295694', '296329', '296370', '296383', '297325', '297326', '297491', '297506', '394686']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'taleinat', 'cheryl.sabella']
pr_nums = ['1839', '2102', '2283', '2496']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30495'
versions = ['Python 3.6', 'Python 3.7']

[terryjreedy]

First get issue #.

[terryjreedy]

The initial patch, #1839, adds missing docstrings to textview and changes names to conform to PEP-8. It was originally attached to bpo-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.

[terryjreedy]

Grepping shows that other references to textview objects (other than in test_textview) are to unchanged class and function names.

[terryjreedy]

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.

[csabella]

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.

[terryjreedy]

New changeset 0aa0a06 by terryjreedy (csabella) in branch 'master':
bpo-30495: IDLE: Modernize textview.py with docstrings and PEP-8 names (bpo-1839)
0aa0a06

[terryjreedy]

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.

[terryjreedy]

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

[csabella]

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 help.py 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 parent.bg 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 bpo-29403: Fix mock's broken autospec behavior on method-bound builtin functions #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 help.py, 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 help.py. 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. help.py 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. help.py used self.wm_title. wm_title is aliased to title, so I didn't know if you wanted those changed. In help.py, you also used self.protocal and not self.wm_protocol, so I wasn't sure about it.

[terryjreedy]

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. http://infohost.nmt.edu/tcc/help/pubs/tkinter/web/index.html 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.
   self.bg = '#ffffff'
   self.fg = 'bpo-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 help.py when editing it next.

[csabella]

Thanks for all the info.

I've made another patch for textfile.py with removing ButtonFrame. I've also included test_textfile.py and help_about.py 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 help.py? 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.

Thanks!