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.

Sign up for GitHub

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

Fix TabbedContent type errors #4243

Closed
wants to merge 2 commits into from
Closed

Fix TabbedContent type errors #4243

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6efb15c
Tidy a type error in TabbedContent.remove_pane
davep Feb 29, 2024
d6a62c8
Tidy a type error in TabbedContent.add_pane
davep Feb 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 15 additions & 7 deletions src/textual/widgets/_tabbed_content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,14 @@
from rich.text import Text, TextType
from typing_extensions import Final

from textual.await_remove import AwaitRemove

from ..app import ComposeResult
from ..await_complete import AwaitComplete
from ..css.query import NoMatches
from ..message import Message
from ..reactive import reactive
from ..widget import Widget
from ..widget import AwaitMount, Widget
from ._content_switcher import ContentSwitcher
from ._tabs import Tab, Tabs

Expand Down Expand Up @@ -430,13 +432,19 @@ def add_pane(
pane = self._set_id(pane, tabs.tab_count + 1)
assert pane.id is not None
pane.display = False

async def _add_part(awaitable: AwaitComplete | AwaitMount) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't seem right. If you need to wrap those awaitables in a coroutine, it suggests that maybe AwaitComplete is type incorrectly.

Maybe AwaitComplete needs to accept an Awaitable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the code was wrong anyway, you mean, and the hint tweak is moot? If so I'll close this. Was just a passing tidy-up anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It may be correct, but the typing error stems from AwaitComplete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. I'll close this and flag up that AwaitComplete might need a wee bit more tinkering.

await awaitable

return AwaitComplete(
tabs.add_tab(
ContentTab(pane._title, pane.id),
before=before if before is None else ContentTab.add_prefix(before),
after=after if after is None else ContentTab.add_prefix(after),
_add_part(
tabs.add_tab(
ContentTab(pane._title, pane.id),
before=before if before is None else ContentTab.add_prefix(before),
after=after if after is None else ContentTab.add_prefix(after),
)
),
self.get_child_by_type(ContentSwitcher).mount(pane),
_add_part(self.get_child_by_type(ContentSwitcher).mount(pane)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actually seems like a flaw. The AwaitCompete class runs its awaitables concurrently. This looks like something that should run in serial.

)

def remove_pane(self, pane_id: str) -> AwaitComplete:
Expand All @@ -449,7 +457,7 @@ def remove_pane(self, pane_id: str) -> AwaitComplete:
An optionally awaitable object that waits for the pane to be removed
and the Cleared message to be posted.
"""
removal_awaitables = [
removal_awaitables: list[AwaitComplete | AwaitRemove] = [
self.get_child_by_type(ContentTabs).remove_tab(
ContentTab.add_prefix(pane_id)
)
Expand Down
Loading