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

fix(listview): update index after items removed #5135

Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4bd7edf
fix(listview): update index after pop
TomJGooding Oct 17, 2024
2abce6d
tests(listview): import future for type hints
TomJGooding Oct 17, 2024
643c2d9
ensure pop error is original index rather than normalized
TomJGooding Oct 17, 2024
5a8dd24
fix(listview): update index after remove_items
TomJGooding Oct 17, 2024
6d572c4
Merge branch 'main' into fix-listview-update-index-after-items-removed
TomJGooding Oct 17, 2024
d85fdec
update changelog
TomJGooding Oct 17, 2024
0a12640
Merge branch 'main' into fix-listview-update-index-after-items-removed
TomJGooding Oct 24, 2024
68e205e
reinstate always_update to index reactive
TomJGooding Oct 24, 2024
336a954
Revert "reinstate always_update to index reactive"
TomJGooding Oct 28, 2024
d330ecd
handle unchanged index without always_update
TomJGooding Oct 28, 2024
014bba1
Merge branch 'main' into fix-listview-update-index-after-items-removed
TomJGooding Nov 15, 2024
7287f33
update changelog
TomJGooding Nov 15, 2024
c51d4eb
Merge branch 'main' into fix-listview-update-index-after-items-removed
TomJGooding Nov 17, 2024
9a7d30b
update changelog
TomJGooding Nov 17, 2024
9314cde
add docstrings
TomJGooding Nov 17, 2024
5f8123e
update changelog
TomJGooding Nov 19, 2024
fc36e21
Merge branch 'main' into fix-listview-update-index-after-items-removed
willmcgugan Nov 20, 2024
25901b7
Merge branch 'main' into fix-listview-update-index-after-items-removed
darrenburns Nov 28, 2024
e507eb3
Merge branch 'main' into fix-listview-update-index-after-items-removed
darrenburns Nov 28, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -94,6 +95,11 @@ 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
Expand Down
58 changes: 48 additions & 10 deletions src/textual/widgets/_list_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from typing_extensions import TypeGuard

from textual._loop import loop_from_index
from textual.await_complete import AwaitComplete
from textual.await_remove import AwaitRemove
from textual.binding import Binding, BindingType
from textual.containers import VerticalScroll
Expand Down Expand Up @@ -275,7 +276,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

Expand All @@ -286,13 +287,30 @@ 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

def remove_items(self, indices: Iterable[int]) -> AwaitRemove:
if len(self) == 0:
raise IndexError("pop from empty list")

index = index if index is not None else -1
item_to_remove = self.query("ListItem")[index]
normalized_index = index if index >= 0 else index + len(self)

async def do_pop():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring and typing por favor. Since its internal, and essentially private, the docstrings can be written for future Textual core-devs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added docstrings in 9314cde, but feel free to tweak!

await item_to_remove.remove()
if self.index is not None:
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
# 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())

def remove_items(self, indices: Iterable[int]) -> AwaitComplete:
"""Remove ListItems from ListView by indices

Args:
Expand All @@ -303,8 +321,28 @@ 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():
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

docstrings please

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:
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())

def action_select_cursor(self) -> None:
"""Select the current item in the list."""
Expand Down
86 changes: 85 additions & 1 deletion tests/listview/test_listview_remove_items.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,31 @@
from __future__ import annotations

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")),
Expand All @@ -14,8 +37,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"""
Expand All @@ -27,6 +57,60 @@ 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


@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()
Loading