From 4bd7edfdc5489ef8e9d65d8ec94af2d43468c57d Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 17 Oct 2024 08:55:56 +0100 Subject: [PATCH 01/11] fix(listview): update index after pop --- src/textual/widgets/_list_view.py | 25 ++++++--- tests/listview/test_listview_remove_items.py | 56 +++++++++++++++++++- 2 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index a60ddf8468..c452ef4a79 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -5,6 +5,7 @@ from typing_extensions import TypeGuard from textual import _widget_navigation +from textual.await_complete import AwaitComplete from textual.await_remove import AwaitRemove from textual.binding import Binding, BindingType from textual.containers import VerticalScroll @@ -231,7 +232,7 @@ def insert(self, index: int, items: Iterable[ListItem]) -> AwaitMount: await_mount = self.mount(*items, before=index) return await_mount - def pop(self, index: Optional[int] = None) -> AwaitRemove: + def pop(self, index: Optional[int] = None) -> AwaitComplete: """Remove last ListItem from ListView or Remove ListItem from ListView by index @@ -242,11 +243,23 @@ def pop(self, index: Optional[int] = None) -> AwaitRemove: An awaitable that yields control to the event loop until the DOM has been updated to reflect item being removed. """ - if index is None: - await_remove = self.query("ListItem").last().remove() - else: - await_remove = self.query("ListItem")[index].remove() - return await_remove + if len(self) == 0: + raise IndexError("pop from empty list") + + index = index if index is not None else -1 + if index < 0: + index += len(self) + item_to_remove = self.query("ListItem")[index] + + async def do_pop(): + await item_to_remove.remove() + if self.index is not None: + if index == self.index: + self.index = self.index + elif index < self.index: + self.index = self.index - 1 + + return AwaitComplete(do_pop()) def remove_items(self, indices: Iterable[int]) -> AwaitRemove: """Remove ListItems from ListView by indices diff --git a/tests/listview/test_listview_remove_items.py b/tests/listview/test_listview_remove_items.py index 43501cdf19..a8f7f6cd93 100644 --- a/tests/listview/test_listview_remove_items.py +++ b/tests/listview/test_listview_remove_items.py @@ -1,8 +1,29 @@ +import pytest + from textual.app import App, ComposeResult -from textual.widgets import ListView, ListItem, Label +from textual.widgets import Label, ListItem, ListView + + +class EmptyListViewApp(App[None]): + def compose(self) -> ComposeResult: + yield ListView() + + +async def test_listview_pop_empty_raises_index_error(): + app = EmptyListViewApp() + async with app.run_test() as pilot: + listview = pilot.app.query_one(ListView) + with pytest.raises(IndexError) as excinfo: + listview.pop() + assert "pop from empty list" in str(excinfo.value) class ListViewApp(App[None]): + def __init__(self, initial_index: int | None = None): + super().__init__() + self.initial_index = initial_index + self.highlighted = [] + def compose(self) -> ComposeResult: yield ListView( ListItem(Label("0")), @@ -14,8 +35,15 @@ def compose(self) -> ComposeResult: ListItem(Label("6")), ListItem(Label("7")), ListItem(Label("8")), + initial_index=self.initial_index, ) + def _on_list_view_highlighted(self, message: ListView.Highlighted) -> None: + if message.item is None: + self.highlighted.append(None) + else: + self.highlighted.append(str(message.item.children[0].renderable)) + async def test_listview_remove_items() -> None: """Regression test for https://github.com/Textualize/textual/issues/4735""" @@ -27,6 +55,32 @@ async def test_listview_remove_items() -> None: assert len(listview) == 4 +@pytest.mark.parametrize( + "initial_index, pop_index, expected_new_index, expected_highlighted", + [ + (2, 2, 2, ["2", "3"]), # Remove highlighted item + (0, 0, 0, ["0", "1"]), # Remove first item when highlighted + (8, None, 7, ["8", "7"]), # Remove last item when highlighted + (4, 2, 3, ["4", "4"]), # Remove item before the highlighted index + (4, -2, 4, ["4"]), # Remove item after the highlighted index + ], +) +async def test_listview_pop_updates_index_and_highlighting( + initial_index, pop_index, expected_new_index, expected_highlighted +) -> None: + """Regression test for https://github.com/Textualize/textual/issues/5114""" + app = ListViewApp(initial_index) + async with app.run_test() as pilot: + listview = pilot.app.query_one(ListView) + + await listview.pop(pop_index) + await pilot.pause() + + assert listview.index == expected_new_index + assert listview._nodes[expected_new_index].highlighted is True + assert app.highlighted == expected_highlighted + + if __name__ == "__main__": app = ListViewApp() app.run() From 2abce6d80bbdc2b4ea3b64258e0db8f35e0cbf83 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 17 Oct 2024 09:19:12 +0100 Subject: [PATCH 02/11] tests(listview): import future for type hints --- tests/listview/test_listview_remove_items.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/listview/test_listview_remove_items.py b/tests/listview/test_listview_remove_items.py index a8f7f6cd93..c6436835d1 100644 --- a/tests/listview/test_listview_remove_items.py +++ b/tests/listview/test_listview_remove_items.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import pytest from textual.app import App, ComposeResult From 643c2d933b0451c7ebe6fef065393762ec2d8546 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 17 Oct 2024 21:10:56 +0100 Subject: [PATCH 03/11] ensure pop error is original index rather than normalized --- src/textual/widgets/_list_view.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index c452ef4a79..d4783dd074 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -247,16 +247,15 @@ def pop(self, index: Optional[int] = None) -> AwaitComplete: raise IndexError("pop from empty list") index = index if index is not None else -1 - if index < 0: - index += len(self) item_to_remove = self.query("ListItem")[index] + normalized_index = index if index >= 0 else index + len(self) async def do_pop(): await item_to_remove.remove() if self.index is not None: - if index == self.index: + if normalized_index == self.index: self.index = self.index - elif index < self.index: + elif normalized_index < self.index: self.index = self.index - 1 return AwaitComplete(do_pop()) From 5a8dd24d5f1bc0c90153dc39a62f41841eda430d Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 17 Oct 2024 22:42:51 +0100 Subject: [PATCH 04/11] fix(listview): update index after remove_items --- src/textual/widgets/_list_view.py | 20 +++++++++++--- tests/listview/test_listview_remove_items.py | 28 ++++++++++++++++++++ 2 files changed, 45 insertions(+), 3 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index d4783dd074..1981d38d00 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -260,7 +260,7 @@ async def do_pop(): return AwaitComplete(do_pop()) - def remove_items(self, indices: Iterable[int]) -> AwaitRemove: + def remove_items(self, indices: Iterable[int]) -> AwaitComplete: """Remove ListItems from ListView by indices Args: @@ -271,8 +271,22 @@ def remove_items(self, indices: Iterable[int]) -> AwaitRemove: """ items = self.query("ListItem") items_to_remove = [items[index] for index in indices] - await_remove = self.remove_children(items_to_remove) - return await_remove + normalized_indices = set( + index if index >= 0 else index + len(self) for index in indices + ) + + async def do_remove_items(): + await self.remove_children(items_to_remove) + if self.index is not None: + removed_before_highlighted = sum( + 1 for index in normalized_indices if index < self.index + ) + if removed_before_highlighted: + self.index -= removed_before_highlighted + elif self.index in normalized_indices: + self.index = self.index + + return AwaitComplete(do_remove_items()) def action_select_cursor(self) -> None: """Select the current item in the list.""" diff --git a/tests/listview/test_listview_remove_items.py b/tests/listview/test_listview_remove_items.py index c6436835d1..48a03633dd 100644 --- a/tests/listview/test_listview_remove_items.py +++ b/tests/listview/test_listview_remove_items.py @@ -83,6 +83,34 @@ async def test_listview_pop_updates_index_and_highlighting( assert app.highlighted == expected_highlighted +@pytest.mark.parametrize( + "initial_index, remove_indices, expected_new_index, expected_highlighted", + [ + (2, [2], 2, ["2", "3"]), # Remove highlighted item + (0, [0], 0, ["0", "1"]), # Remove first item when highlighted + (8, [-1], 7, ["8", "7"]), # Remove last item when highlighted + (4, [2, 1], 2, ["4", "4"]), # Remove items before the highlighted index + (4, [-2, 5], 4, ["4"]), # Remove items after the highlighted index + (4, range(0, 9), None, ["4", None]), # Remove all items + ], +) +async def test_listview_remove_items_updates_index_and_highlighting( + initial_index, remove_indices, expected_new_index, expected_highlighted +) -> None: + """Regression test for https://github.com/Textualize/textual/issues/5114""" + app = ListViewApp(initial_index) + async with app.run_test() as pilot: + listview = pilot.app.query_one(ListView) + + await listview.remove_items(remove_indices) + await pilot.pause() + + assert listview.index == expected_new_index + if expected_new_index is not None: + assert listview._nodes[expected_new_index].highlighted is True + assert app.highlighted == expected_highlighted + + if __name__ == "__main__": app = ListViewApp() app.run() From d85fdecf52f7199720068f5222d870bc2d7a40a3 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 17 Oct 2024 23:01:14 +0100 Subject: [PATCH 05/11] update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 35395ba0ef..2f23106826 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,11 +10,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Fixed `RadioSet` not being scrollable https://github.com/Textualize/textual/issues/5100 +- Fixed `ListView` not updating its index or highlighting after removing items https://github.com/Textualize/textual/issues/5114 ### Added - Added `background-tint` CSS rule https://github.com/Textualize/textual/pull/5117 +### Changed + +- `ListView.pop` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 +- `ListView.remove_items` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 + ## [0.83.0] - 2024-10-10 ### Added From 68e205ee4f8dad0d543752340eacbb8cd92b2033 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Thu, 24 Oct 2024 22:31:03 +0100 Subject: [PATCH 06/11] reinstate always_update to index reactive --- src/textual/widgets/_list_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index afe0620156..fc58927085 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -39,7 +39,7 @@ class ListView(VerticalScroll, can_focus=True, can_focus_children=False): | down | Move the cursor down. | """ - index = reactive[Optional[int]](None, init=False) + index = reactive[Optional[int]](None, always_update=True, init=False) """The index of the currently highlighted item.""" class Highlighted(Message): From 336a954eea7c69b4e39caf18987bb7e326b8e986 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Mon, 28 Oct 2024 17:44:49 +0000 Subject: [PATCH 07/11] Revert "reinstate always_update to index reactive" This reverts commit 68e205ee4f8dad0d543752340eacbb8cd92b2033. --- src/textual/widgets/_list_view.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index fc58927085..afe0620156 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -39,7 +39,7 @@ class ListView(VerticalScroll, can_focus=True, can_focus_children=False): | down | Move the cursor down. | """ - index = reactive[Optional[int]](None, always_update=True, init=False) + index = reactive[Optional[int]](None, init=False) """The index of the currently highlighted item.""" class Highlighted(Message): From d330ecd656687bc508cf1854ebd1c0e4d07511e4 Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Mon, 28 Oct 2024 18:58:02 +0000 Subject: [PATCH 08/11] handle unchanged index without always_update --- src/textual/widgets/_list_view.py | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index afe0620156..a2ebbacc3e 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -268,10 +268,16 @@ def pop(self, index: Optional[int] = None) -> AwaitComplete: async def do_pop(): await item_to_remove.remove() if self.index is not None: - if normalized_index == self.index: + if normalized_index < self.index: + self.index -= 1 + elif normalized_index == self.index: + old_index = self.index + # Force a re-validation of the index self.index = self.index - elif normalized_index < self.index: - self.index = self.index - 1 + # If the index hasn't changed, the watcher won't be called + # but we need to update the highlighted item + if old_index == self.index: + self.watch_index(old_index, self.index) return AwaitComplete(do_pop()) @@ -299,7 +305,13 @@ async def do_remove_items(): if removed_before_highlighted: self.index -= removed_before_highlighted elif self.index in normalized_indices: + old_index = self.index + # Force a re-validation of the index self.index = self.index + # If the index hasn't changed, the watcher won't be called + # but we need to update the highlighted item + if old_index == self.index: + self.watch_index(old_index, self.index) return AwaitComplete(do_remove_items()) From 7287f337270ac320d72c42ea93ea4447e1f7efeb Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Fri, 15 Nov 2024 18:14:49 +0000 Subject: [PATCH 09/11] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4dd37d137..5a8db361d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed duplicated key displays in the help panel https://github.com/Textualize/textual/issues/5037 - Fixed `TextArea` mouse selection with tab characters https://github.com/Textualize/textual/issues/5212 +- Fixed `ListView` not updating its index or highlighting after removing items https://github.com/Textualize/textual/issues/5114 ### Added @@ -81,7 +82,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Fixed - Fixed glitchy ListView https://github.com/Textualize/textual/issues/5163 -- Fixed `ListView` not updating its index or highlighting after removing items https://github.com/Textualize/textual/issues/5114 ## [0.84.0] - 2024-10-22 From 9a7d30ba44477b9027ebeb2ffe55f8eb195472ba Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Sun, 17 Nov 2024 09:00:35 +0000 Subject: [PATCH 10/11] update changelog --- CHANGELOG.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a3abe061c..9d0a8aa978 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fixed `ListView` not updating its index or highlighting after removing items https://github.com/Textualize/textual/issues/5114 +### Changed + +- `ListView.pop` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 +- `ListView.remove_items` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 + ## [0.86.1] - 2024-11-16 ### Fixed @@ -112,11 +117,6 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added `background-tint` CSS rule https://github.com/Textualize/textual/pull/5117 - Added `:first-of-type`, `:last-of-type`, `:odd`, and `:even` pseudo classes https://github.com/Textualize/textual/pull/5139 -### Changed - -- `ListView.pop` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 -- `ListView.remove_items` now returns `AwaitComplete` rather than `AwaitRemove` https://github.com/Textualize/textual/pull/5135 - ## [0.83.0] - 2024-10-10 ### Added From 9314cde1113bf43bf8e5e9ebcbc964f9a7b5d62d Mon Sep 17 00:00:00 2001 From: TomJGooding <101601846+TomJGooding@users.noreply.github.com> Date: Sun, 17 Nov 2024 09:16:18 +0000 Subject: [PATCH 11/11] add docstrings --- src/textual/widgets/_list_view.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py index fb1f4540c7..97d2c150bb 100644 --- a/src/textual/widgets/_list_view.py +++ b/src/textual/widgets/_list_view.py @@ -300,7 +300,8 @@ def pop(self, index: Optional[int] = None) -> AwaitComplete: item_to_remove = self.query("ListItem")[index] normalized_index = index if index >= 0 else index + len(self) - async def do_pop(): + async def do_pop() -> None: + """Remove the item and update the highlighted index.""" await item_to_remove.remove() if self.index is not None: if normalized_index < self.index: @@ -331,7 +332,8 @@ def remove_items(self, indices: Iterable[int]) -> AwaitComplete: index if index >= 0 else index + len(self) for index in indices ) - async def do_remove_items(): + async def do_remove_items() -> None: + """Remove the items and update the highlighted index.""" await self.remove_children(items_to_remove) if self.index is not None: removed_before_highlighted = sum(