bpo-34548: IDLE: use configured theme colors in TextView #9008

taleinat · 2018-08-30T12:45:45Z

https://bugs.python.org/issue34548

taleinat · 2018-09-22T17:21:43Z

@csabella perhaps you'd like to review this?

csabella · 2018-09-22T23:39:44Z

Lib/idlelib/colorizer.py

@@ -32,10 +32,10 @@ def make_pat():
 idprog = re.compile(r"\s+(\w+)", re.S)

 def color_config(text):  # Called from htest, Editor, and Turtle Demo.


This inline comment wouldn't be true anymore after this change.

csabella · 2018-09-22T23:51:04Z

@taleinat Thanks for asking me to take a look at this PR.

This is certainly an interesting change to consider, especially with issue33397 and Terry's idea to use that font resizing class for all windows. Since the TextView is currently used for help_about and the help button on config dialog, I'm lumping it in with the other Help screens (and the current change addressed in issue33397), but I also think Terry wanted to expand the use of TextView into other areas, which makes me think consistency across the app for fg and bg would also be a goal.

When we notice something like this that should be changed, Terry usually opens a master bug tracker issue to make sure any related changes are also done. For example, this doesn't address the help.py window because it's not a TextView, but perhaps the overarching project would be to make all the colors and all the fonts consistent across the app.

Also, I noticed that this color_config method was just added two years ago. I'm not familiar with all the intended changes for the colorizer, but it seems like this function doesn't really fit in with the other comment in the docstring for that method - 'Should be called whenever ColorDelegator is called. ' In the case of adding this to TextView, you aren't calling ColorDelegator. Not that this affects your change because I think you are using the method in the way it was intended, but the color_config method just doesn't seem to be defined quite exactly in the way it should be used. Either it needs to change (in the docstring) for your usage or else you shouldn't be using it. I hope that makes sense.

My thought without having Terry's input is the color_config should live somewhere else. It's setting up a text widget, not really being used as part of the delegator, but maybe that's a wrong assumption because I haven't spent a lot of time with it.

A lot of the recent work on IDLE has been small changes (sometimes behind the scenes) to get the modules ready for a user-facing change. Based on that, even though your change is a good idea, I'm not sure if it should be step one or if colorizer needs some work first. Or if a new text_config module will be added that will handle both fg/bg color and resizing for text.

csabella · 2018-09-22T23:55:27Z

As far as the project as it is now, I do think you should add tests for the colors. If this gets added to other modules for consistency, then the tests would be able to show that consistency, that is, before the change is made, the assert would fail and then after use color_config to set the color, the assert would pass.

Lib/idlelib/textview.py

csabella · 2018-09-23T00:00:18Z

Misc/NEWS.d/next/IDLE/2018-09-22-20-25-07.bpo-34548.7pBzjg.rst

@@ -0,0 +1 @@
+Make TextView use the text colors from the main IDLE configuration.


Terry will probably adjust the blurb to be clearer when he merges this (he has uncanny knack for writing succinctly and accurately). However, I think it should say something more like 'use the colors from the selected configuration theme.'

