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

Confusing behaviour with @click links #3690

Open
davep opened this issue Nov 16, 2023 · 1 comment
Open

Confusing behaviour with @click links #3690

davep opened this issue Nov 16, 2023 · 1 comment
Labels
enhancement New feature or request Task

Comments

@davep
Copy link
Contributor

davep commented Nov 16, 2023

I feel the way that an action is found for a @click link is unintuitive. Consider this code:

from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.screen import Screen
from textual.widgets import Label

class ClickLabel(Label):

    def action_click(self) -> None:
        self.notify("ClickLabel.action_click reporting in!")

class Clicker(Vertical):

    def compose(self) -> ComposeResult:
        yield Label("1: [@click=app.click]Click on this for an app action to fire[/]")
        yield Label("2: [@click=screen.click]Click on this for a screen action to fire[/]")
        yield Label("3: [@click=click]Click on this for this container's action to fire[/]")
        yield ClickLabel("4: [@click=click]Click on this for the custom widget's action to fire[/]")

    def action_click(self) -> None:
        self.notify("Clicker.action_click reporting in!")


class MainScreen(Screen):

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

    def action_click(self) -> None:
        self.notify("Screen.action_click reporting in!")


class ClickBaitApp(App[None]):

    def on_mount(self) -> None:
        self.push_screen(MainScreen())

    def action_click(self) -> None:
        self.notify("App.action_click reporting in!")


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

Here's what happens when you click on each of those links:

  1. The App-level action is fired. This is expected.
  2. The screen-level action is fired. This is expected.
  3. The App-level action is fired; this is (I think) unexpected.
  4. The widget-level action is fired; this is expected.

I feel the way that situation 3 is handled is unintuitive. Surely it would make more sense to have actions that handle clicks looked for going up the DOM? Or if not that, how would we at least introduce a parent namespace too? I feel it's obvious an intuitive that someone would want to compose a collection of widgets into a container and have @click links in the child widgets handled by an action on the container.

@davep davep added enhancement New feature or request Task labels Nov 16, 2023
@davep davep changed the title Confusion behaviour with @click links Confusing behaviour with @click links Nov 16, 2023
davep added a commit to davep/textual-sandbox that referenced this issue Nov 16, 2023
@valentingregoire
Copy link
Contributor

I have another simple example where this bug occurrs:

class ViewerApp(App):
    def on_mount(self) -> None:
        self.push_screen(ViewerScreen())

    def action_next_song(self) -> None:
        self.screen.action_next_song()


class ViewerScreen(Screen):
    BINDINGS = [
        ("q", "app.quit", "Quit"),
        ("n", "next_song", "Next Song"),
    ]

    current_song_index: reactive[int] = reactive(0, recompose=True)

    def action_next_song(self) -> None:
        self.current_song_index += 1

    def compose(self) -> ComposeResult:
        # with VerticalScroll():
        with ContentSwitcher(initial="viewer_txt"):
            yield Label(str(self.current_song_index), id="viewer_txt")
            yield Markdown(
                "some markdown",
                id="viewer_md",
            )

        # yield Static("[@click=next_song]ns[/]", id="link_next_song", classes="link")
        yield Static("Go to [@click=next_song]Next Song[/] please!", id="link_next_song", classes="link")
        yield Header()
        yield Footer()

The action_next_song is not found, we have to catch it in the app rather than in the screen.

This was referenced May 18, 2024
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
Projects
None yet
Development

No branches or pull requests

2 participants