Created attachment 216900 [details] old.diff The current manpage of script(1) says the following. "Certain interactive commands, such as vi(1), create garbage in the typescript file. The script utility works best with commands that do not manipulate the screen. The results are meant to emulate a hardcopy terminal, not an addressable one." script(1) is currently able to record [-r] curses sessions perfectly; only the playback functionality [-p] has the above restriction; this patch removes this restriction by appropriately setting terminal attributes.
Additional notes: . I used https://github.com/iiSeymour/game-of-life to reproduce the problem; authored by GitHub user iiSeymour, this, in the words of the author, is "a python implementation of Conway's Game of Life using the curses module with a small evolutionary twist"; it was written for Linux; therefore, one must call "python gol.py" on FreeBSD to bypass the "#!". . This is a duplicate of https://github.com/freebsd/freebsd/pull/433, which was also created by me. This was done to maximize the chances of getting noticed. If this patch on Bugzilla is accepted, I will close the GitHub pull request, and vice versa. . I submitted a similar patch for "scriptreplay(1)" from util-linux; it has now been merged by the maintainer of the project.
*** Bug 248372 has been marked as a duplicate of this bug. ***
Next time it is fine to just update the patch on the existing bug, FYI.
(In reply to Conrad Meyer from comment #3) Got it! Sorry about the mess. Bugzilla n00b here. I wanted to add author info to the patch and restructure the comments, but could not find a way to edit the comments. Thank you for the suggestion.
Note: To reproduce the problem, please run the script(1) session in a terminal that is not in raw mode. Thank you.
Conceptually more accurate version of previous note: To reproduce the problem, please run the script(1) session in a terminal that does not have post processing turned off. NetBSD has accepted the corresponding patch. They modified it slightly: they call atexit(termreset) instead of the several termreset() calls that are in this patch. While not necessary, upon request, I can edit this patch to reflect that change.
Created attachment 216951 [details] script.diff This patch removes a lot of redundant code that was there in the old one.
Created attachment 216973 [details] script.diff Removes unnecessary "ttyflg = 0" at the beginning of playback() in previous patch because static global variable ttyflg will automatically be initialized to 0 by compiler.
Created attachment 217063 [details] 1.diff Adds elegant error handling. For example, instead of using isatty followed by tcgetattr, tcgetattr is directly used. This is logical because freebsd/lib/libc/gen/isatty.c suggests that isatty is actually implemented using tcgetattr. Upon failure of tcgetattr, errno is checked to decide if stdout is a tty. Note: I am comfortable with Bugzilla now. Therefore, I am closing the GitHub pull request.
I think this is fine. I tested it with a few curses-based games and it fixed some issues with characters appearing in the wrong place during playback. Some curses programs appear to play back properly with or without the change.
A commit references this bug: Author: markj Date: Fri Aug 7 18:48:57 UTC 2020 New revision: 364039 URL: https://svnweb.freebsd.org/changeset/base/364039 Log: script: Put the terminal in raw mode when playing back a session. Otherwise recorded sessions of some interactive programs do not play back properly. PR: 248377 Submitted by: Soumendra Ganguly <0.gangzta@gmail.com> MFC after: 1 week Changes: head/usr.bin/script/script.c
(In reply to Mark Johnston from comment #10) Thank you! I am glad I could contribute to FreeBSD. I love it. You are correct; some programs do fine without it.
(In reply to SOUMENDRA GANGULY from comment #12) No problem, thanks for taking the time to submit a patch.
(In reply to Mark Johnston from comment #13) Thank you. Can we please keep this in "in progress" status for a little while more? I am still working on the program.
Created attachment 217091 [details] 2.diff As pointed out earlier, isatty is unnecessary. It simply checks for tcgetattr failure. This adds more elegant error handling to certain parts based on errno set by tcgetattr. Also, upon more testing, I found out that errno == EBADF is very unusual for tcgetattr in this case: script(1) of course does not close its own stdout voluntarily mid execution. gdb was able to force an EBADF [ p close(1) ]. Therefore, the EBADF error handling is not completely removed by this patch; it is only changed to errno != ENOTTY. Thank you.
(In reply to commit-hook from comment #11) Also, I am not not sure if this is allowed/possible, but if it is, then could you please change/set my email to <soumendraganguly@gmail.com> in the "Submitted by:" field? At least for 2.diff? Name of course stays the same.
(In reply to SOUMENDRA GANGULY from comment #16) Sorry, it's not possible to edit svn commit logs after the fact. I just used the email associated with your bugzilla account. I'll try to use the other email for future commits, but I'd suggest making another account to ensure that others don't do what I did.
(In reply to SOUMENDRA GANGULY from comment #15) Are you happy with this version of the patch?
(In reply to Mark Johnston from comment #18) Yes, I am happy with this. Thank you. Note: 1.diff is the patch that you applied before. 2.diff changes other things. Thank you.
A commit references this bug: Author: markj Date: Tue Aug 11 14:19:06 UTC 2020 New revision: 364112 URL: https://svnweb.freebsd.org/changeset/base/364112 Log: script: Minor cleanups. - Instead of using isatty() to decide whether to call tcgetattr(), just call tcgetattr() directly, since that's all that isatty() does anyway. - Simplify error handling in termset(). Check for errno != ENOTTY from tcgetattr() to handle errors that may be raised while running script(1) under a debugger. PR: 248377 Submitted by: Soumendra Ganguly <soumendraganguly@gmail.com> MFC after: 1 week Changes: head/usr.bin/script/script.c
(In reply to commit-hook from comment #20) Thank you, sir.
A commit references this bug: Author: markj Date: Fri Aug 14 00:55:23 UTC 2020 New revision: 364224 URL: https://svnweb.freebsd.org/changeset/base/364224 Log: MFC r364039: script: Put the terminal in raw mode when playing back a session. PR: 248377 Changes: _U stable/12/ stable/12/usr.bin/script/script.c
A commit references this bug: Author: markj Date: Fri Aug 14 00:55:49 UTC 2020 New revision: 364225 URL: https://svnweb.freebsd.org/changeset/base/364225 Log: MFC r364112: script: Minor cleanups. PR: 248377 Changes: _U stable/12/ stable/12/usr.bin/script/script.c
Thanks for the patches.