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

inconsistent behavior Select Widged #2557

Closed
dominikhoebert opened this issue May 13, 2023 · 12 comments · Fixed by #2567
Closed

inconsistent behavior Select Widged #2557

dominikhoebert opened this issue May 13, 2023 · 12 comments · Fixed by #2567
Assignees
Labels
bug Something isn't working

Comments

@dominikhoebert
Copy link

Getting very inconsistent behavior with the new Select Widged when using their .set_options. Often after the options stay empty, depending on if the Select was used before, an item was selected before or not, or the options before where empy ([]).

from textual import on
from textual.app import App, ComposeResult
from textual.widgets import Select, Button


class SelectBug(App):

    def compose(self) -> ComposeResult:
        self.select = Select(options=[("empty", 0)])
        yield self.select
        yield Button("Change Options", id="change_options")

    @on(Button.Pressed, "#change_options")
    def change_options(self, event):
        self.select.set_options([("Four", 4), ("Five", 5), ("Six", 6)])


if __name__ == "__main__":
    app = SelectBug()
    app.run()

Keep the great work up!

@github-actions
Copy link

We found the following entry in the FAQ which you may find helpful:

Feel free to close this issue if you found an answer in the FAQ. Otherwise, please give us a little time to review.

This is an automated reply, generated by FAQtory

@willmcgugan willmcgugan added the bug Something isn't working label May 14, 2023
@willmcgugan
Copy link
Collaborator

@davep Suspect this is an issue with virtual_size in OptionList (but far from certain).

@willmcgugan
Copy link
Collaborator

The following diff fixes it. AFAICT.

@@ -631,7 +631,7 @@ class OptionList(ScrollView, can_focus=True):
         self.highlighted = None
         self._mouse_hovering_over = None
         self.virtual_size = Size(self.scrollable_content_region.width, 0)
-        self.refresh()
+        self._request_content_tracking_refresh()
         return self

     def _set_option_disabled(self, index: int, disabled: bool) -> Self:

If this is the fix, it just needs tests.

davep added a commit to davep/textual that referenced this issue May 15, 2023
@davep
Copy link
Contributor

davep commented May 15, 2023

Yup, looks good here.

@rodrigogiraoserrao rodrigogiraoserrao self-assigned this May 15, 2023
@davep davep linked a pull request May 15, 2023 that will close this issue
@willmcgugan
Copy link
Collaborator

@rodrigogiraoserrao I think Dave is doing this.

@davep
Copy link
Contributor

davep commented May 15, 2023

While looking at adding some useful testing for this, I've realised that the suggested fix, while it works, is slightly puzzling.

The change sets up a call to _refresh_content_tracking via _on_idle, which is fine; but beforehand in the code there's already a forced call to _refresh_content_tracking. On the surface this would seem to be the same thing, albeit a delayed refresh of the content tracking, rather than an immediate one.

@davep
Copy link
Contributor

davep commented May 15, 2023

Tracing it through so far, I'm starting to suspect it's tied in with the use of overlay: screen. The code that builds the contract tracking data for OptionList can't reasonably do this until its size.width is greater than 0 (or rather, it doesn't bother attempting to calculate any of that data until it is greater than 0).

If I'm reading things correctly the OptionList within Select has a size.width of 0 until it's expanded; upshot being that clearing and then rebuilding the OptionList as part of setting the options up doesn't result in the full rebuild because it can't happen.

An alternative fix (that I'm not proposing, just making a note of to explain things), would be this:

diff --git a/src/textual/widgets/_select.py b/src/textual/widgets/_select.py
index 25564af9..b9b5d0d4 100644
--- a/src/textual/widgets/_select.py
+++ b/src/textual/widgets/_select.py
@@ -329,6 +329,7 @@ class Select(Generic[SelectType], Vertical, can_focus=True):
         overlay = self.query_one(SelectOverlay)
         self.set_class(expanded, "-expanded")
         if expanded:
+            overlay._refresh_content_tracking(force=True)
             overlay.focus()
             if self.value is None:
                 overlay.select(None)

by this time the widget has width and so the underlying content data can be worked out.

@willmcgugan
Copy link
Collaborator

You could compare what happens if you remove overlay: screen from the select.

@davep
Copy link
Contributor

davep commented May 15, 2023

@willmcgugan Good call! Okay, it's not that. And now I look at it it's possibly more obvious: I suspect the problem is that the clear/add is happening while it's display: none, which presumably means it has no size at that point.

@davep
Copy link
Contributor

davep commented May 15, 2023

Makes me wonder if what's needed here (likely needed anyway) is a Show handler on OptionList that does:

self._request_content_tracking_refresh()

much like how Resize gets handled. Curiously though I'm not seeing _on_show or on_show get fired when SelectOverlay does from display: none to display: block. I think I would have expected that.

@davep
Copy link
Contributor

davep commented May 16, 2023

Not directly related, but while trying to work out why the SelectOverlay doesn't seem to get a Show event, I found that Hide never seems to happen: #2574.

davep added a commit to davep/textual that referenced this issue May 16, 2023
While the fix for Textualize#2557 likely isn't *the* fix (see Textualize#2582 for some context
around that), it is a fix that works for now. As such, with the change,
there was a double attempt to refresh the content tracking in the clearing
of options in the OptionList, which shouldn't be necessary.

This removes that.
davep added a commit to davep/textual that referenced this issue May 16, 2023
This helps test the practical impact of the fix added for Textualize#2557.
@davep davep removed a link to a pull request May 16, 2023
@davep davep linked a pull request May 16, 2023 that will close this issue
@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.

4 participants