classification
Title: Add scrolling for IDLE browsers
Type: enhancement Stage: resolved
Components: IDLE Versions: Python 3.9, Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: terry.reedy Nosy List: GeeVye, miss-islington, terry.reedy
Priority: normal Keywords: patch

Created on 2019-08-21 01:54 by GeeVye, last changed 2019-09-05 04:13 by terry.reedy. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 15368 merged GeeVye, 2019-08-21 21:11
PR 15689 merged miss-islington, 2019-09-05 01:35
PR 15690 merged miss-islington, 2019-09-05 01:35
Messages (14)
msg350043 - (view) Author: George Zhang (GeeVye) * Date: 2019-08-21 01:54
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.
msg350047 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-08-21 03:58
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.
msg350048 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-08-21 04:03
#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.
msg350095 - (view) Author: George Zhang (GeeVye) * Date: 2019-08-21 16:44
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.
msg350110 - (view) Author: George Zhang (GeeVye) * Date: 2019-08-21 21:16
Looks like my PRs are getting out of hand... This is the final PR :P
msg350113 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-08-21 21:41
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.
msg350210 - (view) Author: George Zhang (GeeVye) * Date: 2019-08-22 16:24
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).
msg350216 - (view) Author: George Zhang (GeeVye) * Date: 2019-08-22 17:49
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?
msg350222 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-08-22 18:21
'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'.
msg350223 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-08-22 18:22
Don't worry more about tests until I look at what you have done already.
msg351166 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-09-05 01:33
New changeset 2cd902585815582eb059e3b40e014ebe4e7fdee7 by Terry Jan Reedy (GeeTransit) in branch 'master':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (#15368)
https://github.com/python/cpython/commit/2cd902585815582eb059e3b40e014ebe4e7fdee7
msg351171 - (view) Author: miss-islington (miss-islington) Date: 2019-09-05 01:53
New changeset 9c2654d1aa85968fede1b888fba86aebc06c5be6 by Miss Islington (bot) in branch '3.8':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368)
https://github.com/python/cpython/commit/9c2654d1aa85968fede1b888fba86aebc06c5be6
msg351172 - (view) Author: miss-islington (miss-islington) Date: 2019-09-05 01:58
New changeset 16af39aa84cc3553c51d57461964ab4e28029184 by Miss Islington (bot) in branch '3.7':
bpo-37902: IDLE: Add scrolling for IDLE browsers. (GH-15368)
https://github.com/python/cpython/commit/16af39aa84cc3553c51d57461964ab4e28029184
msg351174 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2019-09-05 04:13
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 (#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.)
History
Date User Action Args
2019-09-05 04:13:49terry.reedysetstatus: open -> closed
resolution: fixed
messages: + msg351174

stage: patch review -> resolved
2019-09-05 01:58:25miss-islingtonsetmessages: + msg351172
2019-09-05 01:53:50miss-islingtonsetnosy: + miss-islington
messages: + msg351171
2019-09-05 01:35:24miss-islingtonsetpull_requests: + pull_request15348
2019-09-05 01:35:18miss-islingtonsetstage: test needed -> patch review
pull_requests: + pull_request15347
2019-09-05 01:33:36terry.reedysetmessages: + msg351166
2019-08-22 18:22:03terry.reedysetmessages: + msg350223
2019-08-22 18:21:04terry.reedysetmessages: + msg350222
2019-08-22 17:49:46GeeVyesetmessages: + msg350216
2019-08-22 16:24:09GeeVyesetmessages: + msg350210
2019-08-21 21:41:50terry.reedysetmessages: + msg350113
stage: patch review -> test needed
2019-08-21 21:16:56GeeVyesetmessages: + msg350110
2019-08-21 21:11:58GeeVyesetpull_requests: + pull_request15079
2019-08-21 20:35:05GeeVyesetpull_requests: - pull_request15077
2019-08-21 20:34:59GeeVyesetpull_requests: - pull_request15076
2019-08-21 19:45:29GeeVyesetpull_requests: + pull_request15077
2019-08-21 19:45:29GeeVyesetpull_requests: + pull_request15076
2019-08-21 19:34:55GeeVyesetpull_requests: - pull_request15072
2019-08-21 16:44:49GeeVyesetmessages: + msg350095
2019-08-21 15:19:32GeeVyesetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request15072
2019-08-21 04:03:13terry.reedylinkissue31461 dependencies
2019-08-21 04:03:08terry.reedysetmessages: + msg350048
2019-08-21 03:58:46terry.reedysetmessages: + msg350047
stage: needs patch
2019-08-21 01:54:40GeeVyecreate