gh-77578: IDLE Help - let users control font size #6665

csabella · 2018-04-30T23:44:35Z

Get initial font size from configuration.
Allow for dynamically changing font size via keyboard or mousewheel.

Issue: IDLE help viewer: let users control font size #77578

terryjreedy · 2018-05-01T02:41:40Z

Lib/idlelib/idle_test/test_help.py

@@ -30,5 +40,75 @@ def test_line1(self):
        text = self.frame.text
        self.assertEqual(text.get('1.0', '1.end'), ' IDLE ')

+
+class HelpTestTest(unittest.TestCase):


typo - HelpTextTest

terryjreedy · 2018-05-01T02:56:43Z

Lib/idlelib/help.py

+    def create_fonts(self):
+        "Create fonts to be used with tags."
+        self.base_size = idleConf.GetOption('main', 'EditorWindow',
+                                            'font-size', type='int')


This sets base_size to 10 unless user overrides. Test must not depend on any particular value.

Thanks. I've changed the test to set this value.

Set to 12 in the test

terryjreedy · 2018-05-01T05:32:26Z

Lib/idlelib/help.py

+        self.bind('<<zoom-text-out>>', lambda e: self.zoom(e, 'out'))
+
+        self.event_add('<<zoom-text-in>>', '<Control-Key-minus>')
+        self.bind('<<zoom-text-in>>', lambda e: self.zoom(e, 'in'))


I don't think we need the indirection of a pseudoevent or the lambda.
self.bind('', self.zoom)
etc should work as zoom can switch on event.type (keypress, button, or wheel).

I originally did this because I wasn't sure about hardcoding the checks for the 'plus' and 'minus' keys, if those would be configurable at some point. I've changed it to look for the keys and I suppose it can be changed later to use the keys attached to the binding.

I think, if anything, we have too many key configurations. zoom has been changed. See new review comment.

terryjreedy · 2018-05-01T05:32:29Z

Lib/idlelib/help.py

+        self.event_add('<<zoom-text-in>>', '<Control-Key-minus>')
+        self.bind('<<zoom-text-in>>', lambda e: self.zoom(e, 'in'))
+
+        # Windows and Mac use MouseWheel.


But they use opposite signs and magnitudes. From turtledemo.main
# For wheel up, event.delta = 120 on Windows, -1 on darwin.
I want to use the debugged code from that file (which I helped write ;-) as a starting point, but it can be simplified to one event handler (zoom) and one new size handler (here scale fontsize).

I also want to consider that the basic handling should be the same for editor windows. Perhaps we can factor out a FontSizer class, with the event handlers, that is initialized with the widget to bind methods to and a method to call with the new font size. This would be easier with standardized attribute names.

I hadn't seen the turtledemo code before I made my PR, but it was helpful to look at it now.

Created the FontSizer class. I tried to do it in a way as to not rely on the names that the widget uses. The only thing that is required is that the font used by the widget be an actual Font() and not just defined on the fly.

Still need to review.

terryjreedy · 2018-05-01T05:33:49Z

Lib/idlelib/help.py

+        for tag in self.fonts:
+            self.fonts[tag]['size'] = int(base * scale[tag])
+
+    def zoom(self, event, how='out'):


Delete 'how' and switch on event.type.

Done. I like it much better this way. :-)

terryjreedy · 2018-05-01T05:59:03Z

Lib/idlelib/help.py

+        "Scale tag sizes based on the size of normal text."
+        # Scale percentages are from Sphinx classic.css.
+        scale = {'normal': 1.0, 'h6': 1.0, 'h5': 1.1, 'h4': 1.2, 'h3': 1.4,
+                 'h2': 1.6, 'h1': 2.0, 'em': 1.0, 'pre': 1.0, 'preblock': 0.9}


h6 to h4 are not needed. 'h1': 20 seems too big as current 20/12 = 5/3 = 1.6 seems sufficient. ditto for h2 18/12 = 3/2 = 1.5, h3 = 15/12 = 5/4 = 1.2. The current ratios result in text the nearly matches the Firefox version.

