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

Implements screen modes #2540

Merged
merged 8 commits into from
May 22, 2023
Merged

Implements screen modes #2540

merged 8 commits into from
May 22, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented May 10, 2023

As it stands, the modes "work" but the API feels a bit rough.
Discussion in the related issue: #2327

The app below lets you play with modes.
Press 1 and 2 to switch between modes 1 and 2.
Press P to push random fruit modals into the screen.
Press O to pop a screen from the stack.

Demo code.
from random import choice

from textual.app import App, ComposeResult
from textual.screen import Screen, ModalScreen
from textual.widgets import Footer, Header, Label


FRUITS = "apple mango strawberry banana peach pear melon watermelon".split()


class BaseScreen(Screen[None]):
    BINDINGS = [
        ("1", "one", "Mode 1"),
        ("2", "two", "Mode 2"),
        ("p", "push", "Push rnd scrn"),
        ("o", "pop_screen", "Pop"),
        ("r", "remove", "Remove mode 1"),
    ]

    def __init__(self, label):
        super().__init__()
        self.label = label

    def compose(self) -> ComposeResult:
        yield Header()
        yield Label(self.label)
        yield Footer()

    def action_one(self) -> None:
        self.app.switch_mode("one")

    def action_two(self) -> None:
        self.app.switch_mode("two")

    def action_push(self) -> None:
        self.app.push_screen(FruitModal())

    def action_remove(self) -> None:
        self.app.remove_mode("one")


class FruitModal(ModalScreen[None]):
    BINDINGS = [
        ("1", "one", "Mode 1"),
        ("2", "two", "Mode 2"),
        ("p", "push", "Push rnd scrn"),
        ("o", "pop_screen", "Pop"),
    ]

    def compose(self) -> ComposeResult:
        yield Label(choice(FRUITS))

    def action_one(self) -> None:
        self.app.switch_mode("one")

    def action_two(self) -> None:
        self.app.switch_mode("two")

    def action_push(self) -> None:
        self.app.push_screen(FruitModal())


class ModesApp(App[None]):
    MODES = ["one", "two"]

    def on_mount(self) -> None:
        self.switch_mode("two")
        self.push_screen(BaseScreen("two"))
        self.switch_mode("one")
        self.push_screen(BaseScreen("one"))


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

This will close #2327

@willmcgugan
Copy link
Collaborator

@rodrigogiraoserrao Where are we with this? Is it in progress or are you blocked?

@rodrigogiraoserrao
Copy link
Contributor Author

@rodrigogiraoserrao Where are we with this? Is it in progress or are you blocked?

I'm waiting for you to give feedback on this iteration of the API.

@rodrigogiraoserrao rodrigogiraoserrao marked this pull request as ready for review May 16, 2023 16:40
src/textual/app.py Outdated Show resolved Hide resolved
src/textual/app.py Show resolved Hide resolved
src/textual/app.py Outdated Show resolved Hide resolved
tests/test_screen_modes.py Show resolved Hide resolved
@rodrigogiraoserrao rodrigogiraoserrao self-assigned this May 17, 2023
@rodrigogiraoserrao rodrigogiraoserrao added the enhancement New feature or request label May 17, 2023
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! just one request.

@@ -680,15 +728,17 @@ def screen(self) -> Screen:
ScreenStackError: If there are no screens on the stack.
"""
try:
return self._screen_stack[-1]
return self._screen_stacks[self._current_mode][-1]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you have a property for self._screen_stacks[self._current_mode][-1] ? Seems to pop up in a few places. It could be name _screen_stack ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to ask for a property for self._screen_stacks[self._current_mode]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a screen_stack property that returns a copy of the current stack but in some places I want the actual stack.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a _screen_stack property then?

@rodrigogiraoserrao rodrigogiraoserrao merged commit d48a127 into main May 22, 2023
@rodrigogiraoserrao rodrigogiraoserrao deleted the screen-modes branch May 22, 2023 13:27
@@ -159,6 +159,38 @@ class ScreenStackError(ScreenError):
"""Raised when trying to manipulate the screen stack incorrectly."""


class ModeError(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed while looking in app.py at something else (a week or so after this PR was merged), all of the ModeError classes seem to have been duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in #2715.
What a weird thing, though...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement mode concept
3 participants