scriptreplay: add interactive playback controls #3015
Conversation
Provision for retrival of playback interactive controls using fgetwc without blocking.
delay_for does not change delay. Hint about that using const. This allows for usage of const delay and improves readability.
Adds pause to replay_setup. Adds functions to toggle and get pause state. Adds stdin keys read - looks for space for pause toggle. Adds replay pause to replay render loop.
|
I intend on changing that, pause currently ignores the current step. |
7e8f5e6 to
6f922a3
Compare
term-utils/scriptreplay.c
Outdated
| { | ||
| static const struct timeval mindelay = { .tv_sec = 0, .tv_usec = 100 }; | ||
| static const struct timeval inputDelay = { .tv_sec = 0, .tv_usec = 100000 }; | ||
| struct timeval stepDelay = { 0, 0 }; |
There was a problem hiding this comment.
Please keep the variable names consistent. We rarely use C++-like names like "fooBar;" it seems better to use "inputdelat" or "input_delay."
term-utils/scriptreplay.c
Outdated
| isterm = setterm(&saved, &saved_flag); | ||
|
|
||
| do { | ||
| switch (fgetwc(stdin)) { |
There was a problem hiding this comment.
There is include/fgetwc_or_err.h with fgetwc_or_err(). I guess it can increase the reliability of the code.
There was a problem hiding this comment.
IMO we should not use fgetwc_or_err as we expect fgetwc to have many instances of WEOF with errno !=0 when no user input is added to the stream.
// section from fgetwc_or_err
if (ret == WEOF && errno != 0)
err(EXIT_FAILURE, _("fgetwc() failed"));We run at non-blocking mode and this results in this error showing up every time no input is found getwc() failed: Resource temporarily unavailable, which is very common and repeats often during a replay.
There was a problem hiding this comment.
Good point, I forgot about the non-blocking mode ...
There was a problem hiding this comment.
BTW, why do we need widechar (fgetwc() and wchat_t) here? I guess it will work with arbitrary function like fgetc().
There was a problem hiding this comment.
At start i hoped it would give me a multi-byte char for the key-press, but i didn't know that they don't count, and so fgetwc only reads one char at a time for this usage.
I fogot to change that after i updated the logic
I will change that to fgetc.
term-utils/scriptreplay.c
Outdated
| break; | ||
|
|
||
| if (replay_get_is_paused(setup)) | ||
| { |
There was a problem hiding this comment.
Please follow the current coding style. It means "if ( ) {" rather than "if ( )\n{".
https://www.kernel.org/doc/html/v4.10/process/coding-style.html#placing-braces-and-spaces
We also do not use "//" for comments :-)
there is no real reason to use fgetwc here as arrow key press is not read as a multi-byte by fgetwc and the logic implemented here also reads the keypress byte by bytes.
|
https://github.com/util-linux/util-linux/actions/runs/9228958044/job/25445611203?pr=3015 |
This commit fix clang compile issue. Replace local char veriables with generic ch already used by the in function. Reduce memory usage.
Linux kernel coding style prefers switch and case at the same indent level.
|
My bad, used gcc at my setup. Also noticed the switch-case coding style is not as recommended by the linux kernel and fixed the indent with d616c2f |
Adds interactive playback controls based of #2999 .
Status:
I did not add all suggested in #2999 , These are what i think the most important.
Adding a backward step is non trivial without rewriting lots of how the playback works, and most terminals allow to scroll back the stream anyway.
Speed for holding the key-bind is also self emerging from the
inputDelay.So i think its best to attempt to merge this basic form so it ships the core features with less complex changes.