I had originally added the extra values just to show the progression, even though they weren't needed. Now I've removed them and updated the ratios.

terryjreedy · 2018-05-01T05:59:14Z

Lib/idlelib/help.py

+        if how == 'wheel':
+            # Button 5 or negative delta is when mouse wheel is scrolled down.
+            how = 'in' if event.num == 5 or event.delta < 0 else 'out'
+        factor = 1.1 if how == 'out' else 0.9


I would rather just increase/decrease the integer size by 1, but within bounds.

self.scale_fontsize(max(self.base_size - 1, MINIMUM_FONT_SIZE) if <decrease> else min(self.base_size - 1, MAXIMUM_FONT_SIZE))

terryjreedy · 2018-05-01T06:05:24Z

Lib/idlelib/idle_test/test_help.py

+                 ('<Control-Button-4>', 0, zoomout),
+                 ('<Control-Button-5>', 0, base))
+
+        # Base size starts at 12.


You set it to the default editor text size, which is 10 on buildbots.

Fixed to set this value during the test.

terryjreedy · 2018-05-01T06:05:30Z

Lib/idlelib/idle_test/test_help.py

+
+        # Base size starts at 12.
+        sizes = [text.fonts[tag]['size'] for tag in tags]
+        eq(text.base_size, base[0])


10 is not 12.

terryjreedy · 2018-05-01T06:06:53Z

Lib/idlelib/help.py

+        normalfont = self.findfont(['TkDefaultFont', 'arial', 'helvetica'])
+        fixedfont = self.findfont(['TkFixedFont', 'monaco', 'courier'])
+
+        self.fonts = fonts = {}


I don't really like the doubling of the configuration code, but I have not thought of anything better yet.

I didn't make any changes for this.

csabella · 2018-05-02T18:16:59Z

I have made the requested changes; please review again.

bedevere-bot · 2018-05-02T18:17:01Z

Thanks for making the requested changes!

@terryjreedy: please review the changes made to this pull request.

terryjreedy · 2018-05-17T05:26:56Z

Lib/idlelib/help.py

+        # Zoom out works with or without shift.
+        self.widget.event_add(
+                '<<zoom-text-out>>',
+                *[f'<{shortcut}-Key-equal>', f'<{shortcut}-Key-plus>'])


f(*[1,2]) is exactly the same as f(1,2). Using * in a call is for when one has a name, not a display.
so pass f'<{shortcut}-Key-equal>', f'<{shortcut}-Key-plus>'.
I would still like to remove the pseudoevents.

The unneeded *[..] is still present here and on line 185.

turtledemo.main does not have these pseudoevents. The only reason I can think of now to add them is if they were to make testing significantly easier and more dependable.

