Skip to content

Change to /dev/tty because term.GetState fails#433

Closed
noborus wants to merge 1 commit intogdamore:masterfrom
noborus:fix-screen-init
Closed

Change to /dev/tty because term.GetState fails#433
noborus wants to merge 1 commit intogdamore:masterfrom
noborus:fix-screen-init

Conversation

@noborus
Copy link
Copy Markdown
Contributor

@noborus noborus commented Feb 24, 2021

This is a fix for #432.
The file descriptor of "os.Stdin"
may cause term.GetState to be an error.

The file descriptor of "os.Stdin"
may cause term.GetState to be an error.
@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 24, 2021

Codecov Report

Merging #433 (7c764ab) into master (c94849e) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #433      +/-   ##
==========================================
- Coverage   16.91%   16.89%   -0.03%     
==========================================
  Files          20       20              
  Lines        1383     1385       +2     
==========================================
  Hits          234      234              
- Misses       1135     1137       +2     
  Partials       14       14              
Impacted Files Coverage Δ
tscreen_unix.go 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c94849e...7c764ab. Read the comment docs.

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Mar 5, 2021

I'm probably not going to accept this PR.

I think I prefer to keep the code using stdin the way it is -- its more consistent with other terminal based programs.

I also have a plan to add code to support using file descriptors other than stdin, which might achieve what you want, but that will be later. Definitely I do not plan to reintroduce hard coding /dev/tty.

(I also found /dev/tty on macOS to be kind of a pain as it behaves unlike on other Unix systems.)

@noborus
Copy link
Copy Markdown
Contributor Author

noborus commented Apr 13, 2021

I think that this is not used in windows.
However, there is no idea about a good way to support many OSs.
It is unlikely to be merged, but os.Stdin is wrong and needs to be changed.

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Apr 14, 2021

os.Stdin is not wrong. It's wrong for your use case because you want to do something else with it. But many (most!) applications that run in a terminal use stdin. For example, vim, less, etc.

What I will do is offer an alternative API that will let you initialize a screen with a descriptor of your own choosing. os.Stdin will be the default.

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Apr 18, 2021

Btw, I plan to have a fix for this problem out today. I am sorry I haven't got to it sooner.

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Apr 18, 2021

I am having a rethink about this. I want to do a little testing, but I might just accept it after all. I need to run some tests to convince myself that it doesn't break macOS.

You are correct that this code is not used for Windows.

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Apr 18, 2021

Yes, it works, so I'm going to merge this, but only after I make a minor tweak. (Use os.Open() instead of os.OpenFile, and inline the error check.)

@gdamore
Copy link
Copy Markdown
Owner

gdamore commented Apr 18, 2021

I've merged this another way (after slightly editing the code.)

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.

2 participants