Fixes and improvements relating to scrolling #1902

davep · 2023-02-28T15:03:50Z

The changes here roll two issues into one change. With this PR:

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. The widget single cell scrolling methods no longer work as expected from the keyboard #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. scroll_* doesn't work immediately after loading a Syntax into a Static #1774

A question remains about if we should apply the changes here to scroll_to_widget too, but it's a wee bit involved so we're going to backlog that and review that later (note that scroll_visible is a common way into scroll_to_widget and it has always done a call after refresh anyway).

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

Nuked and recloned my repo earlier and forgot to set up pre-commit again.

rodrigogiraoserrao

Docstrings just need a little bit of trimming! Otherwise, good to go :)

rodrigogiraoserrao · 2023-03-01T09:27:26Z

src/textual/widget.py

+            x: X coordinate (column) to scroll to, or None for no change. Defaults to None.
+            y: Y coordinate (row) to scroll to, or None for no change. Defaults to None.
+            animate: Animate to new scroll position. Defaults to True.
+            speed: Speed of scroll if animate is True. Or None to use duration.
+            duration: Duration of animation, if animate is True and speed is None.
+            easing: An easing method for the scrolling animation. Defaults to "None",
+                which will result in Textual choosing the default scrolling easing function.
+            force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.


Please remove all the defaults from the docstring :)

I see this was likely a copy & paste from other methods, like scroll_relative.
If you want, you can also delete the defaults from there 😆

Let's make this a followup "clean widget.py docs" PR in that case (although I do tend to like to see something in the docstring that makes it clear it's optional and what the default may be and why).

Right. Explaining what the default does or why it is a specific value makes more sense than just writing "Defaults to x".
I don't like "Defaults to x" because it ages badly: in a couple of iterations it will become a lie and it is easy to forget updating that.

A more complete sentence saying something like "Defaults to None, which will result in Textual [...]" is more sensible but it can also be rewritten in a way that doesn't even specify the default, e.g., "An easing method for the scrolling animation to replace the one Textual chooses by default".

What do you think?

But given the follow-up issue, I think this can be merged.

What do you think?

I think if we explain that something is optional, but don't say what the default is and means, that makes more work for the reader. So "An easing method for the scrolling animation to replace the one Textual chooses by default" means either a) we've got to somehow link to somewhere that says what that default is (and so the reader ends up being taken out of their current context and then has to remember they need to go back rather than disappear down a documentation rabbit hole) or b) the reader has to make a note to go find how and where that default behaviour is documented.

I'm totally on board with the whole thing of not repeating facts of the code in the docs, for fear if it going stale, but I do worry that we end up leaving the docs lacking in useful information.

(and now I've seen I've written a fair bit here perhaps this is a consideration for us to spin out into its own issue)

rodrigogiraoserrao · 2023-03-01T09:29:29Z

src/textual/widget.py

+            animate: Animate scroll. Defaults to True.
+            speed: Speed of scroll if animate is True. Or None to use duration.
+            duration: Duration of animation, if animate is True and speed is None.
+            easing: An easing method for the scrolling animation. Defaults to "None",
+                which will result in Textual choosing the configured default scrolling easing function.
+            force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.


Please remove defaults.

Ditto followup widget.py docstring cleanup.

rodrigogiraoserrao · 2023-03-01T09:31:58Z

src/textual/widget.py

+            animate: Animate scroll. Defaults to True.
+            speed: Speed of scroll if animate is True. Or None to use duration.
+            duration: Duration of animation, if animate is True and speed is None.
+            easing: An easing method for the scrolling animation. Defaults to "None",
+                which will result in Textual choosing the configured default scrolling easing function.
+            force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.


Ditto followup widget.py docstring cleanup.

rodrigogiraoserrao · 2023-03-01T09:32:37Z

src/textual/widget.py

+            animate: Animate scroll. Defaults to True.
+            speed: Speed of scroll if animate is True. Or None to use duration.
+            duration: Duration of animation, if animate is True and speed is None.
+            easing: An easing method for the scrolling animation. Defaults to "None",
+                which will result in Textual choosing the configured default scrolling easing function.
+            force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.


Ditto followup widget.py docstring cleanup.

rodrigogiraoserrao · 2023-03-01T09:32:49Z

src/textual/widget.py

+            animate: Animate scroll. Defaults to True.
+            speed: Speed of scroll if animate is True. Or None to use duration.
+            duration: Duration of animation, if animate is True and speed is None.
+            easing: An easing method for the scrolling animation. Defaults to "None",
+                which will result in Textual choosing the configured default scrolling easing function.
+            force: Force scrolling even when prohibited by overflow styling. Defaults to `False`.


Ditto followup widget.py docstring cleanup.

willmcgugan

Just one request

src/textual/widget.py

davep · 2023-03-01T11:15:43Z

PS: Currently working on #1811 so will roll the scroll_* docstring trimming into that.

davep added bug Something isn't working enhancement New feature or request Task labels Feb 28, 2023

Reformat for black

f7849bd

Nuked and recloned my repo earlier and forgot to set up pre-commit again.

This was linked to issues Feb 28, 2023

scroll_* doesn't work immediately after loading a Syntax into a Static #1774

Closed

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

Closed

davep marked this pull request as ready for review February 28, 2023 16:06

davep requested review from rodrigogiraoserrao, willmcgugan and darrenburns February 28, 2023 16:07

willmcgugan mentioned this pull request Feb 28, 2023

fix for line api, wrong dimensions #1891

Closed

3 tasks

rodrigogiraoserrao approved these changes Mar 1, 2023

View reviewed changes

willmcgugan approved these changes Mar 1, 2023

View reviewed changes

src/textual/widget.py Outdated Show resolved Hide resolved

Add typing and a docstring to _lazily_scroll_end

db708bf

davep merged commit b4da48a into Textualize:main Mar 1, 2023

davep deleted the lazy-scrolling branch March 1, 2023 11:13

davep mentioned this pull request Mar 1, 2023

Add missing docstrings to a number of widgets #1811

Closed

9 tasks

This was referenced Mar 1, 2023

Widget documentation sweep #1909

Merged

scroll_end off by one #1215

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes and improvements relating to scrolling #1902

Fixes and improvements relating to scrolling #1902

davep commented Feb 28, 2023 •

edited

Loading

rodrigogiraoserrao left a comment

rodrigogiraoserrao Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

rodrigogiraoserrao Mar 1, 2023

davep Mar 1, 2023

willmcgugan left a comment

davep commented Mar 1, 2023

Fixes and improvements relating to scrolling #1902

Fixes and improvements relating to scrolling #1902

Conversation

davep commented Feb 28, 2023 • edited Loading

rodrigogiraoserrao left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willmcgugan left a comment

Choose a reason for hiding this comment

davep commented Mar 1, 2023

davep commented Feb 28, 2023 •

edited

Loading