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

Allow Drivers to handle key events after being restarted #1150

Merged
merged 3 commits into from
Nov 23, 2022

Conversation

JoshKarpel
Copy link
Contributor

Change: clear driver exit events after key-handling threads are joined in the two concrete Driver implementations, which allows Driver.start_application_mode() to be called again successfully after Driver.stop_application_mode().

This lets something like this work:

import sys
import time
from contextlib import contextmanager, redirect_stdout, redirect_stderr
from typing import Iterator

from textual.app import App, ComposeResult
from textual.widgets import Footer, Header


class SuspendApp(App):
    BINDINGS = [
        ("d", "toggle_dark", "Toggle dark mode"),
        ("s", "suspend", "Suspend and print a message"),
    ]

    def compose(self) -> ComposeResult:
        """Create child widgets for the app."""
        yield Header()
        yield Footer()

    def action_toggle_dark(self) -> None:
        """An action to toggle dark mode."""
        self.dark = not self.dark

    def action_suspend(self) -> None:
        with self.suspend():
            print("Hi!")
            print("Resuming soon...")
            time.sleep(2)

    @contextmanager
    def suspend(self) -> Iterator[None]:
        driver = self._driver

        if driver is not None:
            driver.stop_application_mode()
            with redirect_stdout(sys.__stdout__), redirect_stderr(sys.__stderr__):
                yield
            driver.start_application_mode()


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

Without this change, when application modes resumes at the end of SuspendApp.suspend, the Driver's key thread will be started but will immediately stop because the exit event is still set, so the bindings won't work anymore. With this change, bindings work after the suspend finishes.

I put the event reset immediately after the key thread is joined to follow the general feeling of Driver.stop_application_mode trying to put the system back in its initial state.


I made a few attempts to write tests for this but was stymied by the dependence of the Driver on the event loop and a message pump to send message to - I didn't see an existing pattern in the test suite to copy for tests that rely on the event loop or message pumps, but maybe I just missed it?

I've tested manually on Linux locally (using the above example script), but not on Windows or Mac. Happy to continue hacking on tests for this behavior since it'd be easy to inadvertently regress it, but I think I need some guidance on how to do so.

(For context, I'm using this trick to play around with dropping out of the Textual app and into another CLI app, namely IPython https://github.com/JoshKarpel/spiel/blob/9da45e1f22b4a284ac9691fa9125329598ed28ad/spiel/app.py#L151-L176)

@@ -249,4 +250,4 @@ class MyApp(App):
async def on_mount(self, event: events.Mount) -> None:
self.set_timer(5, callback=self._close_messages)

MyApp.run()
MyApp().run()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated, just noticed this bug while poking around.

@costrouc
Copy link

@JoshKarpel can confirm this works when I tested it on my code to drop the user into an editor and resuming textual. This will resolve issues #1148, #1093 and discussion #165.

@darrenburns
Copy link
Member

Thanks for this @JoshKarpel, it looks really useful - we'll try and review it soon!

# Conflicts:
#	src/textual/drivers/linux_driver.py
@darrenburns
Copy link
Member

Can confirm this fixed the issue inside my file manager app :)

@willmcgugan
Copy link
Collaborator

Thanks @JoshKarpel

@darrenburns Care to merge once tests pass?

@darrenburns darrenburns merged commit 10a3fb1 into Textualize:main Nov 23, 2022
@JoshKarpel
Copy link
Contributor Author

Thanks all!

@JoshKarpel JoshKarpel deleted the reentrant-drivers branch November 23, 2022 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants