classification
Title: IDLE: Document, fix, and complete configdialog font tests
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: cheryl.sabella, louielu, ned.deily, terry.reedy
Priority: normal Keywords:

Created on 2017-07-22 19:07 by terry.reedy, last changed 2017-07-24 06:51 by terry.reedy. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2805 cheryl.sabella, 2017-07-22 19:33
PR 2818 merged terry.reedy, 2017-07-22 23:54
PR 2826 merged terry.reedy, 2017-07-23 16:22
PR 2831 merged terry.reedy, 2017-07-23 20:46
PR 2834 merged terry.reedy, 2017-07-24 04:33
Messages (14)
msg298862 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-22 19:07
This follows #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.
msg298878 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-22 22:33
Patch ready for test on linux and review.
msg298882 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-23 00:26
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.
msg298884 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 02:30
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.
msg298888 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 04:34
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).
msg298902 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-23 12:41
+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
msg298907 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 16:20
New changeset 07ba305a4c169e017e076e490a173a6f9b95b38e by Terry Jan Reedy in branch 'master':
bpo-30993: IDLE - Improve configdialog font page and tests.  (#2818)
https://github.com/python/cpython/commit/07ba305a4c169e017e076e490a173a6f9b95b38e
msg298908 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 17:06
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.
msg298909 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 18:18
New changeset 5aa3bf041de5ee90ccbfcff103dcf3e54c5af237 by Terry Jan Reedy in branch '3.6':
[3.6] bpo-30993: IDLE - Improve configdialog font page and tests.  (GH-2818) (#2826)
https://github.com/python/cpython/commit/5aa3bf041de5ee90ccbfcff103dcf3e54c5af237
msg298913 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-23 20:49
2nd and presumably last PR for this issue: please verify on linux.
msg298920 - (view) Author: Cheryl Sabella (cheryl.sabella) * (Python committer) Date: 2017-07-24 00:31
The tests pass on linux and the htest looks good.  Thank you for making all these changes.  It seems much clearer now, but I do want to spend more time reading the code to understand it.  Looks like you grouped all the font-related functions, so they are ready to become a Class?  The only part I couldn't figure out is how to update highlight_sample since it wouldn't be in the class.

With the backwards compatibility, I thought maybe you have an idea about how to make the changes compatible because of the issue that moves the extensions.  I can see where that would be a problem as far as making changes is concerned.

Maybe I'll look at the creating tests for one of the other tabs next?
msg298926 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-24 04:18
New changeset 77e97ca9ff6f3dbbf98b89b4103c46b43eef5642 by Terry Jan Reedy in branch 'master':
bpo-30993: IDLE - Improve configdialog font page and tests. (#2831)
https://github.com/python/cpython/commit/77e97ca9ff6f3dbbf98b89b4103c46b43eef5642
msg298928 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-24 04:38
Ned, in a comment on PR2826, Louie reported "The patch unittest work on Linux, but on MacOS, I get test.support.ResourceDeined: cannot run without OS X gui process, it is wierd, I'm inside the GUI mode.

Also, I'm not sure if this only on MacOS (is font problem or what), when checking bold box, some font won't render to bold (I've check that this may not relative to patch, in older commit have this problem, too)."

Have you run gui tests, and in particular, IDLE tests on OSX lately?  Is the OSX code in test.support for GUI required still working?
msg298930 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2017-07-24 06:50
New changeset 1daeb259799d0664c9453a3bd8e80411e65b52c9 by Terry Jan Reedy in branch '3.6':
[3.6] bpo-30993: IDLE - Improve configdialog font page and tests. (GH-2831) (#2834)
https://github.com/python/cpython/commit/1daeb259799d0664c9453a3bd8e80411e65b52c9
History
Date User Action Args
2017-07-24 06:51:31terry.reedysetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-07-24 06:50:31terry.reedysetmessages: + msg298930
2017-07-24 05:11:40ned.deilysetnosy: + ned.deily
2017-07-24 04:38:07terry.reedysetmessages: + msg298928
2017-07-24 04:33:56terry.reedysetpull_requests: + pull_request2881
2017-07-24 04:18:28terry.reedysetmessages: + msg298926
2017-07-24 00:31:55cheryl.sabellasetmessages: + msg298920
2017-07-23 20:49:02terry.reedysettitle: IDLE: Document and fix configdialog font tests. -> IDLE: Document, fix, and complete configdialog font tests
messages: + msg298913
stage: needs patch -> patch review
2017-07-23 20:46:08terry.reedysetpull_requests: + pull_request2880
2017-07-23 18:18:29terry.reedysetmessages: + msg298909
2017-07-23 17:06:29terry.reedysetmessages: + msg298908
2017-07-23 16:22:29terry.reedysetpull_requests: + pull_request2878
2017-07-23 16:20:10terry.reedysetmessages: + msg298907
2017-07-23 12:41:13cheryl.sabellasetmessages: + msg298902
2017-07-23 04:34:07terry.reedysetmessages: + msg298888
2017-07-23 02:30:43terry.reedysetmessages: + msg298884
2017-07-23 00:26:39cheryl.sabellasetmessages: + msg298882
2017-07-22 23:54:17terry.reedysetpull_requests: + pull_request2871
2017-07-22 22:33:21terry.reedysetnosy: + louielu, cheryl.sabella
messages: + msg298878
2017-07-22 19:34:38terry.reedylinkissue30780 dependencies
2017-07-22 19:33:00cheryl.sabellasetpull_requests: + pull_request2869
2017-07-22 19:07:43terry.reedycreate