First get issue #.

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.

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

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.

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.

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

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.

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.

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.

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.

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!

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.

Old 2, references; my other main bookmark is the tk reference.
https://www.tcl.tk/man/tcl8.6/TkCmd/contents.htm
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.

New changeset ccccf31 by terryjreedy in branch '3.6':
bpo-30495: IDLE: Modernize textview.py with docstrings and PEP-8 names (bpo-1839) (bpo-2102)
ccccf31

Hi Terry,

Were you waiting on me for changes to the patch or would you like me to make a PR? Thanks!

You were waiting on my review of the revised patch.

1. Remove the help_about changes. Reworking help_about is bpo-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\test_help_about.py", 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\test_help_about.py", 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.

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 <report@bugs.python.org>
wrote:

Changes by Cheryl Sabella <chekat2@gmail.com>:

----------
pull_requests: +2331

---

Python tracker <report@bugs.python.org>
<http://bugs.python.org/issue30495\>

---

New changeset 42bc8be by terryjreedy (csabella) in branch 'master':
bpo-30495: IDLE: improve textview with docstrings, PEP-8 names, more tests. (bpo-2283)
42bc8be

New changeset 6f31717 by terryjreedy in branch '3.6':
[3.6] bpo-30495: IDLE: improve textview with docstrings, PEP-8 names, more tests. (GH-2283) (bpo-2496)
6f31717

I have some questions about the final version:

1. ViewWindow still has self.button_ok defined, but it's not used as far as I can tell. Can that be removed?
2. def ok in ViewFrame calls self.parent.destroy. I was wondering if it should be self.parent.ok? Trying to think of the pros and cons of both ways.

Thanks!

I was trying to think of 2 possible future uses of ViewFrame in addition to current embedding in in ViewWindows: embedded in a tab, and tiled in a panel. With no window close [X] buttom, the button in a frame will be needed, so I left it.

Since the button is now [Close] rather than [OK], the function should be close rather than ok.

I copied, rather than cut, before pasting and editing. The Windows button is not needed. The window ok is for the protocol call, but I don't really know if that is needed. Tk apps seem to work without it. I may investigate more someday. Leave it for now.

By calling window destroy directly, the frame close will survive window close being deleted, but not window close being edited to add other actions.

Ping? ISTM this could be considered done.