From 49422932c7b48e44c0d16969240ca18bc99382ef Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 09:15:05 +0100 Subject: [PATCH 01/10] Move scroll navigation key bindings out of widget and into own container The idea here is that not every widget will scroll, and as such not every widget needs to have default bindings for calling the scrolling methods. Generally scrolling is something done in a container. These days we have *Scroll containers. As such it makes sense to introduce the bindings in a common parent class for those containers. This commit moves the binding from widget and creates that common parent class, and then has HorizontalScroll and VerticalScroll inherit from it. This is, it should be noted, a breaking change. Any code that creates a scrolling widget that assumes that the bindings are just there, where that widget doesn't inherit either from HorizontalScroll or VerticalScroll, will suddenly find that scrolling with the keyboard is no longer possible. See #2332. --- src/textual/containers.py | 36 ++++++++++++++++++++++++++++++++++-- src/textual/widget.py | 12 ------------ 2 files changed, 34 insertions(+), 14 deletions(-) diff --git a/src/textual/containers.py b/src/textual/containers.py index 9d6c35b7f6..0623ef0b30 100644 --- a/src/textual/containers.py +++ b/src/textual/containers.py @@ -3,7 +3,11 @@ """ +from __future__ import annotations +from typing import ClassVar + +from .binding import Binding, BindingType from .widget import Widget @@ -19,6 +23,34 @@ class Container(Widget): """ +class ScrollableContainer(Widget): + """Base container widget that binds navigation keys for scrolling.""" + + BINDINGS: ClassVar[list[BindingType]] = [ + Binding("up", "scroll_up", "Scroll Up", show=False), + Binding("down", "scroll_down", "Scroll Down", show=False), + Binding("left", "scroll_left", "Scroll Up", show=False), + Binding("right", "scroll_right", "Scroll Right", show=False), + Binding("home", "scroll_home", "Scroll Home", show=False), + Binding("end", "scroll_end", "Scroll End", show=False), + Binding("pageup", "page_up", "Page Up", show=False), + Binding("pagedown", "page_down", "Page Down", show=False), + ] + """Keyboard bindings for scrollable containers. + + | Key(s) | Description | + | :- | :- | + | up | Scroll up, if vertical scrolling is available. | + | down | Scroll down, if vertical scrolling is available. | + | left | Scroll left, if horizontal scrolling is available. | + | right | Scroll right, if horizontal scrolling is available. | + | home | Scroll to the home position, if scrolling is available. | + | end | Scroll to the end position, if scrolling is available. | + | pageup | Scroll up one page, if vertical scrolling is available. | + | pagedown | Scroll down one page, if vertical scrolling is available. | + """ + + class Vertical(Widget): """A container which arranges children vertically.""" @@ -31,7 +63,7 @@ class Vertical(Widget): """ -class VerticalScroll(Widget, can_focus=True): +class VerticalScroll(ScrollableContainer, can_focus=True): """A container which arranges children vertically, with an automatic vertical scrollbar.""" DEFAULT_CSS = """ @@ -55,7 +87,7 @@ class Horizontal(Widget): """ -class HorizontalScroll(Widget, can_focus=True): +class HorizontalScroll(ScrollableContainer, can_focus=True): """A container which arranges children horizontally, with an automatic horizontal scrollbar.""" DEFAULT_CSS = """ diff --git a/src/textual/widget.py b/src/textual/widget.py index b0cf8af97d..441e23e08d 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -55,7 +55,6 @@ from ._styles_cache import StylesCache from .actions import SkipAction from .await_remove import AwaitRemove -from .binding import Binding from .box_model import BoxModel from .css.query import NoMatches, WrongType from .css.scalar import ScalarOffset @@ -238,17 +237,6 @@ class Widget(DOMNode): """ - BINDINGS = [ - Binding("up", "scroll_up", "Scroll Up", show=False), - Binding("down", "scroll_down", "Scroll Down", show=False), - Binding("left", "scroll_left", "Scroll Up", show=False), - Binding("right", "scroll_right", "Scroll Right", show=False), - Binding("home", "scroll_home", "Scroll Home", show=False), - Binding("end", "scroll_end", "Scroll End", show=False), - Binding("pageup", "page_up", "Page Up", show=False), - Binding("pagedown", "page_down", "Page Down", show=False), - ] - DEFAULT_CSS = """ Widget{ scrollbar-background: $panel-darken-1; From ee45356c5c1cd09777e7d6f8a33bc420bc2053be Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 09:31:28 +0100 Subject: [PATCH 02/10] Update the CHANGELOG --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8aa50b167b..ac742ea24f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,14 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [0.20.1] - 2023-04-18 +### Changed + +- Breaking change: standard keyboard scrollable navigation bindings have been moved off `Widget` and onto a new base class for scrollable containers (see also below addition) https://github.com/Textualize/textual/issues/2332 + +### Added + +- Added `ScrollableContainer`; a container class that binds the common navigation keys to scroll actions (see also above breaking change) https://github.com/Textualize/textual/issues/2332 + ### Fix - New fix for stuck tabs underline https://github.com/Textualize/textual/issues/2229 From fe5f80bdd6324857aa3de25bb5704f872404d2dc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 09:40:58 +0100 Subject: [PATCH 03/10] Change ScrollView to inherit from ScrollableContainer rather than Widget See #2332 --- CHANGELOG.md | 1 + src/textual/scroll_view.py | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac742ea24f..2508db623d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Breaking change: standard keyboard scrollable navigation bindings have been moved off `Widget` and onto a new base class for scrollable containers (see also below addition) https://github.com/Textualize/textual/issues/2332 +- `ScrollView` now inherits from `Scrollablecontainer` rather than `Widget` https://github.com/Textualize/textual/issues/2332 ### Added diff --git a/src/textual/scroll_view.py b/src/textual/scroll_view.py index e791471e53..bb427b422e 100644 --- a/src/textual/scroll_view.py +++ b/src/textual/scroll_view.py @@ -7,11 +7,11 @@ from rich.console import RenderableType +from .containers import ScrollableContainer from .geometry import Size -from .widget import Widget -class ScrollView(Widget): +class ScrollView(ScrollableContainer): """ A base class for a Widget that handles it's own scrolling (i.e. doesn't rely on the compositor to render children). From 7d82e3068465c04d2ef5cf91abc02a8f5ee38f18 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 09:42:29 +0100 Subject: [PATCH 04/10] Fix demo so that keyboard navigation scrolls the display again The `Body` class inherited from `Container` rather than one of the scrolling containers; until now it had worked because `Widget` provided the bindings to make this happen, now that they've moved into `ScrollableContainer` that stopped working. --- src/textual/demo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/textual/demo.py b/src/textual/demo.py index 3cb55b0273..750ffe079b 100644 --- a/src/textual/demo.py +++ b/src/textual/demo.py @@ -14,7 +14,7 @@ from textual.app import App, ComposeResult from textual.binding import Binding -from textual.containers import Container, Horizontal +from textual.containers import Container, Horizontal, ScrollableContainer from textual.reactive import reactive from textual.widgets import ( Button, @@ -189,7 +189,7 @@ """ -class Body(Container): +class Body(ScrollableContainer): pass From 8a909b0d0ac76f218294c7923dbae1c75dff1fbf Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 09:49:06 +0100 Subject: [PATCH 05/10] Change all containers so they don't inherit bindings This is, to some degree, rendered moot by #2332, but for the moment it still feels worth doing. The initial intention was to make sure that non-scrolling containers and their child classes don't have bindings that may mask other uses for navigation keys. However, it was realised that the "problem" affected more than just containers (hence #2332). But... on the off chance we add any more default bindings to `Widget` (unlikely, but still), this will mean that they don't leak into the containers unless we intend them to. See #2331. --- src/textual/containers.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/textual/containers.py b/src/textual/containers.py index 0623ef0b30..54aa36d507 100644 --- a/src/textual/containers.py +++ b/src/textual/containers.py @@ -23,7 +23,7 @@ class Container(Widget): """ -class ScrollableContainer(Widget): +class ScrollableContainer(Widget, inherit_bindings=False): """Base container widget that binds navigation keys for scrolling.""" BINDINGS: ClassVar[list[BindingType]] = [ @@ -51,7 +51,7 @@ class ScrollableContainer(Widget): """ -class Vertical(Widget): +class Vertical(Widget, inherit_bindings=False): """A container which arranges children vertically.""" DEFAULT_CSS = """ @@ -75,7 +75,7 @@ class VerticalScroll(ScrollableContainer, can_focus=True): """ -class Horizontal(Widget): +class Horizontal(Widget, inherit_bindings=False): """A container which arranges children horizontally.""" DEFAULT_CSS = """ @@ -99,7 +99,7 @@ class HorizontalScroll(ScrollableContainer, can_focus=True): """ -class Center(Widget): +class Center(Widget, inherit_bindings=False): """A container which centers children horizontally.""" DEFAULT_CSS = """ @@ -111,7 +111,7 @@ class Center(Widget): """ -class Middle(Widget): +class Middle(Widget, inherit_bindings=False): """A container which aligns children vertically in the middle.""" DEFAULT_CSS = """ @@ -123,7 +123,7 @@ class Middle(Widget): """ -class Grid(Widget): +class Grid(Widget, inherit_bindings=False): """A container with grid alignment.""" DEFAULT_CSS = """ @@ -134,7 +134,7 @@ class Grid(Widget): """ -class Content(Widget, can_focus=True, can_focus_children=False): +class Content(Widget, can_focus=True, can_focus_children=False, inherit_bindings=False): """A container for content such as text.""" DEFAULT_CSS = """ From 1c5909eb23c72a1b0dd10db4f609f531546d42d7 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 10:04:32 +0100 Subject: [PATCH 06/10] Update the binding inheritance tests for the new moment bindings approach Now that navigation bindings don't pollute the whole widget hierarchy any more some of these tests can be tidied up. --- tests/test_binding_inheritance.py | 24 ++++-------------------- 1 file changed, 4 insertions(+), 20 deletions(-) diff --git a/tests/test_binding_inheritance.py b/tests/test_binding_inheritance.py index e5545dc3cb..6e4c59b0f0 100644 --- a/tests/test_binding_inheritance.py +++ b/tests/test_binding_inheritance.py @@ -47,9 +47,8 @@ async def test_just_app_no_bindings() -> None: # An application with a single alpha binding. # # Sticking with just an app and the default screen: this configuration has a -# BINDINGS on the app itself, and simply binds the letter a -- in other -# words avoiding anything to do with movement keys. The result should be -# that we see the letter a, ctrl+c, and nothing else. +# BINDINGS on the app itself, and simply binds the letter a. The result +# should be that we see the letter a, ctrl+c, and nothing else. class AlphaBinding(App[None]): @@ -114,22 +113,13 @@ def on_mount(self) -> None: async def test_app_screen_with_bindings() -> None: """Test a screen with a single key binding defined.""" async with AppWithScreenThatHasABinding().run_test() as pilot: - # The screen will contain all of the movement keys, because it - # inherits from Widget. That's fine. Let's check they're there, but - # also let's check that they all have a non-priority binding. - assert all( - pilot.app.screen._bindings.get_key(key).priority is False - for key in MOVEMENT_KEYS - ) - # Let's also check that the 'a' key is there, and it *is* a priority - # binding. assert pilot.app.screen._bindings.get_key("a").priority is True ############################################################################## # A non-default screen with a single low-priority alpha key binding. # -# As above, but because Screen sets akk keys as high priority by default, we +# As above, but because Screen sets all keys as high priority by default, we # want to be sure that if we set our keys in our subclass as low priority as # default, they come through as such. @@ -152,13 +142,7 @@ def on_mount(self) -> None: async def test_app_screen_with_low_bindings() -> None: """Test a screen with a single low-priority key binding defined.""" async with AppWithScreenThatHasALowBinding().run_test() as pilot: - # Screens inherit from Widget which means they get movement keys - # too, so let's ensure they're all in there, along with our own key, - # and that everyone is low-priority. - assert all( - pilot.app.screen._bindings.get_key(key).priority is False - for key in ["a", *MOVEMENT_KEYS] - ) + assert pilot.app.screen._bindings.get_key("a").priority is False ############################################################################## From 59e325d0045b27edc2557798ed563c26fbfbc58d Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 10:08:40 +0100 Subject: [PATCH 07/10] Update the CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2508db623d..42869df6da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Breaking change: standard keyboard scrollable navigation bindings have been moved off `Widget` and onto a new base class for scrollable containers (see also below addition) https://github.com/Textualize/textual/issues/2332 - `ScrollView` now inherits from `Scrollablecontainer` rather than `Widget` https://github.com/Textualize/textual/issues/2332 +- Containers no longer inherit any bindings from `Widget` https://github.com/Textualize/textual/issues/2331 ### Added From 14f05899b22188911c3484eb6b6bd613ba1b49e6 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 10:09:20 +0100 Subject: [PATCH 08/10] Correct the location of the new items in the CHANGELOG I'd accidentally started adding things under v0.20.1 rather than under a new unreleased heading. --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42869df6da..7e59e2744c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). -## [0.20.1] - 2023-04-18 +## Unreleased ### Changed @@ -17,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `ScrollableContainer`; a container class that binds the common navigation keys to scroll actions (see also above breaking change) https://github.com/Textualize/textual/issues/2332 +## [0.20.1] - 2023-04-18 + ### Fix - New fix for stuck tabs underline https://github.com/Textualize/textual/issues/2229 From b6b8fbdb414f98b671a701d0ac09c6e87f20a125 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 11:13:01 +0100 Subject: [PATCH 09/10] Fix typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7e59e2744c..4babc5e90d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,7 +10,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed - Breaking change: standard keyboard scrollable navigation bindings have been moved off `Widget` and onto a new base class for scrollable containers (see also below addition) https://github.com/Textualize/textual/issues/2332 -- `ScrollView` now inherits from `Scrollablecontainer` rather than `Widget` https://github.com/Textualize/textual/issues/2332 +- `ScrollView` now inherits from `ScrollableContainer` rather than `Widget` https://github.com/Textualize/textual/issues/2332 - Containers no longer inherit any bindings from `Widget` https://github.com/Textualize/textual/issues/2331 ### Added From c507c4a7858741659f66e24026fe2303e6efd507 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 20 Apr 2023 11:14:11 +0100 Subject: [PATCH 10/10] Driveby typo fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com> --- src/textual/scroll_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/scroll_view.py b/src/textual/scroll_view.py index bb427b422e..46dd934cb4 100644 --- a/src/textual/scroll_view.py +++ b/src/textual/scroll_view.py @@ -13,7 +13,7 @@ class ScrollView(ScrollableContainer): """ - A base class for a Widget that handles it's own scrolling (i.e. doesn't rely + A base class for a Widget that handles its own scrolling (i.e. doesn't rely on the compositor to render children). """