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.

Sign up for GitHub

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

Switch to StdioInputMethod when TERM is 'dumb' #907

Merged
merged 3 commits into from
May 1, 2024
Merged

Switch to StdioInputMethod when TERM is 'dumb' #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f04a184
Switch to StdioInputMethod when TERM is 'dumb'
dgutov Mar 22, 2024
1edfbf1
Add special env for testing
dgutov Mar 22, 2024
b0a9239
Merge branch 'master' into no_multiline_or_singleline_on_dumb_term
tompng Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/irb.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,7 +311,9 @@
# ### Input Method
#
# The IRB input method determines how command input is to be read; by default,
# the input method for a session is IRB::RelineInputMethod.
# the input method for a session is IRB::RelineInputMethod. Unless the
# value of the TERM environment variable is 'dumb', in which case the
# most simplistic input method is used.
#
# You can set the input method by:
#
Expand All @@ -329,7 +331,8 @@
# IRB::ReadlineInputMethod.
# * `--nosingleline` or `--multiline` sets the input method to
# IRB::RelineInputMethod.
#
# * `--nosingleline` together with `--nomultiline` sets the
# input to IRB::StdioInputMethod.
#
#
# Method `conf.use_multiline?` and its synonym `conf.use_reline` return:
Expand Down
9 changes: 7 additions & 2 deletions lib/irb/context.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def initialize(irb, workspace = nil, input_method = nil)
@io = nil
case use_multiline?
when nil
if STDIN.tty? && IRB.conf[:PROMPT_MODE] != :INF_RUBY && !use_singleline?
if term_interactive? && IRB.conf[:PROMPT_MODE] != :INF_RUBY && !use_singleline?
# Both of multiline mode and singleline mode aren't specified.
@io = RelineInputMethod.new(build_completor)
else
Expand All @@ -99,7 +99,7 @@ def initialize(irb, workspace = nil, input_method = nil)
unless @io
case use_singleline?
when nil
if (defined?(ReadlineInputMethod) && STDIN.tty? &&
if (defined?(ReadlineInputMethod) && term_interactive? &&
IRB.conf[:PROMPT_MODE] != :INF_RUBY)
@io = ReadlineInputMethod.new
else
Expand Down Expand Up @@ -151,6 +151,11 @@ def initialize(irb, workspace = nil, input_method = nil)
@command_aliases = @user_aliases.merge(KEYWORD_ALIASES)
end

private def term_interactive?
return true if ENV['TEST_IRB_FORCE_INTERACTIVE']
STDIN.tty? && ENV['TERM'] != 'dumb'
Copy link
Member

Choose a reason for hiding this comment

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

How about STDIN.tty? && STDOUT.tty?? Does this solves your problem?

When STDIN and STDOUT are both tty, it is executed in an interactive terminal.
Reline and IRB both have a tty check, but there is an inconsistency. IRB only checks STDIN.tty?, and on the other hand, Reline only checks $stdout.tty?. If $stdout is not a tty or TERM is dumb, Reline enters unusable test mode.

ENV['TERM'] == 'dumb' is used for testing IRB with Reline. Although using ENV['TERM'] like this is a bit weird, changing the TERM=dumb behavior right now will break the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, both STDIN.tty? and STDOUT.tty? return true when the shell is running inside Emacs. But apparently some of the terminal features it's using don't work as expected in that environment. So treating "dumb" terminals differently makes sense to me (that's the definition - terminal with limited capabilities).

Do you know any other real-world "dumb" terminals? It could be useful to test IRB in one of them as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The tests are indeed currently broken by this change, any recommendations for finding a fix would be welcome.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks for the explanation. I agree IRB should treat dumb terminals differently 👍

For test, could you add a way to test IRB by forcing term_interactive? to be true?
Let it return true if ENV["TEST_IRB_FORCE_INTERACTIVE"] is set, and in IRB's test which uses PTY.spawn, specify both TEST_IRB_FORCE_INTERACTIVE and TERM=dumb env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, added.

end

# because all input will eventually be evaluated as Ruby code,
# command names that conflict with Ruby keywords need special workaround
# we can remove them once we implemented a better command system for IRB
Expand Down
4 changes: 3 additions & 1 deletion test/irb/helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,9 @@ def run_ruby_file(&block)
@envs["XDG_CONFIG_HOME"] ||= tmp_dir
@envs["IRBRC"] = nil unless @envs.key?("IRBRC")

PTY.spawn(@envs.merge("TERM" => "dumb"), *cmd) do |read, write, pid|
envs_for_spawn = @envs.merge('TERM' => 'dumb', 'TEST_IRB_FORCE_INTERACTIVE' => 'true')

PTY.spawn(envs_for_spawn, *cmd) do |read, write, pid|
Timeout.timeout(TIMEOUT_SEC) do
while line = safe_gets(read)
lines << line
Expand Down
Loading