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
24.07k stars 741 forks source link

priority binding order is not respected in the Footer #4639

Open darrenburns opened 3 weeks ago

darrenburns commented 3 weeks ago

The images in https://github.com/Textualize/textual/issues/4637 show the problem.

If you have some priority bindings, the order they appear in the BINDINGS is not respected in the Footer widget.

TomJGooding commented 2 weeks ago

Initially I assumed this would be related to the recent changes to the bindings and/or footer, but actually this seems to predate those changes.

Try running the example app below before and after Textual v0.56.0 (specifically commit 6c459a5).

Unfortunately that's where the trail goes cold, as this changed App.namespace_bindings property no longer exists after dynamic bindings were added in v0.61.0.

from textual.app import App, ComposeResult, RenderResult
from textual.binding import Binding
from textual.widget import Widget
from textual.widgets import Footer

class TestWidget(Widget, can_focus=True):
    DEFAULT_CSS = """
    TestWidget {
        border: tall $primary-background-lighten-3;
        width: 50%;
        height: 50%;
        align: center middle;
    }

    TestWidget:focus {
        border: tall $accent;
    }
    """

    def on_mount(self) -> None:
        self.border_title = self.__class__.__name__

    def render(self) -> RenderResult:
        return ""

class InnerWidget(TestWidget):
    BINDINGS = [
        Binding("3", "overridden", "OVERRIDDEN"),
        Binding("4", "fourth", "Fourth"),
    ]

class OuterWidget(TestWidget):
    BINDINGS = [
        Binding("2", "overridden", "OVERRIDDEN"),
        Binding("3", "third", "Third", priority=True),
    ]

    def compose(self) -> ComposeResult:
        yield InnerWidget()

class PriorityBindingsApp(App):
    CSS = """
    Screen {
        align: center middle;
    }
    """

    BINDINGS = [
        Binding("1", "first", "First"),
        Binding("2", "second", "Second", priority=True),
    ]

    AUTO_FOCUS = "InnerWidget"

    def compose(self) -> ComposeResult:
        yield OuterWidget()
        yield Footer()

if __name__ == "__main__":
    app = PriorityBindingsApp()
    app.run()
TomJGooding commented 2 weeks ago

This quick and dirty fix seems to work for the example app above and passes (almost) all tests[^1].

Unfortunately it looks like no regression test was added with 6c459a5, so I'm not sure whether this also accounts for the issue this commit intended to fix.

diff --git a/src/textual/screen.py b/src/textual/screen.py
index 07dc23571..d746c00fb 100644
--- a/src/textual/screen.py
+++ b/src/textual/screen.py
@@ -324,7 +324,7 @@ class Screen(Generic[ScreenResultType], Widget):
         """

         bindings_map: dict[str, ActiveBinding] = {}
-        for namespace, bindings in self._modal_binding_chain:
+        for namespace, bindings in reversed(self._modal_binding_chain):
             for key, binding in bindings.keys.items():
                 action_state = self.app._check_action_state(binding.action, namespace)
                 if action_state is False:

[^1]: The only failing test is test_example_five_by_five from the snapshot tests, where actually I think the footer bindings are in the wrong order?

TomJGooding commented 2 weeks ago

Unfortunately it looks like no regression test was added with 6c459a5, so I'm not sure whether this also accounts for the issue this commit intended to fix.

Sorry it looks like my quick and dirty fix wouldn't account for issue #4382 this intended to resolve, where the visibility of bindings in the app should be overridden by bindings defined on a screen.

Test App for Issue 4382 ```python from textual.app import App, ComposeResult from textual.binding import Binding from textual.screen import Screen from textual.widgets import Footer, Placeholder class HideBindingScreen(Screen): def compose(self) -> ComposeResult: yield Placeholder(self.__class__.__name__) yield Footer() class ShowBindingScreen(Screen): BINDINGS = [ Binding("p", "app.pop_screen", "SCREEN BINDING SHOWN"), ] def compose(self) -> ComposeResult: yield Placeholder(self.__class__.__name__) yield Footer() class ShowBindingApp(App): BINDINGS = [ Binding("p", "pop_screen", "APP BINDING HIDDEN", show=False), ] def on_mount(self) -> None: self.push_screen(HideBindingScreen()) self.push_screen(ShowBindingScreen()) if __name__ == "__main__": app = ShowBindingApp() app.run() ```