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

Updating a OptionList that contains separator lines can result in a garbled display (since 0.86.0) #5431

Open
davep opened this issue Dec 24, 2024 · 6 comments · May be fixed by #5478
Open

Comments

@davep
Copy link
Contributor

davep commented Dec 24, 2024

Take this code:

from textual import on
from textual.app import App, ComposeResult
from textual.containers import Horizontal
from textual.reactive import var
from textual.widgets import OptionList, Button

class FocusTest(App[None]):

    CSS = """
    OptionList {
        height: 1fr;
    }
    """

    counter: var[int] = var(0)

    def compose(self) -> ComposeResult:
        with Horizontal():
            yield Button("Add")
            yield OptionList()

    @on(Button.Pressed)
    def add_more_stuff(self) -> None:
        self.counter += 1
        self.query_one(OptionList).add_option(
            (f"This is option {self.counter}" if self.counter % 2 else None)
        )

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

if you use the Add button, and keep focus on the Add button, the display within the OptionList starts to make no sense; only when it receives focus does it show correctly:

Screen.Recording.2024-12-24.at.17.06.05.mov

Going back through recent Textual releases, this effect first appears in v0.86.

@davep davep changed the title Updating an unfocused OptionList that contains seperater lines can result in a garbled display (since 0.86.0) Updating a OptionList that contains separator lines can result in a garbled display (since 0.86.0) Dec 24, 2024
@davep
Copy link
Contributor Author

davep commented Dec 24, 2024

Following on from the above, it's not actually that it's being updated when it's focused; it seems to be if it's being updated at all. For example, this variation of the above code:

from textual.app import App, ComposeResult
from textual.reactive import var
from textual.widgets import OptionList

class FocusTest(App[None]):

    CSS = """
    OptionList {
        height: 1fr;
    }
    """

    counter: var[int] = var(0)

    BINDINGS = [
        ("space", "add_more")
    ]

    def compose(self) -> ComposeResult:
        yield OptionList()

    def action_add_more(self) -> None:
        self.counter += 1
        self.query_one(OptionList).add_option(
            (f"This is option {self.counter}" if self.counter % 2 else None)
        )

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

shows the same problem if you keep pressing the space key.

@TomJGooding
Copy link
Contributor

Nice to see you Dave! Looks possibly like some weirdness with the OptionList._content_render_cache, as while hovering the option will show correctly?

@davep
Copy link
Contributor Author

davep commented Dec 24, 2024

Yup, very much what I'm seeing too.

davep added a commit to davep/braindrop that referenced this issue Dec 25, 2024
@TomJGooding
Copy link
Contributor

I think the problem is that the index for the options are different in _render_option_content when called from _add_lines and render_line.

For example, "This is option 5" is cached with an option index of 4 from _add_lines, but then in render_line will have option index of 2.

This seems to fix it, but I'm not sure what is the 'correct' index...?

diff --git a/src/textual/widgets/_option_list.py b/src/textual/widgets/_option_list.py
index 6aeac230c..ff15a0cb3 100644
--- a/src/textual/widgets/_option_list.py
+++ b/src/textual/widgets/_option_list.py
@@ -349,7 +349,7 @@ class OptionList(ScrollView, can_focus=True):
             if isinstance(content, Option):
                 height = len(
                     self._render_option_content(
-                        index, content, "", width - self._left_gutter_width()
+                        option_index, content, "", width - self._left_gutter_width()
                     )
                 )

@davep
Copy link
Contributor Author

davep commented Dec 26, 2024

I've not dived into it, but it's interesting that the bit of code you pick out seems to also relate to getting the height of an option(?), as there's yet another subtle bug I'm seeing that only kicks in from 0.86, as far as I can see, where multi-line options sometimes don't fully render any more, unless you jiggle the width of the window (or presumably do something else that causes a full refresh or similar). I've been on and off trying (and failing) to come up with an MRE for that but right now I'm seeing it all the time in the app I'm working on.

@TomJGooding
Copy link
Contributor

TomJGooding commented Dec 26, 2024

_render_option_content returns the list of strips, so if the wrong index is used to try to retrieve the content from the cache it makes sense you'd see issues with the height too! I'm just not yet sure if the 'correct' index should relates to the option or the line.

EDIT: It looks like get_content_height also calls this _render_option_content method which is probably also a factor!

TomJGooding added a commit to TomJGooding/textual that referenced this issue Jan 16, 2025
@TomJGooding TomJGooding linked a pull request Jan 16, 2025 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants