Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scrolling for IDLE browsers #82083

Closed
GeeTransit mannequin opened this issue Aug 21, 2019 · 14 comments
Closed

Add scrolling for IDLE browsers #82083

GeeTransit mannequin opened this issue Aug 21, 2019 · 14 comments
Assignees
Labels
3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-IDLE type-feature A feature request or enhancement

Comments

@GeeTransit
Copy link
Mannequin

GeeTransit mannequin commented Aug 21, 2019

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
    Copy link
    Mannequin Author

    GeeTransit mannequin commented Aug 21, 2019

    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.

    @GeeTransit GeeTransit mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Aug 21, 2019
    @GeeTransit GeeTransit mannequin assigned terryjreedy Aug 21, 2019
    @GeeTransit GeeTransit mannequin added topic-IDLE type-feature A feature request or enhancement labels Aug 21, 2019
    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    GeeTransit mannequin commented Aug 21, 2019

    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
    Copy link
    Mannequin Author

    GeeTransit mannequin commented Aug 21, 2019

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

    @terryjreedy
    Copy link
    Member

    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
    Copy link
    Mannequin Author

    GeeTransit mannequin commented Aug 22, 2019

    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
    Copy link
    Mannequin Author

    GeeTransit mannequin commented Aug 22, 2019

    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
    Copy link
    Member

    '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
    Copy link
    Member

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

    @terryjreedy
    Copy link
    Member

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

    @miss-islington
    Copy link
    Contributor

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

    @miss-islington
    Copy link
    Contributor

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

    @terryjreedy
    Copy link
    Member

    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.)

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes topic-IDLE type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants