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

Add function to temporarily suspend application mode #1541

Closed
wants to merge 14 commits into from

Conversation

JoshKarpel
Copy link
Contributor

@JoshKarpel JoshKarpel commented Jan 10, 2023

This PR adds the App.suspend() method that I've been using in my Textual projects. This originally came up in #1150 (and related issues), and a few people have since asked how to do this kind of thing on Discord, so it seems that enough people want this behavior that supporting it in core Textual might make sense.

I added a section for it in the docs, but I wasn't really sure where to do it, so I just put it under the Actions documentation, assuming that most people would be interested in this in the context of something like "open a text editor when the user presses a key". One thing that was a little awkward is that "application mode" isn't really a term used in the docs, so I had to kind of dance around the language.

  • Docstrings on all new or modified functions / classes
  • Updated documentation
  • Updated CHANGELOG.md (where appropriate)

Comment on lines +261 to +262
import:
- https://docs.python.org/3/objects.inv
Copy link
Contributor Author

@JoshKarpel JoshKarpel Jan 10, 2023

Choose a reason for hiding this comment

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

This allows cross-linking to the standard library docs using the same syntax as internal links; used here https://github.com/Textualize/textual/pull/1541/files#diff-681b255c8a47359bc04668c27ab9770e5326a66af7931e4e267e8e205cb43aeeR148

Comment on lines 19 to 22
def action_open_repl(self):
with self.suspend():
repl = InteractiveConsole()
repl.interact()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In #1150 I used

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

as a reproduction case, which would simplify this example if desired, but I figured doing something interactive would be more interesting and inspiring.

@JoshKarpel JoshKarpel marked this pull request as ready for review January 10, 2023 23:10
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.

Nice.

Have you checked this against Windows? I never intended start and stop application mode to be repeatable, so I'm not certain it can start after being stopped.

@JoshKarpel
Copy link
Contributor Author

Have you checked this against Windows?

Good call; it does not seem to work the same way on Windows, at least with the REPL example. Investigating...

@JoshKarpel
Copy link
Contributor Author

JoshKarpel commented Jan 12, 2023

