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

Handle other Tabs widgets as DOM descendants of a TabbedContent #3602

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

darrenburns
Copy link
Member

@darrenburns darrenburns commented Oct 27, 2023

The key change here is that when a TabbedContent creates its Tabs, it'll be able to easily identify messages from that instance.

This is important because TabbedContent should only be responding to messages from the Tabs it creates. Messages from other Tabs instances that may appear further down the DOM as descendants should be ignored.

Also fixes #3412, which was a crash.

@TomJGooding
Copy link
Contributor

TomJGooding commented Oct 27, 2023

Thanks Darren, this looks like a better way of handling this. I still don't quite understand how Tabs would still have an active tab after a Cleared message though which is why tests were failing on my attempt #3444 (comment).

I think the docstring is a bit misleading for _on_tabs_cleared, as this message is posted when there are no active tabs, not when all tabs are removed, so perhaps that should be updated.

@darrenburns
Copy link
Member Author

@TomJGooding I think on your attempt, the tab_count != 0 early return inside the tabs cleared wasn't correct, because a Cleared message is sent when all tabs are hidden (which feels wrong, but that's a different story).

So, when that watcher was called, the tab_count was actually 2, triggering the early return. Since this early return prevented TabbedContent.active from being updated, it remained stuck on its last value, and the test failed.

I don't think I've explained great, but the whole flow is very confusing with lots of messages, reactive updates, and side effects happening - it's really not easy to trace execution here.

@TomJGooding
Copy link
Contributor

Sorry just to confirm I meant my change to the early return in #3444 (comment). I tried both active and active_tab (which I always mix up!) but still had the failing tests, suggesting it still has an active tab? I agree the flow is a pretty confusing, so maybe I'm just misunderstanding something here.

@darrenburns
Copy link
Member Author

I think that's a race-condition. active_tab is updated after refresh (via call_after_refresh), so when you check it it may not have been updated yet. active is updated immediately, so maybe using active instead of active_tab would work there... but I'm not sure.

@darrenburns
Copy link
Member Author

Actually - this doesn't work as the Tabs isn't a direct child of the TabbedContent. 😞

…o that it can ignore messages from other, irrelevant Tabs instances that may exist as descendants deeper in the DOM. Also adds some tests to ensure corresponding issues are fixed.
@darrenburns darrenburns marked this pull request as ready for review October 27, 2023 14:45
Copy link
Contributor

@TomJGooding TomJGooding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Darren, this obviously was more involved than I expected from my attempt!

My only suggestion is could we also change this misleading docstring for _on_tabs_cleared please?

src/textual/widgets/_tabbed_content.py Outdated Show resolved Hide resolved
…TabbedContent - it now includes a note that the Cleared message is sent when all tabs are hidden.
@darrenburns darrenburns merged commit efbb655 into main Oct 30, 2023
23 checks passed
@darrenburns darrenburns deleted the tabs-assertionerror-fix branch October 30, 2023 13:10
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 this pull request may close these issues.

Assertion Error on Tabs in TabPane
3 participants