-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
cli.console: implement status messages #6497
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
Conversation
05c28f8 to
69d2810
Compare
372b803 to
d6c0c88
Compare
- Add `ConsoleOutput.msg_status()` method for writing status messages to the console output, if supported by the output stream - Implement line-buffered `ConsoleOutputStream` which handles all writes to the console output and which doesn't support status messages - Implement `ConsoleOutputStreamANSI` with support for status messages via ANSI control sequences - Add feature detection to the `ConsoleOutputStream` constructor, so the right class is instantiated automatically Next: Add support for the "classic Windows Console" APIs
Add support for "classic Windows Console" APIs that allow the implementation of status messages without ANSI control sequences.
- Pass the `ConsoleOutput` instance to `Progress`, as well as the output file path, and pass an optional `Progress` instance to `StreamRunner` instead of instantiating it there - Make `Progress` call the newly added `ConsoleOutput.msg_status()` method for each progress status update message instead of writing messages to `stderr` and adding whitespace padding to each line NEXT: If `--progress=force` is set, make `Progress` write non-status messages to the console output in a slower interval
Restore the `--progress=force` functionality by making it write non-status messages to the console output if the console output doesn't support status messages (not a TTY stream). In this case, increase the update interval from 0.25 seconds to 2 seconds, so the regular console output doesn't get spammed by the download progress.
d6c0c88 to
fdef070
Compare
|
This should be finished now. The other PR which this one here was based on has been merged and I've rebased onto master. I'm going to have a couple more checks, then I'm going to merge this one as well. Few more notes:
Examplesmaster - stdout logs, stderr progress with whitespace paddingPR - sticky download progress status line at the bottomPR - no status line with download progress if not a TTY(SIGINT on cat causes SIGPIPE on streamlink, which is handled correctly, hence no "stream ended" logs, etc) PR - force download progress on non-TTY stream |
Windows Terminal, BASH from git-on-Windows (MinGW64 env)Uses ANSI control sequences Windows Terminal, PowerShell 7Uses ANSI control sequences PowerShell 5Uses classic Windows Console APIs CMDUses classic Windows Console APIs BASH on MinTTY (MinGW64 / MSYS2)Uses classic Windows Console APIs and requires the |
|
I'm not super clear on exactly what changed here and how but I can tell you that all my scripts that invoke streamlink no longer show any status or progress at all after upgrading to 7.3.0. |
https://github.com/streamlink/streamlink/blob/7.3.0/CHANGELOG.md?plain=1#L6
|
I guess I'm asking why the default is now not to show progress/status...I don't even know where or how this change is supposed to be displaying progress/status; my terminal programs and environments are extremely commonplace (GNOME Terminal in Ubuntu!) Almost every change I've tracked and occasionally contributed to with this project I've understood except this one, while it's trivial for me to add progress=force to my config file it just seems like an odd thing to add. |
|
Post the actual issue/error and how you're running Streamlink instead of trying to describe what's wrong. The progress output is working perfectly fine, even in Gnome Terminal on Ubuntu.
It is not. The default value is still As you can read in the changelog and the description of this merged PR you're commenting on, the progress output has been moved from hardcoded So if you're running Streamlink with the The motivation for these changes have been clearly pointed out in the linked issue this PR has closed, namely #6449. |
|
OK, I've figured out what's going on here and it's that the behavior of Thanks for all your efforts here. |
Original PR #6497 by bastimeyer Original: streamlink/streamlink#6497
Merged from original PR #6497 Original: streamlink/streamlink#6497
Resolves #6449
These are unfinished changes... The implementation works and should mostly be fine, apart from stuff that may come up while finishing and cleaning this up. Most tests however still need to be done (the CI status checks will therefore currently fail, tests and typing stuff). I still decided to open this PR, because this helps me finding potential issues.The PR branch is currently based on #6496, because this branch hasn't been merged yet. Once merged, I'm going to rebase here.ConsoleOutput.msg_status()ConsoleOutputStream(StreamWrapper)without status message supportConsoleOutputStreamANSIwith status message support via ANSI control sequencesWindowsConsoleandConsoleOutputStreamWindowsProcessandStreamRunnerProgresscall the newly addedConsoleOutput.msg_status()instead of writing tostderrand adding whitespace padding to each messageProgressinstance outside ofStreamRunner--progress=forcewrite regular console output messages in a slower interval when not a TTY. This restores and improves the old behavior.This assumes that ANSI control sequence processing is supported by each terminal application on non-Windows unless the env vars
TERM=dumborTERM=unknownare set. That feature assumption should be fine though.The docs of the Windows Console API calls are all linked in the implementation.
One issue I noticed while testing this in my Windows 10 VM was while using the mintty terminal from git-for-windows (MinGW / MSYS2). Apparently,
isatty()calls always return false because the stdio streams are wrapped. This means that status messages won't work here. The workaround for that is wrapping the command inwinpty(winpty streamlink ...). This is a known issue in this env and git even implements its own workarounds in its code, according to my research. This isn't feasible for Streamlink though and should be done by CPython anyway, so I don't care.As commented in the last commit,
--progress=forcenow doesn't make any sense anymore, as the support for status messages needs to be checked at runtime and can't be forced. Previously we simply wrote status messages tostderrand added whitespace padding, so forcing status messages on non-interactive stdio streams made sense.--progress=forcenow could be changed so that it'll makeProgresswrite non-status-messages to the non-interactive console output, but in a slower interval, so that it doesn't spam that many messages.