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

It's possible to somehow break the working of TabbedContent #2229

Closed
davep opened this issue Apr 6, 2023 · 7 comments · Fixed by #2305
Closed

It's possible to somehow break the working of TabbedContent #2229

davep opened this issue Apr 6, 2023 · 7 comments · Fixed by #2305
Assignees
Labels
bug Something isn't working Task

Comments

@davep
Copy link
Contributor

davep commented Apr 6, 2023

This one isn't exactly easy to explain, but hopefully easy to see and recreate with the following code (which is a distilled version of what I'm doing in a bigger application, where I found it).

Worth noting: the motivation here is that (in the application I'm working on) the TabbedContent is acting as a sidebar, where each pane has content that can be focused, and I want folk to be people to switch tabs without needing to navigate to the tab bar and back into the pane again. As such there's some bindings in place that call into the Tabs and uses their prev/next tab actions.

from textual.app        import App, ComposeResult
from textual.binding    import Binding
from textual.containers import Vertical
from textual.widgets    import Header, Footer, TabbedContent, TabPane, Tabs, DirectoryTree

class SelfFocusPane(TabPane):

    DEFAULT_CSS = """
    SelfFocusPane {
        height: 100% !important;
    }
    DirectoryTree {
        width: 100%;
        height: 100% !important;
    }
    """

    def compose( self ) -> ComposeResult:
        """Compose the child widgets."""
        yield DirectoryTree(".")

    def on_show( self ) -> None:
        self.query_one( DirectoryTree ).focus()

class TabbedContentIssueApp( App[ None ] ):

    CSS = """
    Screen {
        align: center middle;
    }

    Screen > Vertical {
        width: 42;
    }

    TabbedContent {
        border: round red;
        max-width: 40;
        height: 100%;
    }

    ContentSwitcher {
        height: 1fr !important;
    }
    """

    BINDINGS = [
        Binding( "shift+left", "previous", "Previous" ),
        Binding( "shift+right", "next", "Next" ),
    ]

    def compose( self ) -> ComposeResult:
        yield Header()
        with Vertical():
            with TabbedContent():
                for n in range( 6 ):
                    yield SelfFocusPane( f"Tab {n}")
        yield Footer()

    def on_mount(self) -> None:
        self.query_one(Tabs).focus()

    def action_previous(self) -> None:
        self.query_one(Tabs).action_previous_tab()

    def action_next(self) -> None:
        self.query_one(Tabs).action_next_tab()

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

In experimenting, it looks like the SelfFocusPane.on_show setting focus to the child of the pane is key here; remove that and I can't recreate the issue.

@davep davep added bug Something isn't working Task labels Apr 6, 2023
davep added a commit to davep/textual-sandbox that referenced this issue Apr 6, 2023
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Apr 12, 2023
@rodrigogiraoserrao
Copy link
Contributor

Hey @davep, can you be more specific about what I should see that is wrong, or how to trigger it exactly?

I opened the app, I used the arrow keys and Enter to move around the directory trees, I can use the bindings to shift to different tabs, and everything seems to be working just fine.

@davep
Copy link
Contributor Author

davep commented Apr 12, 2023

Try just holding the bound keys down and let it go full speed, letting it wrap around plenty of times. Hopefully the issue should appear.

If not I'll retest here with main.

@rodrigogiraoserrao
Copy link
Contributor

I held down Shift + Right for a bit, then I let go.
When I do, the underline will stay pretty much frozen somewhere (not necessarily aligned with a tab) and clicking the bindings in the footer will only work occasionally.
Is this more or less the issue you find?

@willmcgugan
Copy link
Collaborator

Yeah, that's it.

@rodrigogiraoserrao
Copy link
Contributor

I found a funnier way of triggering the issue:

Add this handler to your app:

class TabbedContentIssueApp(App[None]):
    # ...
    def key_p(self) -> None:
        self.action_next()
        self.action_next()

If you press P just once, the underline bar is going to take AGES to move two tabs to the right.
I haven't figured out exactly what funky interaction is happening, but I have a potential fix in the works.

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Apr 17, 2023

The actual issue can be more easily uncovered by pressing P on this app while connected to the devtools:

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


class MyApp(App[None]):
    def compose(self) -> ComposeResult:
        with TabbedContent():
            with TabPane("one"):
                yield Label("./")
            with TabPane("two"):
                yield Label("./")
            with TabPane("three", id="three"):
                yield Label("./")

    def key_p(self) -> None:
        self.query_one(Tabs).action_next_tab()
        self.query_one(Tabs).action_next_tab()


app = MyApp()

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

What's happening?

There is a potential loop between Tabs and TabbedContent.
Consider what happens when we call Tabs._move_tab:

  1. we end by setting self.active which is the reactive Tabs.active
  2. the watcher Tabs.watch_active posts a message TabActivated
  3. the TabbedContent handles it in TabbedContent._on_tabs_tab_activated and sets self.active which is the reactive TabbedContent.active
  4. the watcher sets Tabs.active again.

This generally isn't an issue and TabbedContent.active is reactive so that we can set the active tab programmatically on the TabbedContent.
However, if we move between tabs too quickly, we trigger two of those potential loops at the same time, and each loop will undo what the other did, creating an infinite chain of assignments to the reactive attributes and TabActivated messages posted.

willmcgugan pushed a commit that referenced this issue Apr 18, 2023
* Add regression test for #2229.

* Fix potential reactive-watch loop.

* Simplify regression test.

Labels are cheaper to use and the final visual result of the test won't depend on the directory it runs from.

* Simplify solution.

Turns out I didn't need a descriptor. :(

* Fail on empty tab.
@github-actions
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 Task
Projects
None yet
3 participants