The main problem I was hitting was that, after getting out of the terminal (interesting issue with that; I'll get back to it in a moment), I get:

│ C:\Users\joshk\projects\textual\src\textual\drivers\win32.py:164 in enable_application_mode                            │
│                                                                                                                        │
│   161 │   terminal_out = sys.stdout                                                                                    │
│   162 │                                                                                                                │
│   163 │   current_console_mode_in = _get_console_mode(terminal_in)                                                     │
│ ❱ 164 │   current_console_mode_out = _get_console_mode(terminal_out)                                                   │
│   165 │                                                                                                                │
│   166 │   def restore() -> None:                                                                                       │
│   167 │   │   """Restore console mode to previous settings"""                                                          │
│                                                                                                                        │
│ ╭──────────────────────────────────────── locals ────────────────────────────────────────╮                             │
│ │ current_console_mode_in = 487                                                          │                             │
│ │             terminal_in = <_io.TextIOWrapper name='<stdin>' mode='r' encoding='utf-8'> │                             │
│ │            terminal_out = <textual.app._NullFile object at 0x000001BEE87E1D90>         │                             │
│ ╰────────────────────────────────────────────────────────────────────────────────────────╯                             │
│                                                                                                                        │
│ C:\Users\joshk\projects\textual\src\textual\drivers\win32.py:147 in _get_console_mode                                  │
│                                                                                                                        │
│   144 │   Returns:                                                                                                     │
│   145 │   │   int: The current console mode.                                                                           │
│   146 │   """                                                                                                          │
│ ❱ 147 │   windows_filehandle = msvcrt.get_osfhandle(file.fileno())                                                     │
│   148 │   mode = wintypes.DWORD()                                                                                      │
│   149 │   KERNEL32.GetConsoleMode(windows_filehandle, ctypes.byref(mode))                                              │
│   150 │   return mode.value                                                                                            │
│                                                                                                                        │
│ ╭────────────────────────── locals ───────────────────────────╮                                                        │
│ │ file = <textual.app._NullFile object at 0x000001BEE87E1D90> │                                                        │
│ ╰─────────────────────────────────────────────────────────────╯                                                        │
╰────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
AttributeError: '_NullFile' object has no attribute 'fileno'

Which is happening because we're still inside this block when suspend() is running: https://github.com/JoshKarpel/textual/blob/ee088d2d921890b382094e117ae9a2ed116bca47/src/textual/app.py#L1527-L1530

I believe this can be resolved by using

    terminal_in = sys.__stdin__
    terminal_out = sys.__stdout__

in win32.enable_application_mode() - those always point to the original stdin and stdout (only stdout is needed here, but I did stdin too for symmetry, though maybe there are other places that are relying on stdin redirection?).

Second issue: no readline on non-Unix.

Third issue: running quit() in the embedded terminal on Windows kills the Textual app entirely (no error, it just exits), while on Unix it puts you back in the app. I suspect this is because it turns out) that quit() in the REPL closes stdin. Crtl-Z enter sends EOF (like Ctrl-D on Unix) and works as expected. Though I like this example it might be too thorny to have consistent and unsurprising behavior; I will think a bit on a good replacement that is hopefully still interactive.

@JoshKarpel
Copy link
Contributor Author

Alright, take two, now with better cross-platform support! I realized I could do subprocess.run(sys.executable) to get consistent behavior across platforms by decoupling how the REPL closes from the Textual app that's running it. I've also got a more meaningful test there to keep things working.

@@ -132,3 +132,23 @@ Textual supports the following builtin actions which are defined on the app.
- [action_switch_screen][textual.app.App.action_switch_screen]
- [action_screenshot][textual.app.App.action_screenshot]
- [action_toggle_dark][textual.app.App.action_toggle_dark]

## Temporarily suspending the application
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think "App Basics" might be a better place for this. It's only tangentially related to actions.

# print("hi")
#
# The print will sometimes not go to sys.__stdout__ as expected.
time.sleep(0.025)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This may be masking a race condition. I wonder if we need to do a flush after stopping application mode so no escape sequences arrive after the yield.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦🏻‍♂️ of course! I will experiment.

Copy link
Contributor Author

@JoshKarpel JoshKarpel Jan 18, 2023

Choose a reason for hiding this comment

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

Initial results: self.console.file.flush() before yield does not fix the problem. Given that it's random whether the prints appear or not, the sleep is definitely masking a race condition, but I'm not sure where it is.

But here's where it gets extra weird: I tried adding some debug code to _NullFile.write to see if the writes were still going there, but it seems like they aren't, which makes it seem like they're just completely vanishing, which doesn't make much sense. The investigation continues...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious! Do you think "hi!" is not being written at all?

We have a thread that writes to stdout, so that we can prepare the next frame while the previous frame is being printed. I wonder if that thread is still writing in the background for a fraction of a second.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aha! That's promising - I didn't realize that self.console.file was that thread (should have printed it!)

textual/src/textual/app.py

Lines 284 to 299 in 0be1d96

if sys.__stdout__ is None:
file = _NullFile()
else:
self._writer_thread = _WriterThread()
self._writer_thread.start()
file = self._writer_thread
self.console = Console(
file=file,
markup=False,
highlight=False,
emoji=False,
legacy_windows=False,
_environ=environ,
)
self.error_console = Console(markup=False, stderr=True)

And _WriterThread.flush is a no-op!

textual/src/textual/app.py

Lines 190 to 192 in 0be1d96

def flush(self) -> None:
"""Flush the file (a no-op, because flush is done in the thread)."""
return

I bet the problem lies in there - I'll try to find some time to investigate today.

Copy link
Contributor Author

@JoshKarpel JoshKarpel Jan 19, 2023

Choose a reason for hiding this comment

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

I think that it must have been that! (Maybe printing before the escape code to stop the alternate screen was printed?)

Here's my test case:

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


class SuspendApp(App):
    BINDINGS = [
        ("r", "open_repl", "Open REPL"),
    ]

    def compose(self) -> ComposeResult:
        yield Static(
            "Press r to open the Python built-in REPL. Run quit() to return to the app."
        )
        yield Footer()

    def action_open_repl(self) -> None:
        import time
        for _ in range(10):
            with self.suspend():
                print(f'hi {_}!')
                time.sleep(1)
                print(f'bye {_}!\n')


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

I've got two potential fixes tested: I can either give _WriterThread the ability to temporarily pause by adding some additional threading goop to it (I've pushed this version up to the branch) (inspired by https://stackoverflow.com/questions/33640283/thread-that-i-can-pause-and-resume), or I can join the _WriterThread before yielding and then start a new one afterwards. Something like:

    @contextmanager
    def suspend(self) -> Iterator[None]:
        driver = self._driver
        if driver is not None:
            driver.stop_application_mode()
            
            if self._writer_thread is not None:
                self._writer_thread.stop()

            with redirect_stdout(sys.__stdout__), redirect_stderr(sys.__stderr__):
                yield
                
            if self._writer_thread is not None:
                self._writer_thread = _WriterThread()
                self._writer_thread.start()
                self.console.file = self._writer_thread

            driver.start_application_mode()

I am not sure which would be better. The threading goop seems more maintainable long-term since it's self-contained, but could cause performance problems in that tight loop. Thoughts?

@davep
Copy link
Contributor

davep commented Jan 24, 2023

Currently looking to see if this will play well with #1582 -- right now playing in https://github.com/davep/textual/tree/sleep-resume; nothing concrete yet, more mulling over how it might work.

davep added a commit to davep/textual that referenced this pull request Jan 24, 2023
Having stolen Textualize#1541 from
https://github.com/JoshKarpel this builds on that good work to get killing
the process with SIGTSTP to work.

Initial testing is looking good.

See Textualize#1582.
@davep
Copy link
Contributor

davep commented Jan 24, 2023

Now part of #1655 and happily helping #1582 happen.

@JoshKarpel
Copy link
Contributor Author

Now part of #1655 and happily helping #1582 happen.

Nice! Feel free to close this one and do changes/review in your branch - whatever workflow works for you. I keep getting merge conflicts on the changelog trying to keep up from a different timezone 🤣

@davep
Copy link
Contributor

davep commented Jan 24, 2023

Nice! Feel free to close this one and do changes/review in your branch - whatever workflow works for you.

Will do, thanks. I'll keep this one open for the moment, just until I'm happy that the other is heading in the right direction. I would have added to this one but I utterly failed to work out which magic incantation would let me grab a PR from upstream into the origin of my fork and let me work on that. Got to love git. 😄

I keep getting merge conflicts on the changelog trying to keep up from a different timezone 🤣

Pfft; we're all in the same timezone and that still happens. ¯\(ツ)

davep added a commit to davep/textual that referenced this pull request Jan 22, 2024
Pulling out the very core of Textualize#1541 to start to build it up again and
experiment and test (getting into the forge so I can then pull it down onto
Windows and test there).
davep added a commit to davep/textual that referenced this pull request Jan 23, 2024
Adding Josh Karpel as a co-author here; not because of the docstring, but
the core idea started with Textualize#1541 and this is a reimplementation of that code
in the current version of Textual.

Co-authored-by: Josh Karpel <[email protected]>
@davep
Copy link
Contributor

davep commented Jan 31, 2024

Wee bit later on but... making it over the line as part of #4064.

@davep davep closed this Jan 31, 2024
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.

3 participants