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

Populating a ListView via constructor vs via inherit and compose #1588

Closed
davep opened this issue Jan 17, 2023 · 2 comments · Fixed by #1600
Closed

Populating a ListView via constructor vs via inherit and compose #1588

davep opened this issue Jan 17, 2023 · 2 comments · Fixed by #1600
Assignees
Labels
bug Something isn't working

Comments

@davep
Copy link
Contributor

davep commented Jan 17, 2023

There seems to be a subtle difference in the working of a ListView if you create one by passing the ListItems to it, vs if you create a custom ListView by inheriting from it and using compose to populate it. Take the following example code, which places both approaches side-by-side:

from textual.app        import App, ComposeResult
from textual.containers import Horizontal
from textual.widgets    import Header, Footer, ListView, ListItem, Label

class CustomListView( ListView ):

    def __init__( self, choices: list[ str ] ) -> None:
        super().__init__()
        self._choices = choices

    def compose( self ) -> ComposeResult:
        """Compose the child widgets."""
        for choice in self._choices:
            yield ListItem( Label( choice ) )

class ListViewMakerApp( App[ None ] ):

    CSS = """
    ListView {
        width: 1fr;
        height: 1fr;
        border: round red;
    }
    """

    OPTIONS = [ f"This is the nifty test option {n}" for n in range( 20 ) ]

    def compose( self ) -> ComposeResult:
        yield Header()
        yield Horizontal(
            ListView(
                *[ ListItem( Label( option ) ) for option in self.OPTIONS ]
            ),
            CustomListView( self.OPTIONS )
        )
        yield Footer()

if __name__ == "__main__":
    ListViewMakerApp().run()

I feel the CustomListView would not be an unusual approach for people to take; perhaps wanting to make their own commonly-used selection list of simple values (or even very complex child values that they want easily handled). Side-by-side the ListViews look the same to start with:

Screenshot 2023-01-17 at 11 10 47

Note that the non-inherited ListView is to the left and that, even though it doesn't have focus, the first item is highlighted. If you tab into it you can cursor around, etc, just fine. On the other hand notice that the right ListView (which is a CustomListView) has no obvious highlighted item and if you tab into it nothing gets highlighted.

Further to this, if you (via keyboard) focus the inherited (right hand) ListView and then attempt to cursor down, the following exception will be raised:

╭─────────────────────────────── Traceback (most recent call last) ────────────────────────────────╮
│ /Users/davep/develop/python/textual-sandbox/.venv/lib/python3.10/site-packages/textual/widgets/_ │
│ list_view.py:127 in action_cursor_down                                                           │
│                                                                                                  │
│   124 │   │   self.emit_no_wait(self.Selected(self, selected_child))                             │
│   125 │                                                                                          │
│   126 │   def action_cursor_down(self) -> None:                                                  │
│ ❱ 127 │   │   self.index += 1                                                                    │
│   128 │                                                                                          │
│   129 │   def action_cursor_up(self) -> None:                                                    │
│   130 │   │   self.index -= 1                                                                    │
│                                                                                                  │
│ ╭──────────────────────────── locals ─────────────────────────────╮                              │
│ │ self = CustomListView(pseudo_classes={'focus', 'focus-within'}) │                              │
│ ╰─────────────────────────────────────────────────────────────────╯                              │
╰──────────────────────────────────────────────────────────────────────────────────────────────────╯
TypeError: unsupported operand type(s) for +=: 'NoneType' and 'int'
@davep davep added the bug Something isn't working label Jan 17, 2023
@Textualize Textualize deleted a comment from github-actions bot Jan 17, 2023
@davep
Copy link
Contributor Author

davep commented Jan 17, 2023

The quickest of fixes, which I'd like to confirm with @darrenburns first, is this:

diff --git a/src/textual/widgets/_list_view.py b/src/textual/widgets/_list_view.py
index 062986cb..1ea7100d 100644
--- a/src/textual/widgets/_list_view.py
+++ b/src/textual/widgets/_list_view.py
@@ -52,6 +52,9 @@ class ListView(Vertical, can_focus=True, can_focus_children=False):
         super().__init__(*children, name=name, id=id, classes=classes)
         self.index = initial_index

+    def on_mount( self ) -> None:
+        self.index = self.index or 0
+
     @property
     def highlighted_child(self) -> ListItem | None:
         """ListItem | None: The currently highlighted ListItem,

@davep davep self-assigned this Jan 18, 2023
davep added a commit to davep/textual that referenced this issue Jan 18, 2023
Added to test the fail in Textualize#1588. Any fix for this should cause this test to pass.
davep added a commit to davep/textual that referenced this issue Jan 18, 2023
davep added a commit to davep/textual that referenced this issue Jan 18, 2023
This seeks to fix Textualize#1588 by ensuring that the index property is set to an
acceptable value after any items have been mounted within the ListView, thus
ensuring that an inherited ListView, which uses compose to add items rather
than having them passed via __init__, works as expected.
@github-actions
Copy link

Don't forget to star the repository!

Follow @textualizeio for Textual updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant