List view #1143

darrenburns · 2022-11-09T09:45:44Z

darrenburns · 2022-11-21T15:18:57Z

src/textual/dom.py

@@ -259,7 +259,7 @@ def screen(self) -> "Screen":
        from .screen import Screen

        node = self
-        while node and not isinstance(node, Screen):
+        while node is not None and not isinstance(node, Screen):


This is the line that ultimately resulted in those weird NoScreen errors. Because ListView has a __len__, the condition was evaluating as False and terminating the search for the Screen.

Ah. Good catch!

darrenburns · 2022-11-21T15:20:23Z

src/textual/widgets/_list_view.py

+class ListView(Vertical, can_focus=True, can_focus_children=False):
+    DEFAULT_CSS = """
+    ListView {
+        scrollbar-size-vertical: 1;


Subjective, but I prefer width 1 scrollbars for scrollbars that aren't Screen-level.

It was more of an issue with the click target being too small. Suspect for that reason, we should keep it at 2 , even if the aesthetics suffer for it.

willmcgugan

Nice. Some requests to consider.

We're kind of finding our feet here. As we build these widgets, I think we will formulate some best practices / standards.

willmcgugan · 2022-11-23T09:26:59Z

src/textual/widgets/_list_item.py

+    ListItem > Widget :hover {
+        background: $boost;
+    }
+    ListView ListItem.--highlight {


Should we add a > here? For the (hopefully rare) scenario of nested ListViews

willmcgugan · 2022-11-23T09:28:25Z

src/textual/widgets/_list_view.py

+class ListView(Vertical, can_focus=True, can_focus_children=False):
+    DEFAULT_CSS = """
+    ListView {
+        scrollbar-size-vertical: 1;


It was more of an issue with the click target being too small. Suspect for that reason, we should keep it at 2 , even if the aesthetics suffer for it.

src/textual/widgets/_list_view.py

willmcgugan · 2022-11-23T09:32:12Z

src/textual/widgets/_list_view.py

+
+    @property
+    def highlighted_child(self) -> ListItem | None:
+        """Get the currently highlighted ListItem


I think we're going to adopt property style docstrings in this format:

"""ListItem | None: description"""

willmcgugan · 2022-11-23T09:34:08Z

src/textual/widgets/_list_view.py

+        self._scroll_highlighted_region()
+        self.emit_no_wait(self.Highlighted(self, new_child))
+
+    async def append(self, item: ListItem) -> None:


Could we make this non-async?

Use emit_no_wait and return the awaitable from the mount method.

Hopefully that won't break anything.

Ditto for clear

Technically yes, but I think this will cause a lot confusion/subtle bugs. It needs to be async because we need to await the call to remove inside it. Otherwise we'll send a ChildrenUpdated with undefined children (could contain children before or after update, who knows).

If I receive a ChildrenUpdated message, I won't know inside my on_children_updated handler if the children have actually been updated yet or not, and I'll have no easy way of awaiting it (I won't have access to the AwaitRemove in there). It would mean we can't actually trust that message any more since the state of the children is undefined. Doing anything to my ListView on receipt of this message would be dangerous.

Do we need a children update message? Generally it is the parent that is doing the updating, so it will know there has been an updated.

Do you have a use-case in mind?

I've removed it, don't think it's necessary.

It was to give the ancestors a single hook (the children update handler) that they could use when the children changed. e.g. a single place they can update a label to reflect the number of items in the list rather.

willmcgugan · 2022-11-23T09:34:39Z

src/textual/widgets/_list_view.py

+        self.index = self.children.index(event.sender)
+        self.emit_no_wait(self.Selected(self, event.sender))
+
+    def key_up(self, event: events.Key) -> None:


Are these used at all? We want to use bindings wherever possible.

Yeah, this is where the handling is actually done. The bindings for up/down are dummies just to populate the footer. As far as I know when you use bindings you have no way to stop the event and so it was doing Textuals default scrolling behaviour when you pressed up and down.

I've hit a similar issue several times now - maybe I'm just missing something though. In my case I actually very rarely want the scrolling to occur when up/down is pressed.

Ah, I think that is because the Widget class was using key events. I changed that in the tree pr to use bindings. You might find it will work with bindings now.

That works with bindings now, thanks.

docs/widgets/list_item.md

docs/widgets/list_view.md

tests/snapshot_tests/test_snapshots.py

src/textual/widgets/_list_view.py

Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>

As pointed out in Textualize#1649, a `ChildrenUpdated` message is documented but it doesn't exist in the code. It looks like it got added during development, then removed after it was realised it wasn't needed, but presumably it got left in the docs: Textualize#1143 (comment)

darrenburns added 14 commits November 7, 2022 11:05

Add ListView and ListItem skeletons

9cd6bda

Add Highlighted and Selected messages to ListView

93c7bf3

Add _list_view module and export types

f6111e7

Merge branch 'main' of github.com:Textualize/textual into list-view

9f46df4

Adding bindings to ListView

bab7889

Fix containers not being focusable

8c9baaf

Fix tests to allow for focusable containers

46bc494

Add test for non-focusable container with focusable children

d027adf

Fix a typo in a test

58b43ab

Merge branch 'main' of github.com:Textualize/textual into list-view

ed06e28

Merge branch 'support-focusable-containers' into list-view

ab5d9bd

Merge branch 'main' of github.com:Textualize/textual into list-view

17dc927

Add list view and docs

6042346

Merge branch 'main' of github.com:Textualize/textual into list-view

fb9502f

willmcgugan marked this pull request as draft November 9, 2022 14:45

darrenburns added 15 commits November 15, 2022 15:56

Merge branch 'main' of github.com:Textualize/textual into list-view

5bc1606

Switch back to reactives for list view highlighting

58cbf3a

Updates to list view, add append and a (currently broken) remove

d665a6e

Add _dom_lock to App

44cbce1

Merge branch 'main' of github.com:Textualize/textual into dom-lock

a131ac5

Remove in a task, lock the DOM

9572cbd

Use async with with lock

c1590fd

Merge branch 'dom-lock' into list-view

8b7670e

Merge branch 'main' of github.com:Textualize/textual into list-view

cc0a5e3

Remove unused handler

5ac3afd

Merge branch 'main' of github.com:Textualize/textual into list-view

a6c350b

Fix issue where ListView appeared detached from Screen

b5a1a17

Removing some print debugging

cbbfcb9

Adding example for list itemm

eafb9ec

Adding list view snapshot test with key presses

b21eb06

darrenburns commented Nov 21, 2022

View reviewed changes

Update snapshot

afafc05

darrenburns marked this pull request as ready for review November 21, 2022 15:58

darrenburns marked this pull request as draft November 21, 2022 15:59

darrenburns added 3 commits November 21, 2022 16:42

Fix click handler

c57f6b9

Fix snapshot test

be31894

Ensure mkdocs.yml is up to date with listview stuff

4e0c77e

darrenburns marked this pull request as ready for review November 22, 2022 10:36

willmcgugan requested changes Nov 23, 2022

View reviewed changes

darrenburns added 3 commits November 23, 2022 11:00

Some tidy up/changes from code review

1317d19

Merge branch 'main' of github.com:willmcgugan/textual into list-view

eb8c078

Update snapshot for ListView - keys no longer shown in footer

01cbd7b

rodrigogiraoserrao suggested changes Nov 23, 2022

View reviewed changes

darrenburns and others added 6 commits November 30, 2022 11:37

PR feedback

853d056

Update docs/widgets/list_item.md

4834807

Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>

Update docs/widgets/list_view.md

422f9be

Co-authored-by: Rodrigo Girão Serrão <5621605+rodrigogiraoserrao@users.noreply.github.com>

Merge branch 'list-view' of github.com:Textualize/textual into list-view

0698bc2

Merge branch 'main' of github.com:Textualize/textual into list-view

f0c4023

Use bindings for handling cursor

29f4887

darrenburns requested a review from willmcgugan November 30, 2022 13:19

rodrigogiraoserrao approved these changes Dec 1, 2022

View reviewed changes

willmcgugan approved these changes Dec 9, 2022

View reviewed changes

willmcgugan added 2 commits December 9, 2022 10:09

Merge branch 'main' into list-view

cebeafb

fix snapshot

24a182c

willmcgugan merged commit 54a2e3e into main Dec 9, 2022

willmcgugan deleted the list-view branch December 9, 2022 10:28

kratsg mentioned this pull request Jan 23, 2023

docs: ChildrenUpdated does not exist in code, but mentioned in docs #1649

Closed

davep mentioned this pull request Jan 24, 2023

Remove mention of ChildrenUpdated from the ListView docs #1652

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

List view #1143

List view #1143

darrenburns commented Nov 9, 2022 •

edited

Loading

darrenburns Nov 21, 2022 •

edited

Loading

willmcgugan Nov 21, 2022

darrenburns Nov 21, 2022

willmcgugan Nov 23, 2022

willmcgugan left a comment

willmcgugan Nov 23, 2022

willmcgugan Nov 23, 2022

willmcgugan Nov 23, 2022

willmcgugan Nov 23, 2022

darrenburns Nov 23, 2022

willmcgugan Nov 23, 2022

darrenburns Nov 30, 2022

willmcgugan Nov 23, 2022

darrenburns Nov 23, 2022

willmcgugan Nov 23, 2022

darrenburns Nov 30, 2022

List view #1143

List view #1143

Conversation

darrenburns commented Nov 9, 2022 • edited Loading

darrenburns Nov 21, 2022 • edited Loading

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

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

darrenburns commented Nov 9, 2022 •

edited

Loading

darrenburns Nov 21, 2022 •

edited

Loading