IDLE: Document, fix, and complete configdialog font tests

[terryjreedy]

BPO	30993
Nosy	@terryjreedy, @ned-deily, @mlouielu, @csabella
PRs	bpo-30981: IDLE: Add more configdialog font page tests. #2805 bpo-30993: IDLE - Improve configdialog font page and tests.  #2818 [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (G… #2826 bpo-30993: IDLE - Improve configdialog font page and tests. #2831 [3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH… #2834

^{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 2017-07-24.06:51:31.735>
created_at = <Date 2017-07-22.19:07:43.732>
labels = ['expert-IDLE', 'type-feature', '3.7']
title = 'IDLE: Document, fix, and complete configdialog font tests'
updated_at = <Date 2017-07-24.06:51:31.734>
user = 'https://github.com/terryjreedy'

bugs.python.org fields:

activity = <Date 2017-07-24.06:51:31.734>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2017-07-24.06:51:31.735>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2017-07-22.19:07:43.732>
creator = 'terry.reedy'
dependencies = []
files = []
hgrepos = []
issue_num = 30993
keywords = []
message_count = 14.0
messages = ['298862', '298878', '298882', '298884', '298888', '298902', '298907', '298908', '298909', '298913', '298920', '298926', '298928', '298930']
nosy_count = 4.0
nosy_names = ['terry.reedy', 'ned.deily', 'louielu', 'cheryl.sabella']
pr_nums = ['2805', '2818', '2826', '2831', '2834']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30993'
versions = ['Python 3.6', 'Python 3.7']

[terryjreedy]

This follows bpo-30981 and the comments on PR 2805 after the close notice.

• Causal chains in a directed acyclic graph link user actions to provisional entries in changes and and changes in the example displays. Explain these better in the docstring. Each font test function tests pieces of the graph.
  *test_font_set should run even if not run first.

[terryjreedy]

Patch ready for test on linux and review.

[csabella]

Works on linux. I just had one question on github about a test for sizelist and moving the initialization of the Tk variables into setUp.

I also deleted a second comment that was just a wrong comment, but it's led me to another question.

For test_indent_scale, you have the widget test and the changes test in the same place. I know you mentioned that you had thought about this before, but now that you have the widget tests for the font name, font bold, (and soon) font size, would there be any reason to split the test_font_set tests into the same place where each of the widgets is tested? That way, all the pieces of the graph flow would be in one place for each widget/Tk Variable/trace function.

[terryjreedy]

I am thinking that the tab title should be 'Font/Indent' rather than 'Font/Tabs' to avoid the confusion between the simulated file-folder tab that we click on and the tab key that indents. Opinions?

But maybe the indent widget will be moved, eliminating the issue, or maybe more will be added, as part of converting 'extensions' to features, requiring a different rename. 'Reformat paragraph, also affect text layout.

[terryjreedy]

Yes, test_font_set could be split up. But then the setup would have to be repeated for each, and it would be harder to verify that each .set() call was being tested the same way.

The event graph for indent_scale is a simple linear chain, and the simple liner test mirrors that.

The combined test of bold_toggle and set_samples is not what I wanted, but I prefer it to the alternative of depending on an undocumented internal detail of the tcl wrapper maker (_tkinter, I suspect).

[csabella]

+1 on the Font/Indent name. And I agree that the indent widget would make more sense on the General tab, or another name, like Editor/Shell Preferences.

Another thought I had was Fonts should maybe be part of a Theme. At the theme level (not at the tag level), define font name/font size and then have font bold at the tag level. Spyder has that, so the function names (IDLE tag of definition) are bolded and the rest aren't. It's subtle, but I find it pleasing to work with because function/class/method names stand out. That would be after everything else though.

Another thought I had with the cycle in fonts: A wdiget is clicked on, which calls the tkinter command callback. But, the Tk variable is also updated which calls the trace function. Those are the two paths. What if the trace function called set_samples? Then font_bold and font_size widgets wouldn't need the command argument. I think opt_menu_highlight_target does that now because the command is commented out.

instead of:
click bold-toggle ~> set font_bold -> var_changed_font
|
\---> call set_sample

this:
click bold_toggle -> set font_bold -> var_changed_font -> set_sample

[terryjreedy]

New changeset 07ba305 by Terry Jan Reedy in branch 'master':
bpo-30993: IDLE - Improve configdialog font page and tests. (bpo-2818)
07ba305

[terryjreedy]

A big issue with changing to tagging individual elements is back compatibility. Besides which, if the font is not bold, I cannot imaging bolding anything other than the definition names. This also seems to venture beyond 'keep IDLE simple for beginners'. How about a new option to just bold definition names?

Factoring out the call to set_samples to one place is a great idea that simplifies code and tests. I did it and the htest dialog worked just fine. The tests also passed as were, but need reworking. I am doing that now. The result will be one FontTest class that tests that each widget sets its var, tests that var setting adds changes and calls set_samples, and tests set_samples.

[terryjreedy]

New changeset 5aa3bf0 by Terry Jan Reedy in branch '3.6':
[3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH-2818) (bpo-2826)
5aa3bf0

[terryjreedy]

2nd and presumably last PR for this issue: please verify on linux.