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

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

Open
mathman1618 opened this issue Nov 6, 2024 · 8 comments · May be fixed by #5239
Open

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

mathman1618 opened this issue Nov 6, 2024 · 8 comments · May be fixed by #5239

Comments

@mathman1618
Copy link

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:

pane = self._set_id(pane, tabs.tab_count + 1)

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
Copy link
Member

Are you awaiting the calls to add_pane and remove_pane?

await remove_pane(...)
await add_pane(...)

@mathman1618
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

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()

@Textualize Textualize deleted a comment from github-actions bot Nov 7, 2024
@willmcgugan
Copy link
Collaborator

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
Copy link
Contributor

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 added a commit to TomJGooding/textual that referenced this issue Nov 7, 2024
Fixes Textualize#5215 where removing
then adding a pane could crash with a `DuplicateIds` exception.

Rather than assigning the ID based on the *current* tab count, this adds
a `_cumulative_tab_count` to ensure added panes have a unique ID.
@TomJGooding
Copy link
Contributor

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
Copy link
Author

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.

@TomJGooding TomJGooding linked a pull request Nov 15, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants