Add a timeout to cursor_pos #750
Conversation
d40e7ba to
64fadc7
Compare
64fadc7 to
b598d0a
Compare
| @@ -412,7 +412,7 @@ def ambiguous_width | |||
| end | |||
|
|
|||
| private def may_req_ambiguous_char_width | |||
| @ambiguous_width = 2 if io_gate.dumb? || !STDIN.tty? || !STDOUT.tty? | |||
| @ambiguous_width = 1 if io_gate.dumb? || !STDIN.tty? || !STDOUT.tty? | |||
| timeout_at = Time.now + timeout | ||
| buf = +'' | ||
| while (wait = timeout_at - Time.now) > 0 && stdin.wait_readable(wait) | ||
| buf << stdin.readpartial(1024) |
There was a problem hiding this comment.
Why did this stdin.readpartial(1024) become necessary?
There was a problem hiding this comment.
When an io is ready for read, normally several bytes are readable. readpartial is more efficient than calling wait_readable and getc many times.
The code buf = match.pre_match + match.post_match assumes post_match exists. It assumes res << input part will append long bytes. I think the original intention of this method is to use readpartial instead of getc.
There was a problem hiding this comment.
Using readpartial is more efficient, I understand now. Thanks!
| end | ||
| end | ||
| Reline::CursorPos.new(column, row) | ||
| [match[:column].to_i - 1, match[:row].to_i - 1] if match |
There was a problem hiding this comment.
IHMO: Instead of returning an array or false, returning an empty array or [0, 0] might be more consistent.
There was a problem hiding this comment.
I think returning retrieved_value or nil (if not found/not available) is not a bad idea.
For example, [1,2,3,4].index 5 returns nil instead of -1 or 0.
User.find_by(id: -1) returns nil instead of User.new(id: nil, name: nil).
Actual retrieving part returns nil, def cursor_pos can return either CursorPos.new(0, 0) or CursorPos.new(-1, -1) or any other value. (I think def cursor_pos can return nil but it requires more changes in other files)
There was a problem hiding this comment.
User.find_by(id: -1) returns nil instead of User.new(id: nil, name: nil).
Indeed, it seems good to return nil. Thank you for your explanation!
| timeout_at = Time.now + timeout | ||
| buf = +'' | ||
| while (wait = timeout_at - Time.now) > 0 && stdin.wait_readable(wait) | ||
| buf << stdin.readpartial(1024) |
There was a problem hiding this comment.
Using readpartial is more efficient, I understand now. Thanks!
| end | ||
| end | ||
| Reline::CursorPos.new(column, row) | ||
| [match[:column].to_i - 1, match[:row].to_i - 1] if match |
There was a problem hiding this comment.
User.find_by(id: -1) returns nil instead of User.new(id: nil, name: nil).
Indeed, it seems good to return nil. Thank you for your explanation!
Add timeout to cursor_pos. Some terminal emulator or test code using pty does not respond to cursor position query
\e[6n. Delay caused by timeout is better than unresponsive.Fixes #674 and ruby/irb#296
Remove
Reline::IO::ANSI#cursor_posfallback toSTDOUT.pread. Not available in some environment. We don't need inaccurate value.Change fallback value of cursor_pos to
[0, 0]. We don't need to return[dummy_row=1, default_ambiguous_width=1]Always fallback east asian ambiguous width to 1.
Note that GNU Readline and zsh also uses ambiguous_width=1. The fallback value was inconsistent, some times 1 and sometimes 2.