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

Hide event not working as advertised? #3460

Closed
davep opened this issue Oct 4, 2023 · 10 comments · Fixed by #4769
Closed

Hide event not working as advertised? #3460

davep opened this issue Oct 4, 2023 · 10 comments · Fixed by #4769
Labels
bug Something isn't working needs discussion question Further information is requested Task

Comments

@davep
Copy link
Contributor

davep commented Oct 4, 2023

As I read the docs for Hide, it's my understanding that this code should show show and hide notifications as I hit the spacebar:

from textual.app import App, ComposeResult
from textual.widgets import Label

class ShowHideLabel(Label):

    def on_show(self) -> None:
        self.notify("Show!")

    def on_hide(self) -> None:
        self.notify("Hide!")

class ShowHideApp(App[None]):

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

    def compose(self) -> ComposeResult:
        yield ShowHideLabel("Here I am")

    def action_toggle(self) -> None:
        self.query_one(ShowHideLabel).visible = not self.query_one(ShowHideLabel).visible

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

but I only ever see show events.

@davep davep added bug Something isn't working question Further information is requested Task labels Oct 4, 2023
davep added a commit to davep/textual-sandbox that referenced this issue Oct 4, 2023
@TomJGooding
Copy link
Contributor

TomJGooding commented Oct 5, 2023

I think the root of the problem might be here following commit a40300a. If I've understood correctly, won't hidden_widgets be empty?

map, widgets = self._arrange_root(parent, size, visible_only=False)
new_widgets = map.keys()
# Newly visible widgets
shown_widgets = new_widgets - old_widgets
# Newly hidden widgets
hidden_widgets = self.widgets - widgets

It looks like the Hide messages are posted by the screen here:

hidden, shown, resized = self._compositor.reflow(self, size)
Hide = events.Hide
Show = events.Show
for widget in hidden:
widget.post_message(Hide())

@willmcgugan
Copy link
Collaborator

I think so, yes. Widgets with visible = False are still considered in the layout, but filtered out somewhere else.

@TomJGooding
Copy link
Contributor

Well I've confirmed it wasn't that commit as changing back to visible_only=True doesn't work either.

I think best left to the experts, but it was interesting digging deeper into Textual!

@willmcgugan
Copy link
Collaborator

I think what is happening is that the sets for shown / hidden / resized ignore the visible property.

It would be nice if setting visible=False did produce a Hide in the same way as setting display=False. But only if it can be done in a way that doesn't impact performance too much.

Worth investigating.

@darrenburns darrenburns self-assigned this Nov 2, 2023
@darrenburns darrenburns removed their assignment Nov 6, 2023
@darrenburns
Copy link
Member

We need to discuss what the Hide event should do.

Right now, the docs says a Hide event will fire when a widget moves off-screen, which feels very wrong to me.

I think Hide should only fire when visibility or display is changed.

@willmcgugan
Copy link
Collaborator

The Hide event is sent when the widget was in the previous layout, but not in the new one. Essentially the logical inverse of the Show event.

visibility is a bit of a special case. If visibility is False it is still considered to be in the layout, but never actually drawn.

The logic is somewhat determined by how the compositor is implemented. It uses set operations to very efficiently figure out what is shown / hidden.

I'd be interested in understanding why you think it should work differently. Let's have a chat about it tomorrow!

@willmcgugan willmcgugan self-assigned this Nov 8, 2023
@willmcgugan willmcgugan removed their assignment Nov 21, 2023
@willmcgugan
Copy link
Collaborator

@darrenburns Does this need any work?

@darrenburns
Copy link
Member

@willmcgugan Yes, the example in the original post was never fixed. I started "fixing" it, but wasn't sure about the exact intended behaviour. The reason I was unsure was because of all the ways all the ways a widget could go from being visible to not visible - being hidden behind a docked widget, offset pushing it off screen, hidden behind an overlay widget, modal screen etc (also wasn't sure if the compositor handle all of these cases in the hidden/visible sets).

This behaviour of a Hide event firing in these situations also felt "wrong" to me in that it was too magical. I expected the Hide event to only fire when visible/display was explicitly set via code, rather than it possibly being sent by some other widget which happens to be rendered above it, hiding it from view.

Given all of this it felt better to raise it for discussion instead of guessing.

@willmcgugan
Copy link
Collaborator

Going to park this for now. I think there may be two concepts that are being conflated: Events when widgets come in to the viewport, and events when something is toggled. Until we have a use case we can consider, it is hard to know what the behaviour shoulld be.

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 needs discussion question Further information is requested Task
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants