Add scrolling for IDLE browsers

[GeeTransit]

BPO	37902
Nosy	@terryjreedy, @miss-islington, @GeeTransit
PRs	bpo-37902: IDLE: Add scrolling for IDLE browsers. #15368 [3.8] bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368) #15689 [3.7] bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368) #15690

^{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 2019-09-05.04:13:49.169>
created_at = <Date 2019-08-21.01:54:40.380>
labels = ['3.8', 'expert-IDLE', 'type-feature', '3.7', '3.9']
title = 'Add scrolling for IDLE browsers'
updated_at = <Date 2019-09-05.04:13:49.167>
user = 'https://github.com/GeeTransit'

bugs.python.org fields:

activity = <Date 2019-09-05.04:13:49.167>
actor = 'terry.reedy'
assignee = 'terry.reedy'
closed = True
closed_date = <Date 2019-09-05.04:13:49.169>
closer = 'terry.reedy'
components = ['IDLE']
creation = <Date 2019-08-21.01:54:40.380>
creator = 'GeeTransit'
dependencies = []
files = []
hgrepos = []
issue_num = 37902
keywords = ['patch']
message_count = 14.0
messages = ['350043', '350047', '350048', '350095', '350110', '350113', '350210', '350216', '350222', '350223', '351166', '351171', '351172', '351174']
nosy_count = 3.0
nosy_names = ['terry.reedy', 'miss-islington', 'GeeTransit']
pr_nums = ['15368', '15689', '15690']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue37902'
versions = ['Python 3.7', 'Python 3.8', 'Python 3.9']

[GeeTransit]

I've just started using IDLE's module/path browsers and they offer a lot! Putting aside the issue of them opening in separate windows, they have a small change that could be made to improve them.

Both browsers have scrollbars, but (for me at least) I cannot scroll using my mouse. I propose adding support for scrolling similar to the editor/shell windows.

[terryjreedy]

I agree. We added mousewheel scrolling to editor just over a year ago and later added it to text views. But for the browsers, I want to factor out the common code. It is a bit tricky since the 3 major systems each send different events for the same action, and macOS has the opposite convention for how the text moves when pushing the wheel up.

[terryjreedy]

bpo-31461 is the index issue for class browser. Mousewheel scrolling was listed without an issue. Now there is, and this has been added as a dependency.

[GeeTransit]

I looked at the code for scrolling and moved it over to the ScrolledCanvas and TreeNode (because it uses a Label that sits on the canvas, meaning we have to rebind it here).

I haven't figured out how to add the scroll-by-pressing-down-and-moving way but I'll look further into it.

About the factoring out part, the ScrolledCanvas and TreeNode are both used by the two browsers, meaning that no code has to be added to them. In the future, a factory function could be made that when called with a canvas, it returns an event callback function that can be bound to the canvas. When called, it scrolls depending on the event type and other info.

[GeeTransit]

Looks like my PRs are getting out of hand... This is the final PR :P

[terryjreedy]

I won't merge with mousescroll duplicated, or worse, triplicated. If 'self.text/canvas' is replaced with 'event.widget', then the 'self' parameter can be deleted and the function made a standalone module function. For now, put it in tree, copied with the docstring from editor.py. Then import it into editor. (Reason: Avoid creating more import loops.)

This will make mousescroll depend on IDLE not subclassing text/canvas and overriding yview_scroll. It currently does not and I expect it never will. But, to be safe, this should be added to the docstring, and tested (fairly simple) along with other added tests.

The labels partially blocking the canvas bindings is nasty. Mousescroll is wrapped as a tk script for each label. I expect to eventually replace the labels and other visual tree stuff with a ttk.Treeview. Then no canvas wheel bindings will be needed. In anticipation of that, replace 'yview_scroll(' with the equivalent 'yview(SCROLL,' (Treeview only has the latter.) The resulting line will be
event.widget.yview(SCROLL, lines, "units")

For some reason, creating a module browser for pyshell takes 3 times as long with my repository 3.9 debug build as with my 3.8 installed normal build. (The ration is usually about 2.) But the patch with the extra bindings and wrappings does not make it much worse, if any.

Scrolling by moving mouse while holding down middle mouse wheel seems to be built into Text. But I never/seldom use it and have not tried to add it to anything else. At least on Windows, it works differently than on Firefox. Text only moved with drag, which makes it jerky, not with distance from start position. And one cannot only scroll part of a large file, not to top and bottom. Notepad and notepad++ do not have it. So skip it for now.

When one edits a branch and pushes commits to ones github fork, the changes appear on any existing PR. Closing a PR and reopening a new one is unusual and loses comments. Post to core-mentorship if you need help with git and our workflow. My PR-15360 comment applies to the current one.

[GeeTransit]

I renamed mousescroll to handlescroll as it's an independent callback function and I think it fits its use case better. I can keep it as mousescroll if you like though.

The handlescroll function is now a standalone module function in tree.py and the EditorWindow imports it for use (instead of creating its own function). Its signature is handlescroll(event, widget=None) where event is the event (yeah...) and widget is an optional argument that overrides the widget to call yview(SCROLL, lines, "units") on.

The second argument was added so that the nasty labels don't have to use the same function but with one line changed (we redirect handlescroll to use self.canvas because event.widget would refer to the Label which has no yview function).

I've added tests on handlescroll with different events. I've also added a test on multicall that checks to test if MultiCallCreator overrides yview.

Sorry about the PR closing and reopening. I was panicking about commiting more than once and saw that I should make a new branch for the patch (instead of using master branch).

[GeeTransit]

Also, how should I get the new code coverage percentage (or should it be ignored for now)?

I'm thinking of adding a few more tests that send invalid events which would raise KeyError but I don't think that this behaviour will be used (and it's not documented). Should it give a more specific error message?

The test for multicall is only targeting the MultiCall class that gets returned. How should it check for any possible subclasses of it?

[terryjreedy]

'mousescroll' was not exact because the mouse is also used to scroll with the scrollbar. 'handlescroll' is worse. 'wheelscroll' seems awkward. 'scrollwheel' (scroll with the mouse wheel) is specific. At least in idlelib, event handlers are routinely called something_event, so use 'wheel_event'. Pressing the wheel is a Button-3 event (there used to be 3-button mice before wheeels) and a handler for that would be 'button3_event' or 'button3_press_event'.

[terryjreedy]

Don't worry more about tests until I look at what you have done already.

[terryjreedy]

New changeset 2cd9025 by Terry Jan Reedy (GeeTransit) in branch 'master':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (bpo-15368)
2cd9025

[miss-islington]

New changeset 9c2654d by Miss Islington (bot) in branch '3.8':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368)
9c2654d

[miss-islington]

New changeset 16af39a by Miss Islington (bot) in branch '3.7':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368)
16af39a

[terryjreedy]

Thanks for the patch. More IDLE patches would be welcome should you want to attack something else.

Possible browser scrolling refinements:

1. Scroll by an integral number of labels. This is easy with text. For our synthesized tree, we would have to calculate the # of canvas pixels to scroll. However, if we switch to ttk.Treeview (bpo-31552), it, like Text has a height in lines. So its yview method might scroll in line units, like text.

2. Only bind wheel event(s) used on system? Should test on 3 systems. Do all *nix other than tk for macOS use X-window buttons? Not a big deal.

3. Use bindtags to bind wheel events just once. Then unbind when shutdown? (Check if unbind elsewhere.)