I tried back in May and I've been trying for a few days to get the tests to work without the virtual events. Even though it seems like the text widget has focus, doing a text.event_generate('<Control-Key-plus>') just won't activate the binding. I'm sure I'm just missing something obvious, but I cannot figure out what it is. :-(

terryjreedy · 2018-05-17T05:30:13Z

Lib/idlelib/help.py

+        self.bind('<<zoom-text-out>>', lambda e: self.zoom(e, 'out'))
+
+        self.event_add('<<zoom-text-in>>', '<Control-Key-minus>')
+        self.bind('<<zoom-text-in>>', lambda e: self.zoom(e, 'in'))


I think, if anything, we have too many key configurations. zoom has been changed. See new review comment.

Lib/idlelib/help.py

terryjreedy · 2018-05-17T05:34:20Z

Lib/idlelib/help.py

+        self.event_add('<<zoom-text-in>>', '<Control-Key-minus>')
+        self.bind('<<zoom-text-in>>', lambda e: self.zoom(e, 'in'))
+
+        # Windows and Mac use MouseWheel.


Still need to review.

terryjreedy

You can leave the one suggested code change until I retest and recheck code. 3.7.0c1 is scheduled next Tues, but I don't want to rush this without test on Mac. I really must try out devguide instructions with a small MacBook portable I was given.

Lib/idlelib/help.py

terryjreedy

We started with this code, extracted from turtledemo.main.

# Menu bindings (which we might use for context menu)
        menu.add_command(label="Decrease (C-'-')", command=self.decrease_size,
                         font=menufont)
        menu.add_command(label="Increase (C-'+')", command=self.increase_size,
                         font=menufont)
# Key/mouse bindings
        text.bind_all('<%s-minus>' % shortcut, self.decrease_size)
        text.bind_all('<%s-underscore>' % shortcut, self.decrease_size)
        text.bind_all('<%s-equal>' % shortcut, self.increase_size)
        text.bind_all('<%s-plus>' % shortcut, self.increase_size)
        text.bind('<Control-MouseWheel>', self.update_mousewheel)
        text.bind('<Control-Button-4>', self.increase_size)
        text.bind('<Control-Button-5>', self.decrease_size)
# Event/action functions
darwin = sys.platform == 'darwin'

    def set_txtsize(self, size):
        txtfont[1] = size
        self.text['font'] = tuple(txtfont)
        self.output_lbl['text'] = 'Font size %d' % size

    def decrease_size(self, dummy=None):
        self.set_txtsize(max(txtfont[1] - 1, MINIMUM_FONT_SIZE))
        return 'break'

    def increase_size(self, dummy=None):
        self.set_txtsize(min(txtfont[1] + 1, MAXIMUM_FONT_SIZE))
        return 'break'

    def update_mousewheel(self, event):
        # For wheel up, event.delta = 120 on Windows, -1 on darwin.
        # X-11 sends Control-Button-4 event instead.
        if (event.delta < 0) == (not darwin):
            return self.decrease_size()
        else:
            return self.increase_size()

As far as I know, menu, keyboard, and mouse methods work on all three major systems. There are no pseudoevents. To the extent possible, the meaning of actions is determined by their bindings.

Note: the code above is extracted from 4 different locations. This is the first time I have looked at it altogether.

At my request, Cheryl combined and consolidated the functions into one zoom handler. At Tal's request, she partly undid what I had requested. Zoom() has been separated into multiple functions again. However, I prefer the original code to the result. If we don't agree on reverting to the original design, lets discuss.

terryjreedy · 2018-09-04T01:54:22Z

Lib/idlelib/help.py

+        # Zoom out works with or without shift.
+        self.widget.event_add(
+                '<<zoom-text-out>>',
+                *[f'<{shortcut}-Key-equal>', f'<{shortcut}-Key-plus>'])


The unneeded *[..] is still present here and on line 185.

turtledemo.main does not have these pseudoevents. The only reason I can think of now to add them is if they were to make testing significantly easier and more dependable.

terryjreedy · 2018-09-04T02:33:19Z

Lib/idlelib/help.py

+
+    def zoom_mousewheel(self, event):
+        "Adjust font size based on mouse wheel direction."
+        wheel_up = {EventType.MouseWheel: event.delta > 0,


This leaves out '== darwin', which I believe is correct for font resizing even if not correct for scrolling.

I wasn't sure when that logic needed to be applied and when it didn't. So, it's only adjusted on scrolling and not on other uses of the mousewheel?

I tested turtledemo a month ago and font resizing works the same as on Windows. Moving the top of the wheeI up increases size. I don't actually know if this is the correct convention on Mac, but in the absence of information otherwise, including the absence of complaint about turtledemo, I will consider it correct. I may have tested command-wheel-up in Safari and will recheck tomorrow.

When we copied to logic for scrolling, the result was buggy since the convention on Mac, which I know I verified with Safari, is the opposite of Windows (and Linux, I believe). I will try to find and point you to the tracker discussion.

terryjreedy · 2018-09-04T02:44:17Z

Lib/idlelib/help.py

@@ -176,31 +176,39 @@ def bind_events(self):
        shortcut = 'Command' if darwin else 'Control'
        # Zoom out works with or without shift.
        self.widget.event_add(
-                '<<zoom-text-out>>',
+                '<<zoom-text-in>>',


I guess the pseudoevent error was masked since the following line has the same error and the direction was recalculated (in former version of zoom).

While refamiliarizing myself with this code, I kept getting confused on what 'zoom out' and 'zoom in' meant and added a comment to the new methods in help.py to say which means larger text and which means smaller text. Once I did that, I realized I had it backwards in the definitions.

I agree that these names are potentially confusing and propose above (May 17 comment on line 180) 'make_font_bigger' and 'make_font_smaller'.

terryjreedy

github threw away or hid my my review summary. Will try to find or recreate.

Now it is visible again.

csabella · 2018-09-04T22:21:35Z

When I looked back through my original commits and Terry's comments, I thought the main issue was that I was using lambda and sending another parameter to the zoom method. I didn't recall that you preferred to have just one zoom handler. Splitting them out here was a good exercise for me to get reacquainted with this change, but I don't have a preference either way. Although, splitting them out did force me to add tests for the keybindings and also I noticed I didn't test the bounds before.

taleinat · 2018-09-05T06:58:32Z

My apologies, @terryjreedy and @csabella. I wasn't aware of Terry's request to consolidate the zoom handling into a single handler. Cheryl, please do as Terry has requested, as he has the final say on these matters.

terryjreedy · 2018-09-05T09:24:18Z

Tal, I assumed that you had not gone through all the hidden 'outdated' comments. In the future, perhaps I should give better summary reviews as they should be easier to find. In any case, I welcome your review of all IDLE issues. Your dislike of zoom got me to take another look. I partly like it, but partly not. It is okay with me if we occasionally try a refactoring and decide against it.

I responded about pseudoevents and handler names on the May 17 comment on line 180.
I responded about wheel convention on yesterdays comment on line 204.

It conflicted with my requested changes.

taleinat · 2018-09-05T09:32:03Z

@terryjreedy, on the contrary, I should just take more care to read all of the previous discussion. You've got more than enough on your plate, you shouldn't bother to write summaries when the reviews are there to be read.

terryjreedy

Current requested changes: Reverse previous request to combine turtledemo functions into zoom(). However, add psuedoevents '<<increase_font_size>>' and '<<decrease_font_size>>', use the same names for the handler functions. Don't use *[] in event add. Use original mouse logic.

Use pseudoevents for testing. Don't worry about key-mouse events now.

* Remove use of callback to adjust fonts in tags. * Adjust tag font sizes the same as the main font. * TODO: Tags don't retain ratio, so after they all go to min size, they don't recover original ratios.

* Originally resized Font objects, but now can resize fonts defined as tuples.

taleinat · 2020-09-20T18:46:42Z

Ping, @csabella?

terryjreedy · 2020-09-21T07:00:47Z

I summarized 'remaining issues' on Sept 8, 2018, and added more on Sept 23 and Dec 14. I need to check all against current code updated since then.

csabella · 2020-09-21T11:59:58Z

As far as I know, I had made all the requested changes and I had tried to keep up with rebasing.

ambv · 2022-05-17T16:46:22Z

This missed the boat for inclusion in Python 3.9 which accepts security fixes only as of today.

github-actions · 2022-08-17T00:12:05Z

This PR is stale because it has been open for 30 days with no activity.

github-actions · 2024-12-15T00:11:58Z

This PR is stale because it has been open for 30 days with no activity.

csabella requested a review from terryjreedy as a code owner April 30, 2018 23:44

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

bedevere-bot added the awaiting review label Apr 30, 2018

terryjreedy changed the title ~~bpo-25198: IDLE Help: Modifications for font size~~ bpo-33397: IDLE Help: Modifications for font size May 1, 2018

terryjreedy requested changes May 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels May 1, 2018

bedevere-bot added awaiting change review and removed awaiting changes labels May 2, 2018

terryjreedy self-assigned this May 17, 2018

terryjreedy added needs backport to 3.6 labels May 17, 2018

terryjreedy reviewed May 17, 2018

View reviewed changes

terryjreedy requested changes May 17, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels May 17, 2018

taleinat previously requested changes Aug 1, 2018

View reviewed changes

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

terryjreedy requested changes Sep 4, 2018

View reviewed changes

python deleted a comment from bedevere-bot Sep 5, 2018

terryjreedy requested changes Sep 5, 2018

View reviewed changes

csabella added 9 commits June 20, 2020 09:18

Move FontSizer to textview.

a23cb1c

Retain Close button when increasing font size.

e88f488

Use decorator for changing text widget

90cb041

Adjust font tag sizes within FontSizer.

e03e4b2

* Remove use of callback to adjust fonts in tags. * Adjust tag font sizes the same as the main font. * TODO: Tags don't retain ratio, so after they all go to min size, they don't recover original ratios.

Allow for sizing fonts defined in different ways.

645a0e0

* Originally resized Font objects, but now can resize fonts defined as tuples.

Add tests for fonts defined as tuples.

5c1d58a

Refactor return values

98295cb

Update split to splitlist

d6f80e8

Remove unrelated changes to editor.py

dab3eb5

csabella force-pushed the bpo25198 branch from 4d71d1b to dab3eb5 Compare June 20, 2020 13:19

terryjreedy added needs backport to 3.9 and removed needs backport to 3.7 labels Sep 21, 2020

terryjreedy added 3 commits September 21, 2020 08:37

Update 2018-04-30-19-11-09.bpo-25198.yzJ3SL.rst

26b3342

Merge remote-tracking branch 'upstream/master' into pr_6665

100bdea

Merge branch 'master' into bpo25198

803800d

ambv removed the needs backport to 3.8 label May 4, 2021

ambv removed the needs backport to 3.9 label May 17, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot added the stale label Aug 17, 2022

terryjreedy changed the title ~~bpo-33397: IDLE Help: Modifications for font size~~ gh-77578: IDLE Help: Modifications for font size Nov 6, 2022

terryjreedy changed the title ~~gh-77578: IDLE Help: Modifications for font size~~ gh-77578: IDLE Help - let users control font size Nov 6, 2022

terryjreedy mentioned this pull request Nov 7, 2022

IDLE help viewer: let users control font size #77578

Open

github-actions bot removed the stale label Sep 21, 2024

github-actions bot added the stale label Dec 15, 2024

gh-77578: IDLE Help - let users control font size #6665

Are you sure you want to change the base?

gh-77578: IDLE Help - let users control font size #6665

Conversation

csabella commented Apr 30, 2018 • edited by terryjreedy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy May 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csabella commented May 2, 2018

bedevere-bot commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment

Choose a reason for hiding this comment

terryjreedy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terryjreedy left a comment • edited Loading

Choose a reason for hiding this comment

csabella commented Sep 4, 2018

taleinat commented Sep 5, 2018

terryjreedy commented Sep 5, 2018

taleinat commented Sep 5, 2018

terryjreedy left a comment

Choose a reason for hiding this comment

taleinat commented Sep 20, 2020

terryjreedy commented Sep 21, 2020

csabella commented Sep 21, 2020

ambv commented May 17, 2022

github-actions bot commented Aug 17, 2022

github-actions bot commented Dec 15, 2024

csabella commented Apr 30, 2018 •

edited by terryjreedy

Loading

terryjreedy May 17, 2018 •

edited

Loading

csabella May 2, 2018 •

edited

Loading

csabella May 2, 2018 •

edited

Loading

terryjreedy left a comment •

edited

Loading

terryjreedy left a comment •

edited

Loading