Tal is a core developer and may be the one who merges this, making the final choice of blurb (usually aimed at users) and commit message (aimed at other developers and future IDLE maintainers, including ourselves.

In the meanwhile, I replaced the above with "Use user-selected color theme for read-only text views." I don't like 'Make' when another verb is available. Since this change is visible to users, I replaced the internal name 'TextView'.

If I write decent blurbs, it is because I edit and rewrite. I sometimes rewrite again before committing to the IDLE news file viewable from About IDLE.

terryjreedy · 2018-09-23T03:37:17Z

In bpo-27117, I extracted the color_config code from Editor_Window.__init__ so that configured colors could be used with any text widget. The use then was for turtledemo (which uses colorizer) and the colorizer htest. I will modify the docstring. (I think the function is OK where it is ;-).

terryjreedy

I cannot help but be pleased that the factoring_out of color_config make this TODO so trivial. Tal, thanks for noticing.

Cheryl, thank you for the review. bpo-33396 is the master issue for the help viewer. I just added item 9 about further upgrades to text viewer. I included consistency in using a proportional font for non-code text, at least for read-only text. (Help viewer already does this.)

Given that color_config has been tested in use, and that it works with the About IDLE files, including colorizing selections the same as in the editor, I approve of merging without other tests.

miss-islington · 2018-09-23T12:23:17Z

Thanks @taleinat for the PR 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

https://bugs.python.org/issue34548 (cherry picked from commit c87d9f4) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

bedevere-bot · 2018-09-23T12:23:40Z

GH-9513 is a backport of this pull request to the 3.7 branch.

https://bugs.python.org/issue34548 (cherry picked from commit c87d9f4) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

bedevere-bot · 2018-09-23T12:23:48Z

GH-9514 is a backport of this pull request to the 3.6 branch.

taleinat · 2018-09-23T12:24:53Z

I'm not backporting this to 2.7 since @terryjreedy's refactoring out of color_config() wasn't backported to it, and this PR depends on that.

https://bugs.python.org/issue34548 (cherry picked from commit c87d9f4) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

bpo-34548: IDLE: Make TextView use the configured theme colors

1256c49

taleinat requested a review from terryjreedy August 30, 2018 12:45

the-knights-who-say-ni added the CLA signed label Aug 30, 2018

bedevere-bot added the awaiting merge label Aug 30, 2018

bpo-34548: add a NEWS entry

68d6454

csabella reviewed Sep 22, 2018

View reviewed changes

Lib/idlelib/textview.py Outdated Show resolved Hide resolved

csabella reviewed Sep 23, 2018

View reviewed changes

terryjreedy added 2 commits September 22, 2018 21:55

Merge remote-tracking branch 'origin/master' into pr_9008

6d825ef

Minor changes to colorizer, textview, and blurb.

92f39d3

Tweak color_config docstring.

e86927c

terryjreedy approved these changes Sep 23, 2018

View reviewed changes

Update 2018-09-22-20-25-07.bpo-34548.7pBzjg.rst

9b2831c

taleinat changed the title ~~bpo-34548: IDLE: Make TextView use the configured theme colors~~ bpo-34548: IDLE: use configured theme colors in TextView Sep 23, 2018

taleinat added 🤖 automerge and removed 🤖 automerge labels Sep 23, 2018

miss-islington merged commit c87d9f4 into python:master Sep 23, 2018

bedevere-bot removed the awaiting merge label Sep 23, 2018

bedevere-bot removed the needs backport to 3.7 label Sep 23, 2018

bedevere-bot removed the needs backport to 3.6 label Sep 23, 2018

miss-islington added a commit that referenced this pull request Sep 23, 2018

bpo-34548: IDLE: use configured theme colors in TextView (GH-9008)

aeadf59

https://bugs.python.org/issue34548 (cherry picked from commit c87d9f4) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

miss-islington added a commit that referenced this pull request Sep 23, 2018

bpo-34548: IDLE: use configured theme colors in TextView (GH-9008)

6b48f98

https://bugs.python.org/issue34548 (cherry picked from commit c87d9f4) Co-authored-by: Tal Einat <taleinat+github@gmail.com>

taleinat deleted the issue34548/IDLE_text_view_use_configured_colors branch September 23, 2018 13:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34548: IDLE: use configured theme colors in TextView #9008

bpo-34548: IDLE: use configured theme colors in TextView #9008

taleinat commented Aug 30, 2018 •

edited by bedevere-bot

Loading

taleinat commented Sep 22, 2018

csabella Sep 22, 2018

csabella commented Sep 22, 2018

csabella commented Sep 22, 2018

csabella Sep 23, 2018

terryjreedy Sep 23, 2018

terryjreedy commented Sep 23, 2018 •

edited

Loading

terryjreedy left a comment

miss-islington commented Sep 23, 2018

bedevere-bot commented Sep 23, 2018

bedevere-bot commented Sep 23, 2018

taleinat commented Sep 23, 2018

		@@ -32,10 +32,10 @@ def make_pat():
		idprog = re.compile(r"\s+(\w+)", re.S)

		def color_config(text): # Called from htest, Editor, and Turtle Demo.

		@@ -0,0 +1 @@
		Make TextView use the text colors from the main IDLE configuration.

bpo-34548: IDLE: use configured theme colors in TextView #9008

bpo-34548: IDLE: use configured theme colors in TextView #9008

Conversation

taleinat commented Aug 30, 2018 • edited by bedevere-bot Loading

taleinat commented Sep 22, 2018

csabella Sep 22, 2018

Choose a reason for hiding this comment

csabella commented Sep 22, 2018

csabella commented Sep 22, 2018

csabella Sep 23, 2018

Choose a reason for hiding this comment

terryjreedy Sep 23, 2018

Choose a reason for hiding this comment

terryjreedy commented Sep 23, 2018 • edited Loading

terryjreedy left a comment

Choose a reason for hiding this comment

miss-islington commented Sep 23, 2018

bedevere-bot commented Sep 23, 2018

bedevere-bot commented Sep 23, 2018

taleinat commented Sep 23, 2018

taleinat commented Aug 30, 2018 •

edited by bedevere-bot

Loading

terryjreedy commented Sep 23, 2018 •

edited

Loading