From d005b543717413d8d0199cacbbe24af1597406cb Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Wed, 23 Oct 2024 19:09:00 +0100 Subject: [PATCH 01/12] Fix list view flicker --- src/textual/_loop.py | 43 ++++++++++++++- src/textual/widget.py | 89 +++++++++++++++++++++++++------ src/textual/widgets/_list_view.py | 72 ++++++++++++------------- 3 files changed, 150 insertions(+), 54 deletions(-) diff --git a/src/textual/_loop.py b/src/textual/_loop.py index 7a057fc214..0f50f864b3 100644 --- a/src/textual/_loop.py +++ b/src/textual/_loop.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import Iterable, TypeVar +from typing import Iterable, Literal, Sequence, TypeVar T = TypeVar("T") @@ -43,3 +43,44 @@ def loop_first_last(values: Iterable[T]) -> Iterable[tuple[bool, bool, T]]: first = False previous_value = value yield first, True, previous_value + + +def loop_from_index( + values: Sequence[T], index: int, direction: Literal[+1, -1] = +1 +) -> Iterable[tuple[int, T]]: + """Iterate over values in a sequence from a given starting index, wrapping the index + if it would go out of bounds. + + Args: + values: A sequence of values. + index: Starting index. + direction: Direction to move index (+1 for forward, -1 for backward). + + Yields: + A tuple of index and value from the sequence. + """ + count = len(values) + for _ in range(count): + index = (index + direction) % count + yield (index, values[index]) + + +def loop_from_index_no_wrap( + values: Sequence[T], index: int, direction: Literal[+1, -1] = +1 +) -> Iterable[tuple[int, T]]: + """Iterate over values in a sequence from a given starting index, without wrapping the index. + + Args: + values: A sequence of values. + index: Starting index. + direction: Direction to move index (+1 for forward, -1 for backward). + + Yields: + A tuple of index and value from the sequence. + """ + count = len(values) + maxima = (-1, count) + for _ in range(count): + if (index := index + direction) in maxima: + break + yield (index, values[index]) diff --git a/src/textual/widget.py b/src/textual/widget.py index cf072cba6e..917efd9d8b 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -2349,6 +2349,7 @@ def scroll_to( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll to a given (absolute) coordinate, optionally animating. @@ -2362,22 +2363,37 @@ def scroll_to( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. Note: The call to scroll is made after the next refresh. """ - self.call_after_refresh( - self._scroll_to, - x, - y, - animate=animate, - speed=speed, - duration=duration, - easing=easing, - force=force, - on_complete=on_complete, - level=level, - ) + if immediate: + self._scroll_to( + x, + y, + animate=animate, + speed=speed, + duration=duration, + easing=easing, + force=force, + on_complete=on_complete, + level=level, + ) + else: + self.call_after_refresh( + self._scroll_to, + x, + y, + animate=animate, + speed=speed, + duration=duration, + easing=easing, + force=force, + on_complete=on_complete, + level=level, + ) def scroll_relative( self, @@ -2391,6 +2407,7 @@ def scroll_relative( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll relative to current position. @@ -2404,6 +2421,8 @@ def scroll_relative( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self.scroll_to( None if x is None else (self.scroll_x + x), @@ -2415,6 +2434,7 @@ def scroll_relative( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def scroll_home( @@ -2427,6 +2447,7 @@ def scroll_home( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll to home position. @@ -2438,6 +2459,8 @@ def scroll_home( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ if speed is None and duration is None: duration = 1.0 @@ -2451,6 +2474,7 @@ def scroll_home( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def scroll_end( @@ -2463,6 +2487,7 @@ def scroll_end( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll to the end of the container. @@ -2474,6 +2499,8 @@ def scroll_end( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ if speed is None and duration is None: duration = 1.0 @@ -2510,6 +2537,7 @@ def scroll_left( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll one cell left. @@ -2521,6 +2549,8 @@ def scroll_left( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self.scroll_to( x=self.scroll_target_x - 1, @@ -2531,6 +2561,7 @@ def scroll_left( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def _scroll_left_for_pointer( @@ -2583,6 +2614,7 @@ def scroll_right( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll one cell right. @@ -2594,6 +2626,8 @@ def scroll_right( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self.scroll_to( x=self.scroll_target_x + 1, @@ -2604,6 +2638,7 @@ def scroll_right( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def _scroll_right_for_pointer( @@ -2656,6 +2691,7 @@ def scroll_down( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll one line down. @@ -2667,6 +2703,8 @@ def scroll_down( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self.scroll_to( y=self.scroll_target_y + 1, @@ -2677,6 +2715,7 @@ def scroll_down( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def _scroll_down_for_pointer( @@ -2729,6 +2768,7 @@ def scroll_up( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll one line up. @@ -2740,6 +2780,8 @@ def scroll_up( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self.scroll_to( y=self.scroll_target_y - 1, @@ -2942,6 +2984,7 @@ def scroll_to_widget( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> bool: """Scroll scrolling to bring a widget in to view. @@ -2956,6 +2999,8 @@ def scroll_to_widget( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. Returns: `True` if any scrolling has occurred in any descendant, otherwise `False`. @@ -2982,6 +3027,7 @@ def scroll_to_widget( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) if scroll_offset: scrolled = True @@ -3021,6 +3067,7 @@ def scroll_to_region( level: AnimationLevel = "basic", x_axis: bool = True, y_axis: bool = True, + immediate: bool = False, ) -> Offset: """Scrolls a given region in to view, if required. @@ -3041,6 +3088,8 @@ def scroll_to_region( level: Minimum level required for the animation to take place (inclusive). x_axis: Allow scrolling on X axis? y_axis: Allow scrolling on Y axis? + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. Returns: The distance that was scrolled. @@ -3101,6 +3150,7 @@ def clamp_delta(delta: Offset) -> Offset: force=force, on_complete=on_complete, level=level, + immediate=immediate, ) return delta @@ -3115,6 +3165,7 @@ def scroll_visible( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll the container to make this widget visible. @@ -3127,11 +3178,12 @@ def scroll_visible( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ parent = self.parent if isinstance(parent, Widget): - self.call_after_refresh( - self.screen.scroll_to_widget, + self.screen.scroll_to_widget( self, animate=animate, speed=speed, @@ -3141,6 +3193,7 @@ def scroll_visible( force=force, on_complete=on_complete, level=level, + immediate=immediate, ) def scroll_to_center( @@ -3155,6 +3208,7 @@ def scroll_to_center( origin_visible: bool = True, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll this widget to the center of self. @@ -3170,9 +3224,11 @@ def scroll_to_center( origin_visible: Ensure that the top left corner of the widget remains visible after the scroll. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ - self.call_after_refresh( - self.scroll_to_widget, + + self.scroll_to_widget( widget=widget, animate=animate, speed=speed, @@ -3183,6 +3239,7 @@ def scroll_to_center( origin_visible=origin_visible, on_complete=on_complete, level=level, + immediate=immediate, ) def can_view(self, widget: Widget) -> bool: diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index a60ddf8468..e8e47f8172 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -4,7 +4,7 @@ from typing_extensions import TypeGuard -from textual import _widget_navigation +from textual._loop import loop_from_index_no_wrap from textual.await_remove import AwaitRemove from textual.binding import Binding, BindingType from textual.containers import VerticalScroll @@ -38,7 +38,7 @@ class ListView(VerticalScroll, can_focus=True, can_focus_children=False): | down | Move the cursor down. | """ - index = reactive[Optional[int]](0, always_update=True, init=False) + index = reactive[Optional[int]](None, init=False) """The index of the currently highlighted item.""" class Highlighted(Message): @@ -117,17 +117,12 @@ def __init__( super().__init__( *children, name=name, id=id, classes=classes, disabled=disabled ) - # Set the index to the given initial index, or the first available index after. - self._index = _widget_navigation.find_next_enabled( - children, - anchor=initial_index if initial_index is not None else None, - direction=1, - with_anchor=True, - ) + self._initial_index = initial_index def _on_mount(self, _: Mount) -> None: """Ensure the ListView is fully-settled after mounting.""" - self.index = self._index + with self.prevent(self.Highlighted): + self.index = self._initial_index @property def highlighted_child(self) -> ListItem | None: @@ -165,16 +160,25 @@ def _is_valid_index(self, index: int | None) -> TypeGuard[int]: def watch_index(self, old_index: int | None, new_index: int | None) -> None: """Updates the highlighting when the index changes.""" + + if new_index is not None: + self.scroll_to_widget( + self.children[new_index], animate=False, immediate=True + ) + if self._is_valid_index(old_index): old_child = self._nodes[old_index] assert isinstance(old_child, ListItem) old_child.highlighted = False - if self._is_valid_index(new_index) and not self._nodes[new_index].disabled: + if ( + new_index is not None + and self._is_valid_index(new_index) + and not self._nodes[new_index].disabled + ): new_child = self._nodes[new_index] assert isinstance(new_child, ListItem) new_child.highlighted = True - self._scroll_highlighted_region() self.post_message(self.Highlighted(self, new_child)) else: self.post_message(self.Highlighted(self, None)) @@ -271,27 +275,28 @@ def action_select_cursor(self) -> None: def action_cursor_down(self) -> None: """Highlight the next item in the list.""" - candidate = _widget_navigation.find_next_enabled( - self._nodes, - anchor=self.index, - direction=1, - ) - if self.index is not None and candidate is not None and candidate < self.index: - return # Avoid wrapping around. - - self.index = candidate + if self.index is None: + if self._nodes: + self.index = 0 + else: + index = self.index + for index, item in loop_from_index_no_wrap(self._nodes, self.index): + if not item.disabled: + self.index = index + break def action_cursor_up(self) -> None: """Highlight the previous item in the list.""" - candidate = _widget_navigation.find_next_enabled( - self._nodes, - anchor=self.index, - direction=-1, - ) - if self.index is not None and candidate is not None and candidate > self.index: - return # Avoid wrapping around. - - self.index = candidate + if self.index is None: + if self._nodes: + self.index = len(self._nodes) - 1 + else: + for index, item in loop_from_index_no_wrap( + self._nodes, self.index, direction=-1 + ): + if not item.disabled: + self.index = index + break def _on_list_item__child_clicked(self, event: ListItem._ChildClicked) -> None: event.stop() @@ -299,13 +304,6 @@ def _on_list_item__child_clicked(self, event: ListItem._ChildClicked) -> None: self.index = self._nodes.index(event.item) self.post_message(self.Selected(self, event.item)) - def _scroll_highlighted_region(self) -> None: - """Used to keep the highlighted index within vision""" - if self.highlighted_child is not None: - self.call_after_refresh( - self.scroll_to_widget, self.highlighted_child, animate=False - ) - def __len__(self) -> int: """Compute the length (in number of items) of the list view.""" return len(self._nodes) From ac85f286fa9b325a8f0362c89d8733272c4e0179 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Wed, 23 Oct 2024 19:16:01 +0100 Subject: [PATCH 02/12] loop --- CHANGELOG.md | 7 ++++++ src/textual/_loop.py | 37 ++++++++++--------------------- src/textual/widgets/_list_view.py | 8 +++---- 3 files changed, 23 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a7487fe28..e7dd34ab05 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,13 @@ 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/). +## Unreleased + +### Added + +- Added `immediate` parameter to scroll methods https://github.com/Textualize/textual/pull/5164 +- Added `textual._loop.loop_from_index` https://github.com/Textualize/textual/pull/5164 + ## [0.84.0] - 2024-10-22 ### Fixed diff --git a/src/textual/_loop.py b/src/textual/_loop.py index 0f50f864b3..7711a99d57 100644 --- a/src/textual/_loop.py +++ b/src/textual/_loop.py @@ -46,7 +46,7 @@ def loop_first_last(values: Iterable[T]) -> Iterable[tuple[bool, bool, T]]: def loop_from_index( - values: Sequence[T], index: int, direction: Literal[+1, -1] = +1 + values: Sequence[T], index: int, direction: Literal[+1, -1] = +1, wrap: bool = True ) -> Iterable[tuple[int, T]]: """Iterate over values in a sequence from a given starting index, wrapping the index if it would go out of bounds. @@ -55,32 +55,19 @@ def loop_from_index( values: A sequence of values. index: Starting index. direction: Direction to move index (+1 for forward, -1 for backward). + bool: Should the index wrap when out of bounds? Yields: A tuple of index and value from the sequence. """ count = len(values) - for _ in range(count): - index = (index + direction) % count - yield (index, values[index]) - - -def loop_from_index_no_wrap( - values: Sequence[T], index: int, direction: Literal[+1, -1] = +1 -) -> Iterable[tuple[int, T]]: - """Iterate over values in a sequence from a given starting index, without wrapping the index. - - Args: - values: A sequence of values. - index: Starting index. - direction: Direction to move index (+1 for forward, -1 for backward). - - Yields: - A tuple of index and value from the sequence. - """ - count = len(values) - maxima = (-1, count) - for _ in range(count): - if (index := index + direction) in maxima: - break - yield (index, values[index]) + if wrap: + maxima = (-1, count) + for _ in range(count): + if (index := index + direction) in maxima: + break + yield (index, values[index]) + else: + for _ in range(count): + index = (index + direction) % count + yield (index, values[index]) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index e8e47f8172..4ec1ff711b 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -4,7 +4,7 @@ from typing_extensions import TypeGuard -from textual._loop import loop_from_index_no_wrap +from textual._loop import loop_from_index from textual.await_remove import AwaitRemove from textual.binding import Binding, BindingType from textual.containers import VerticalScroll @@ -280,7 +280,7 @@ def action_cursor_down(self) -> None: self.index = 0 else: index = self.index - for index, item in loop_from_index_no_wrap(self._nodes, self.index): + for index, item in loop_from_index(self._nodes, self.index, wrap=False): if not item.disabled: self.index = index break @@ -291,8 +291,8 @@ def action_cursor_up(self) -> None: if self._nodes: self.index = len(self._nodes) - 1 else: - for index, item in loop_from_index_no_wrap( - self._nodes, self.index, direction=-1 + for index, item in loop_from_index( + self._nodes, self.index, direction=-1, wrap=False ): if not item.disabled: self.index = index From a1831d94639be10fde53a324ba4a2258d5711fe1 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 10:52:35 +0100 Subject: [PATCH 03/12] select first node --- src/textual/_loop.py | 10 +++++----- src/textual/scroll_view.py | 3 +++ src/textual/widgets/_list_view.py | 16 +++++++++++----- tests/listview/test_listview_initial_index.py | 2 +- .../snapshot_apps/listview_index.py | 3 ++- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/textual/_loop.py b/src/textual/_loop.py index 7711a99d57..e0ef2b76d9 100644 --- a/src/textual/_loop.py +++ b/src/textual/_loop.py @@ -48,7 +48,7 @@ def loop_first_last(values: Iterable[T]) -> Iterable[tuple[bool, bool, T]]: def loop_from_index( values: Sequence[T], index: int, direction: Literal[+1, -1] = +1, wrap: bool = True ) -> Iterable[tuple[int, T]]: - """Iterate over values in a sequence from a given starting index, wrapping the index + """Iterate over values in a sequence from a given starting index, potentially wrapping the index if it would go out of bounds. Args: @@ -62,12 +62,12 @@ def loop_from_index( """ count = len(values) if wrap: - maxima = (-1, count) for _ in range(count): - if (index := index + direction) in maxima: - break + index = (index + direction) % count yield (index, values[index]) else: + maxima = (-1, count) for _ in range(count): - index = (index + direction) % count + if (index := index + direction) in maxima: + break yield (index, values[index]) diff --git a/src/textual/scroll_view.py b/src/textual/scroll_view.py index 56b947172f..42c89ebb23 100644 --- a/src/textual/scroll_view.py +++ b/src/textual/scroll_view.py @@ -123,6 +123,7 @@ def scroll_to( force: bool = False, on_complete: CallbackType | None = None, level: AnimationLevel = "basic", + immediate: bool = False, ) -> None: """Scroll to a given (absolute) coordinate, optionally animating. @@ -136,6 +137,8 @@ def scroll_to( force: Force scrolling even when prohibited by overflow styling. on_complete: A callable to invoke when the animation is finished. level: Minimum level required for the animation to take place (inclusive). + immediate: If `False` the scroll will be deferred until after a screen refresh, + set to `True` to scroll immediately. """ self._scroll_to( diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index 4ec1ff711b..9757899957 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -121,8 +121,16 @@ def __init__( def _on_mount(self, _: Mount) -> None: """Ensure the ListView is fully-settled after mounting.""" - with self.prevent(self.Highlighted): - self.index = self._initial_index + + if self._initial_index is not None and self.children: + index = self._initial_index + if index >= len(self.children): + index = 0 + if self._nodes[index].disabled: + for index, node in loop_from_index(self._nodes, index, wrap=True): + if not node.disabled: + break + self.index = index @property def highlighted_child(self) -> ListItem | None: @@ -162,9 +170,7 @@ def watch_index(self, old_index: int | None, new_index: int | None) -> None: """Updates the highlighting when the index changes.""" if new_index is not None: - self.scroll_to_widget( - self.children[new_index], animate=False, immediate=True - ) + self.scroll_to_widget(self._nodes[new_index], animate=False, immediate=True) if self._is_valid_index(old_index): old_child = self._nodes[old_index] diff --git a/tests/listview/test_listview_initial_index.py b/tests/listview/test_listview_initial_index.py index 515de4f044..ab2eed573e 100644 --- a/tests/listview/test_listview_initial_index.py +++ b/tests/listview/test_listview_initial_index.py @@ -39,4 +39,4 @@ def compose(self) -> ComposeResult: app = ListViewDisabledItemsApp() async with app.run_test() as pilot: list_view = pilot.app.query_one(ListView) - assert list_view._index == expected_index + assert list_view.index == expected_index diff --git a/tests/snapshot_tests/snapshot_apps/listview_index.py b/tests/snapshot_tests/snapshot_apps/listview_index.py index 1aa88aec3b..7ea856bb67 100644 --- a/tests/snapshot_tests/snapshot_apps/listview_index.py +++ b/tests/snapshot_tests/snapshot_apps/listview_index.py @@ -22,7 +22,8 @@ def compose(self) -> ComposeResult: async def watch_data(self, data: "list[int]") -> None: await self._menu.remove_children() await self._menu.extend((ListItem(Label(str(value))) for value in data)) - self._menu.index = len(self._menu) - 1 + new_index = len(self._menu) - 1 + self._menu.index = new_index async def on_ready(self): self.data = list(range(0, 30, 2)) From ab02cf75deb1956a33ef5cdcd4361ad0260e14a3 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 11:19:31 +0100 Subject: [PATCH 04/12] list view fix --- src/textual/widgets/_list_view.py | 11 ++++++++--- tests/snapshot_tests/snapshot_apps/listview_index.py | 1 + 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index 9757899957..b92f1bf773 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -170,7 +170,14 @@ def watch_index(self, old_index: int | None, new_index: int | None) -> None: """Updates the highlighting when the index changes.""" if new_index is not None: - self.scroll_to_widget(self._nodes[new_index], animate=False, immediate=True) + selected_widget = self._nodes[new_index] + if selected_widget.region: + self.scroll_to_widget(self._nodes[new_index], animate=False) + else: + # Call after refresh to permit a refresh operation + self.call_after_refresh( + self.scroll_to_widget, selected_widget, animate=False + ) if self._is_valid_index(old_index): old_child = self._nodes[old_index] @@ -200,8 +207,6 @@ def extend(self, items: Iterable[ListItem]) -> AwaitMount: until the DOM has been updated with the new child items. """ await_mount = self.mount(*items) - if len(self) == 1: - self.index = 0 return await_mount def append(self, item: ListItem) -> AwaitMount: diff --git a/tests/snapshot_tests/snapshot_apps/listview_index.py b/tests/snapshot_tests/snapshot_apps/listview_index.py index 7ea856bb67..9027e2a3e0 100644 --- a/tests/snapshot_tests/snapshot_apps/listview_index.py +++ b/tests/snapshot_tests/snapshot_apps/listview_index.py @@ -22,6 +22,7 @@ def compose(self) -> ComposeResult: async def watch_data(self, data: "list[int]") -> None: await self._menu.remove_children() await self._menu.extend((ListItem(Label(str(value))) for value in data)) + new_index = len(self._menu) - 1 self._menu.index = new_index From 58116282f69dad432f5fe93b61c0ac90e0ae825a Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 11:32:21 +0100 Subject: [PATCH 05/12] test for loop_from_index --- src/textual/_loop.py | 10 +++++++++- tests/test_loop.py | 39 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/src/textual/_loop.py b/src/textual/_loop.py index e0ef2b76d9..ad4be901f6 100644 --- a/src/textual/_loop.py +++ b/src/textual/_loop.py @@ -46,11 +46,17 @@ def loop_first_last(values: Iterable[T]) -> Iterable[tuple[bool, bool, T]]: def loop_from_index( - values: Sequence[T], index: int, direction: Literal[+1, -1] = +1, wrap: bool = True + values: Sequence[T], + index: int, + direction: Literal[-1, +1] = +1, + wrap: bool = True, ) -> Iterable[tuple[int, T]]: """Iterate over values in a sequence from a given starting index, potentially wrapping the index if it would go out of bounds. + Note that the first value to be yielded is a step from `index`, and `index` will be yielded *last*. + + Args: values: A sequence of values. index: Starting index. @@ -60,6 +66,8 @@ def loop_from_index( Yields: A tuple of index and value from the sequence. """ + # Sanity check for devs who miss the typing errors + assert direction in (-1, +1), "direction must be -1 or +1" count = len(values) if wrap: for _ in range(count): diff --git a/tests/test_loop.py b/tests/test_loop.py index 87ef97da76..65b0aa0097 100644 --- a/tests/test_loop.py +++ b/tests/test_loop.py @@ -1,4 +1,4 @@ -from textual._loop import loop_first, loop_first_last, loop_last +from textual._loop import loop_first, loop_first_last, loop_from_index, loop_last def test_loop_first(): @@ -26,3 +26,40 @@ def test_loop_first_last(): assert next(iterable) == (False, False, "oranges") assert next(iterable) == (False, False, "pears") assert next(iterable) == (False, True, "lemons") + + +def test_loop_from_index(): + assert list(loop_from_index("abcdefghij", 3)) == [ + (4, "e"), + (5, "f"), + (6, "g"), + (7, "h"), + (8, "i"), + (9, "j"), + (0, "a"), + (1, "b"), + (2, "c"), + (3, "d"), + ] + + assert list(loop_from_index("abcdefghij", 3, direction=-1)) == [ + (2, "c"), + (1, "b"), + (0, "a"), + (9, "j"), + (8, "i"), + (7, "h"), + (6, "g"), + (5, "f"), + (4, "e"), + (3, "d"), + ] + + assert list(loop_from_index("abcdefghij", 3, wrap=False)) == [ + (4, "e"), + (5, "f"), + (6, "g"), + (7, "h"), + (8, "i"), + (9, "j"), + ] From 2974c1f64b3aacb6cd6ebcb53f6f4163f36309cf Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 11:34:34 +0100 Subject: [PATCH 06/12] todo comment --- src/textual/_widget_navigation.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/textual/_widget_navigation.py b/src/textual/_widget_navigation.py index c547d01be0..d80ae36b37 100644 --- a/src/textual/_widget_navigation.py +++ b/src/textual/_widget_navigation.py @@ -101,6 +101,8 @@ def find_last_enabled(candidates: Sequence[Disableable]) -> int | None: ) +# TODO: This method is quiet inefficient if an "anchor" is supplied +# I think we could optimize this, but its not clear if we need it at all def find_next_enabled( candidates: Sequence[Disableable], anchor: int | None, From 0dbeb743417d097ade60b7ca77f47cdc949a7731 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 11:35:34 +0100 Subject: [PATCH 07/12] changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7dd34ab05..01cb767e94 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `immediate` parameter to scroll methods https://github.com/Textualize/textual/pull/5164 - Added `textual._loop.loop_from_index` https://github.com/Textualize/textual/pull/5164 +### Fixed + +- Fixed glitchy ListView https://github.com/Textualize/textual/issues/5163 + ## [0.84.0] - 2024-10-22 ### Fixed From 0bb81368e9af3e5bcee3271c08a5da59f942405d Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Thu, 24 Oct 2024 11:38:51 +0100 Subject: [PATCH 08/12] optimization --- src/textual/_loop.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/textual/_loop.py b/src/textual/_loop.py index ad4be901f6..67546999c0 100644 --- a/src/textual/_loop.py +++ b/src/textual/_loop.py @@ -74,8 +74,13 @@ def loop_from_index( index = (index + direction) % count yield (index, values[index]) else: - maxima = (-1, count) - for _ in range(count): - if (index := index + direction) in maxima: - break - yield (index, values[index]) + if direction == +1: + for _ in range(count): + if (index := index + 1) >= count: + break + yield (index, values[index]) + else: + for _ in range(count): + if (index := index - 1) < 0: + break + yield (index, values[index]) From 952ee22edc6d4c31560a529289790dbae1b390bd Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 24 Oct 2024 14:21:32 +0100 Subject: [PATCH 09/12] Simplify find_next_enabled --- src/textual/_profile.py | 2 +- src/textual/_widget_navigation.py | 18 ++++++------------ 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/textual/_profile.py b/src/textual/_profile.py index 279a873ec0..3e880cf15c 100644 --- a/src/textual/_profile.py +++ b/src/textual/_profile.py @@ -16,4 +16,4 @@ def timer(subject: str = "time") -> Generator[None, None, None]: yield elapsed = perf_counter() - start elapsed_ms = elapsed * 1000 - log(f"{subject} elapsed {elapsed_ms:.2f}ms") + log(f"{subject} elapsed {elapsed_ms:.4f}ms") diff --git a/src/textual/_widget_navigation.py b/src/textual/_widget_navigation.py index d80ae36b37..1d010a17b4 100644 --- a/src/textual/_widget_navigation.py +++ b/src/textual/_widget_navigation.py @@ -8,12 +8,13 @@ from __future__ import annotations -from functools import partial from itertools import count from typing import Literal, Protocol, Sequence from typing_extensions import TypeAlias +from textual._loop import loop_from_index + class Disableable(Protocol): """Non-widgets that have an enabled/disabled status.""" @@ -136,17 +137,10 @@ def find_next_enabled( ) return None - start = anchor + direction if not with_anchor else anchor - key_function = partial( - get_directed_distance, - start=start, - direction=direction, - wrap_at=len(candidates), - ) - enabled_candidates = [ - index for index, candidate in enumerate(candidates) if not candidate.disabled - ] - return min(enabled_candidates, key=key_function, default=anchor) + for index, candidate in loop_from_index(candidates, anchor, direction, wrap=True): + if not candidate.disabled: + return index + return None def find_next_enabled_no_wrap( From 5ffa224041884583e130a55c82465c69390e2e3e Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 24 Oct 2024 14:25:31 +0100 Subject: [PATCH 10/12] Optimise optionlist/radio_set scrolling method --- src/textual/_widget_navigation.py | 2 +- tests/test_widget_navigation.py | 6 ------ 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/textual/_widget_navigation.py b/src/textual/_widget_navigation.py index 1d010a17b4..a5e8299000 100644 --- a/src/textual/_widget_navigation.py +++ b/src/textual/_widget_navigation.py @@ -140,7 +140,7 @@ def find_next_enabled( for index, candidate in loop_from_index(candidates, anchor, direction, wrap=True): if not candidate.disabled: return index - return None + return anchor def find_next_enabled_no_wrap( diff --git a/tests/test_widget_navigation.py b/tests/test_widget_navigation.py index a322f3846f..44f8b17159 100644 --- a/tests/test_widget_navigation.py +++ b/tests/test_widget_navigation.py @@ -142,16 +142,10 @@ def test_find_next_enabled_no_wrap(candidates, anchor, direction, result): @pytest.mark.parametrize( ["function", "start", "direction"], [ - (find_next_enabled, 0, 1), - (find_next_enabled, 0, -1), (find_next_enabled_no_wrap, 0, 1), (find_next_enabled_no_wrap, 0, -1), - (find_next_enabled, 1, 1), - (find_next_enabled, 1, -1), (find_next_enabled_no_wrap, 1, 1), (find_next_enabled_no_wrap, 1, -1), - (find_next_enabled, 2, 1), - (find_next_enabled, 2, -1), (find_next_enabled_no_wrap, 2, 1), (find_next_enabled_no_wrap, 2, -1), ], From 441a6d2a23612f7a6c9892ffb7845e91b25ca58e Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 24 Oct 2024 14:26:05 +0100 Subject: [PATCH 11/12] Remove TODO --- src/textual/_widget_navigation.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/textual/_widget_navigation.py b/src/textual/_widget_navigation.py index a5e8299000..023e94fb02 100644 --- a/src/textual/_widget_navigation.py +++ b/src/textual/_widget_navigation.py @@ -102,8 +102,6 @@ def find_last_enabled(candidates: Sequence[Disableable]) -> int | None: ) -# TODO: This method is quiet inefficient if an "anchor" is supplied -# I think we could optimize this, but its not clear if we need it at all def find_next_enabled( candidates: Sequence[Disableable], anchor: int | None, From 4afe6792cb8fff9450cfb649720a3de01854f5d7 Mon Sep 17 00:00:00 2001 From: Darren Burns Date: Thu, 24 Oct 2024 14:30:18 +0100 Subject: [PATCH 12/12] Remove unused param --- src/textual/_widget_navigation.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/textual/_widget_navigation.py b/src/textual/_widget_navigation.py index 023e94fb02..302a332090 100644 --- a/src/textual/_widget_navigation.py +++ b/src/textual/_widget_navigation.py @@ -106,7 +106,6 @@ def find_next_enabled( candidates: Sequence[Disableable], anchor: int | None, direction: Direction, - with_anchor: bool = False, ) -> int | None: """Find the next enabled object if we're currently at the given anchor. @@ -119,8 +118,6 @@ def find_next_enabled( enabled object. direction: The direction in which to traverse the candidates when looking for the next enabled candidate. - with_anchor: Consider the anchor position as the first valid position instead of - the last one. Returns: The next enabled object. If none are available, return the anchor.