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

Disabling a selected tab in TabbedContent leaves it selected #3148

Closed
davep opened this issue Aug 23, 2023 · 12 comments
Closed

Disabling a selected tab in TabbedContent leaves it selected #3148

davep opened this issue Aug 23, 2023 · 12 comments
Assignees
Labels
enhancement New feature or request Task wontfix This will not be worked on

Comments

@davep
Copy link
Contributor

davep commented Aug 23, 2023

With #3145 in mind and this applied:

diff --git a/src/textual/widgets/_tabbed_content.py b/src/textual/widgets/_tabbed_content.py
index e3d8d20c2..99d12ba1b 100644
--- a/src/textual/widgets/_tabbed_content.py
+++ b/src/textual/widgets/_tabbed_content.py
@@ -381,7 +381,7 @@ class TabbedContent(Widget):
         event.stop()
         tab_id = event.tab.id
         try:
-            self.query_one(f"TabPane#{tab_id}").disabled = True
+            self.get_child_by_id(f"TabPane#{tab_id}").disabled = True
         except NoMatches:
             return
 
@@ -404,7 +404,7 @@ class TabbedContent(Widget):
             Tabs.TabError: If there are any issues with the request.
         """
 
-        self.query_one(Tabs).disable(tab_id)
+        self.get_child_by_type(Tabs).disable(tab_id)
 
     def enable_tab(self, tab_id: str) -> None:
         """Enables the tab with the given ID.
@@ -416,7 +416,7 @@ class TabbedContent(Widget):
             Tabs.TabError: If there are any issues with the request.
         """
 
-        self.query_one(Tabs).enable(tab_id)
+        self.get_child_by_type(Tabs).enable(tab_id)
 
     def hide_tab(self, tab_id: str) -> None:
         """Hides the tab with the given ID.
@@ -428,7 +428,7 @@ class TabbedContent(Widget):
             Tabs.TabError: If there are any issues with the request.
         """
 
-        self.query_one(Tabs).hide(tab_id)
+        self.get_child_by_type(Tabs).hide(tab_id)
 
     def show_tab(self, tab_id: str) -> None:
         """Shows the tab with the given ID.
@@ -440,4 +440,4 @@ class TabbedContent(Widget):
             Tabs.TabError: If there are any issues with the request.
         """
 
-        self.query_one(Tabs).show(tab_id)
+        self.get_child_by_type(Tabs).show(tab_id)

consider this code:

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

class TabsInTabsApp(App[None]):

    def compose(self) -> ComposeResult:
        with TabbedContent(id="top-level"):
            with TabPane("One", id="one"):
                yield Label("This is tab one level one")
            with TabPane("Two", id="two"):
                yield Label("This is tab two level one")
            with TabPane("Go Deeper", id="deeper"):
                with TabbedContent():
                    with TabPane("One", id="one"):
                        yield Label("This is tab one level one")
                    with TabPane("Two", id="two"):
                        yield Label("This is tab two level one")

    def on_mount(self) -> None:
        top_level_tabs = self.query_one("#top-level", TabbedContent)
        top_level_tabs.disable_tab("one")

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

On startup the disabled tab will still be selected. Perhaps when a tab is disabled the next eligible tab should be selected (much like I believe it does if you remove a tab?).

@davep davep added enhancement New feature or request Task labels Aug 23, 2023
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Aug 23, 2023

Disabled tabs can be selected if they're disabled after being selected, and that's by design.

Inside on_mount things have already been mounted and you just happened to disable a tab that was already selected.

When a tab is disabled do we want to move automatically to the next one, then?

@davep
Copy link
Contributor Author

davep commented Aug 23, 2023

When a tab is disabled do we want to move automatically to the next one, then?

I sense it's something we should consider. IIRC don't we move focus of a widget when it is disabled? If so I feel we should attempt similar when it comes to tabs.

rodrigogiraoserrao added a commit that referenced this issue Aug 23, 2023
@rodrigogiraoserrao
Copy link
Contributor

IIRC don't we move focus of a widget when it is disabled?

Yes

If so I feel we should attempt similar when it comes to tabs.

You may be right.
Who do we talk to? 😆

@davep
Copy link
Contributor Author

davep commented Aug 23, 2023

I think we get to decide. :-)

@rodrigogiraoserrao
Copy link
Contributor

I'm all up for being consistent with how focus works in other situations.
Then, disabling a tab should move the active tab just like hiding does?

@davep
Copy link
Contributor Author

davep commented Aug 23, 2023

Then, disabling a tab should move the active tab just like hiding does?

I've not seen/tested hiding, but if it works as I'd expect then, yeah, I think that's likely what people would expect to happen (assuming disabled really is "can't be selected at all, no way, not at all").

@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Aug 23, 2023

You haven't seen hiding? That's because it's working correctly! 😆

assuming disabled really is "can't be selected at all, no way, not at all

That's it.

@rodrigogiraoserrao rodrigogiraoserrao self-assigned this Aug 23, 2023
@davep
Copy link
Contributor Author

davep commented Aug 23, 2023

You haven't seen hiding? That's because it's working correctly!

rimshot.gif

rodrigogiraoserrao added a commit that referenced this issue Aug 23, 2023
rodrigogiraoserrao added a commit that referenced this issue Aug 23, 2023
@rodrigogiraoserrao
Copy link
Contributor

rodrigogiraoserrao commented Aug 23, 2023

I think I'll go back on this.

There's precedent in other frameworks for a disabled tab that isn't automatically unselected, much like disabling a button wouldn't suddenly interrupt whatever actions were being carried out because the button was clicked previously.
Plus, in the edge case where all existing tabs are disabled, it looks very awkward if no pane is visible (even if disabled).

@davep do you think this is sensible at all or do you firmly believe we should deactivate a disabled tab?

@davep
Copy link
Contributor Author

davep commented Aug 23, 2023

I've got no firm view either way really (I was just refreshing my memory on how VCL did/does it and it's anything goes really, if I'm reading it correctly). Personally I'd favour least astonishment within the context of how disabled works more generally in Textual.

@rodrigogiraoserrao
Copy link
Contributor

I'm closing this as “wontfix” with the caveat that this was a close call and therefore the decision might be reversed in the future.

Arguments in favour of not deactivating the currently-active tab if it is disabled, in no particular order:

  • an argument alluding to how focus goes out of a widget if it is disabled suggested we should activate another tab immediately, but focus doesn't cycle out of the currently-active tab, so there's really no expectation for the focus to automatically jump to another widget in another tab if the currently active tab is disabled;
  • Qt behaves in this way; (win32 doesn't allow disabling tabs and I couldn't figure out what tkinter does from their docs)
  • the edge case where we deactivate all tabs would look really odd because the tabbed content would be “empty”

@rodrigogiraoserrao rodrigogiraoserrao closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2023
@rodrigogiraoserrao rodrigogiraoserrao added the wontfix This will not be worked on label Aug 29, 2023
@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
enhancement New feature or request Task wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants