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

The widget single cell scrolling methods no longer work as expected from the keyboard #1897

Closed
davep opened this issue Feb 28, 2023 · 3 comments · Fixed by #1902
Closed

The widget single cell scrolling methods no longer work as expected from the keyboard #1897

davep opened this issue Feb 28, 2023 · 3 comments · Fixed by #1902
Assignees
Labels
bug Something isn't working Task

Comments

@davep
Copy link
Contributor

davep commented Feb 28, 2023

Consider the following (slightly over-complicated, but designed to show all the related information) code:

from rich.segment        import Segment
from textual.app         import App, ComposeResult
from textual.widgets     import Header, Footer
from textual.scroll_view import ScrollView
from textual.geometry    import Size
from textual.strip       import Strip

class ScrollViewTest( ScrollView, can_focus=True ):

    def __init__( self, height: int ) -> None:
        super().__init__()
        self.virtual_size = Size( 0, height )

    def render_line( self, y: int ) -> Strip:
        return Strip( [
            Segment( f"{self.scroll_offset.y + y:10}" ),
            Segment( f"{y:10}" ),
            *(
                [ Segment( f"{' ' * 10}scroll_offset.y == {self.scroll_offset.y}") ]
                if y == 0 else []
            )
        ] )

class ScrollTestApp( App[ None ] ):

    def compose( self ) -> ComposeResult:
        yield Header()
        yield ScrollViewTest( 1000 )
        yield Footer()

    def on_mount( self ) -> None:
        self.query_one( ScrollViewTest ).focus()

if __name__ == "__main__":
    ScrollTestApp().run()

If you run it and, using just the keyboard, try and scroll down just one line, or up one line, you'll find it's not possible. It will always scroll two lines.

It seems that this was introduced as part of #1689 ("Scroll sensitivity"). Two App-level properties were introduced, called scroll_sensitivity_x and scroll_sensitivity_y. Both properties say they dictate the number of cells to scroll in the given direction "with wheel or trackpad". However, they're used in the related Widget.scroll_* methods regardless of the input method.

At the very least scroll_up, scroll_down, scroll_left and scroll_right should be modified to only apply the sensitivity setting if the input device is a pointing device rather than a key binding.

@davep davep added bug Something isn't working Task labels Feb 28, 2023
davep added a commit to davep/textual-sandbox that referenced this issue Feb 28, 2023
@davep
Copy link
Contributor Author

davep commented Feb 28, 2023

Bumping this to TODO and claiming it as it intersects with what'll need doing with #1774. This should be merged first I feel, then #1774 can follow (or it may be done as part of the same change; I've yet to decide -- either way I'm promoting it and claiming it).

@willmcgugan
Copy link
Collaborator

I think we need to differentiate between mouse scroll (which should respect sensitivity and keyboard scroll which should not respect sensitivity). Suspect we will need to add a "mouse_initiated" bool to the scroll up / down / left / right methods.

davep added a commit to davep/textual that referenced this issue Feb 28, 2023
The changes here roll two issues into one change. With this commit:

- Scrolling up/down/etc using the keyboard now moves just one cell, rather
  than moving the number of cells specified by the scroll sensitivity that's
  intended for pointing devices. Textualize#1897
- Where appropriate the scrolling is done lazily; that is it is done after
  the next refresh, helping to ensure that the scroll will take into account
  any updates in the same parent call. Textualize#1774
@davep davep linked a pull request Feb 28, 2023 that will close this issue
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants