Textualize / textual

The lean application framework for Python. Build sophisticated user interfaces with a simple Python API. Run your apps in the terminal and a web browser.
https://textual.textualize.io/
MIT License
25.76k stars 795 forks source link

DuplicateIds exception raised when removing a tab and adding a new tab #5215

Open mathman1618 opened 2 weeks ago

mathman1618 commented 2 weeks ago

I have an app in which multiple tabs of a TabbedContent widget may be created on startup. These tabs can also be closed/removed by the user. If two (or more) tabs are created at startup, closing a tab other than the last followed by creating a new tab crashes the app with an exception like the following:

DuplicateIds: Tried to insert a widget with ID '--content-tab-tab-2', but a widget ContentTab(id='--content-tab-tab-2') already exists with that ID in this list of children. The children of a widget must have unique IDs.

My code adds new tabs via TabbedContent.add_pane and removes them via TabbedContent.remove_pane.

I think the exception is due to how new TabPane ids are generated by TabbedContent._set_id here: https://github.com/Textualize/textual/blob/09003dfbc0ff2879811cdc0b33dfdb26731c2cb1/src/textual/widgets/_tabbed_content.py#L427

Textual Diagnostics

Versions

Name Value
Textual 0.85.2
Rich 13.7.1

Python

Name Value
Version 3.12.7
Implementation CPython
Compiler GCC 11.4.0
Executable /home/justin/.pyenv/versions/3.12.7/bin/python3.12

Operating System

Name Value
System Linux
Release 6.8.0-47-generic
Version #⁠47~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Wed Oct 2 16:16:55 UTC 2

Terminal

Name Value
Terminal Application tmux (3.2a)
TERM xterm-direct
COLORTERM truecolor
FORCE_COLOR Not set
NO_COLOR Not set

Rich Console options

Name Value
size width=135, height=72
legacy_windows False
min_width 1
max_width 135
is_terminal False
encoding utf-8
max_height 72
justify None
overflow None
no_wrap False
highlight None
markup None
height None
darrenburns commented 2 weeks ago

Are you awaiting the calls to add_pane and remove_pane?

await remove_pane(...)
await add_pane(...)
mathman1618 commented 2 weeks ago

No, I wasn't awaiting. I just changed my code to await both the add_pane and remove_pane calls, but the same exception is raised.

mathman1618 commented 2 weeks ago

I think what's happening is something like this:

  1. Start the app with two tabs, 'tab-1' and 'tab-2' (the ids are really '--content-tab-tab-1' and '--content-tab-tab-2', but I'll just use the last part for brevity). The node list for the parent widget of the TabbedContent holds these two ids.
  2. Close the first tab. 'tab-1' is removed from the node list; 'tab-2' remains.
  3. Try to open a new tab. The _set_id method is called to assign a new id, which is computed from tabs.tab_count + 1.
  4. Since there is only one tab present, the last expression evaluates to 2 and _set_id returns an id like 'tab-2', identical to the id of the still-open tab.
  5. When the tab is mounted, the id 'tab-2' is added to the appropriate node list, but this list already contains the id 'tab-2', hence the exception.
TomJGooding commented 2 weeks ago

I've created a quick MRE to demonstrate the issue. This also revealed that the tabs highlighting isn't updated correctly when the tab is removed.

from textual.app import App, ComposeResult
from textual.widgets import Footer, Label, TabbedContent, TabPane

class ExampleApp(App):
    BINDINGS = [
        ("r", "remove_pane", "Remove first pane"),
        ("a", "add_pane", "Add pane"),
    ]

    def compose(self) -> ComposeResult:
        with TabbedContent(initial="tab-2"):
            with TabPane("tab-1"):
                yield Label("tab-1")
            with TabPane("tab-2"):
                yield Label("tab-2")
        yield Footer()

    def action_remove_pane(self) -> None:
        tabbed_content = self.query_one(TabbedContent)
        tabbed_content.remove_pane("tab-1")

    def action_add_pane(self) -> None:
        tabbed_content = self.query_one(TabbedContent)
        new_pane = TabPane("tab-3", Label("tab-3"))
        tabbed_content.add_pane(new_pane)

if __name__ == "__main__":
    app = ExampleApp()
    app.run()
willmcgugan commented 2 weeks ago

This may be an easy fix. That reference to tab_count is obviously flawed. The fix may be to replace it with a incrementing integer.

TomJGooding commented 2 weeks ago

That was my initial reaction too, but worried that those default ID's might start to get confusing. But I suppose if you want to access the TabPanes programmatically, you should really be supplying your own ID's anyway.

TomJGooding commented 2 weeks ago

After looking into this a bit deeper, it seems the Tabs widget already assigns ID's based on a counter. However this counter only increments when the tab ID hasn't been provided.

Should the TabbedContent reflect this setting of ID's? This would make these widgets more consistent, but might be a breaking change.

mathman1618 commented 2 weeks ago

As a workaround, I've changed my app to store an incrementing counter and use it to generate explicit IDs for the TabPane instances, as suggested above.