Skip to content

Windows fix #775

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

Merged
tompng merged 12 commits into from
Nov 8, 2024
Merged

Windows fix #775

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3e1cfa2
test_yamatanooroti: close tempfile before unlink
YO4 Oct 16, 2024
8050239
test_yamatanooroti: omit because of windows does not support job control
YO4 Oct 16, 2024
235367d
test_yamatanooroti: change startup message detection for windows
YO4 Oct 12, 2024
93a23bc
windows.rb: can call win32api using nil as NULL for pointer argument
YO4 Oct 16, 2024
3875659
windows.rb: fix get_screen_size
YO4 Oct 16, 2024
9b3ffd8
windows.rb: import scroll_down() from ansi.rb
YO4 Oct 16, 2024
053bbdb
windows.rb: add auto linewrap control if VT output not supported (leg…
YO4 Oct 19, 2024
2429165
unfreeze WIN32API pointer arguments
YO4 Oct 31, 2024
060ba14
remove useless code from Win32API#call
YO4 Nov 5, 2024
df70541
Correctly handle top of console viewport
YO4 Nov 5, 2024
eb5959c
Revert "remove useless code from Win32API#call"
YO4 Nov 6, 2024
5443ab5
Revert "windows.rb: can call win32api using nil as NULL for pointer a…
YO4 Nov 6, 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
65 changes: 36 additions & 29 deletions lib/reline/io/windows.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ def initialize(dllname, func, import, export = "0", calltype = :stdcall)
def call(*args)
import = @proto.split("")
args.each_with_index do |x, i|
args[i], = [x == 0 ? nil : +x].pack("p").unpack(POINTER_TYPE) if import[i] == "S"
args[i], = [x == 0 ? nil : x&.+@].pack("p").unpack(POINTER_TYPE) if import[i] == "S"
Copy link
Member

@tompng tompng Nov 4, 2024

Choose a reason for hiding this comment

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

This code does not expect x to be non-zero number because [+3].pack('p') raises error.
Zero represents NULL_POINTER, so the possible code would be

# Use 0 as null_pointer
[x == 0 ? nil : +x].pack("p")
@ScrollConsoleScreenBuffer.call(rect, 0, dst, fill)
# Represent NULL_POINTER by nil, not by 0
[x&.+@].pack("p")
@ScrollConsoleScreenBuffer.call(rect, nil, dst, fill)

By the way, I think if x is a frozen string, a new mutable string +x might be garbaged collected before @func.call is called.
If the frozen string issue is solved in L174 (mode = +"\0\0\0\0"), I think removing +@ is better.

[x == 0 ? nil : x].pack("p").... # x is 0 or string
# or
[x].pack("p").... # x is nil or string

Copy link
Contributor Author

@YO4 YO4 Nov 5, 2024

Choose a reason for hiding this comment

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

The current patch is the result of a minimization of fixes because I could not figure out the intent of args repacking.
By checking the code around ruby 1.9 to 2.0, I found that DL::CFunc of Ruby/DL (Ruby/DL2), the backend of WIN32API at that time, accepts only integers.
I think the repacking itself is unnecessary because the current Fiddle allows more flexible passing of arguments.

def call(*args)
  return @func.call(*args)
end

The above may be enough. Is there something I'm missing?

Copy link
Member

@tompng tompng Nov 5, 2024

Choose a reason for hiding this comment

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

I think the NULL problem is fixed in 9b3ffd8
String 0.chr * n +"\0\0\0..." seems to be always passed to import[i]=="S" (import is 'P') argument except
call_with_console_handle(@ScrollConsoleScreenBuffer, scroll_rectangle, nil, destination_origin, fill)
and this line is removed in 9b3ffd8

Copy link
Contributor Author

@YO4 YO4 Nov 5, 2024

Choose a reason for hiding this comment

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

Thanks for the suggestion. I was not aware of 9b3ffd8 at all.
However, I am worried about touching a code that I don't know the basis for, and I now think that code is unnecessary.
So I think it was done in 060ba14 .

Copy link
Member

@tompng tompng Nov 6, 2024

Choose a reason for hiding this comment

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

def call(*args)
  return @func.call(*args)
end

This looks good to me.
However, I don't have confidence that this works in all supported ruby versions and fiddle version. I think it is better to avoid risk since next ruby release is close in time.
How about leaving it as is (as original [x == 0 ? nil : +x].pack("p")....) for now, and change it after adding windows ci?

Copy link
Contributor Author

@YO4 YO4 Nov 7, 2024

Choose a reason for hiding this comment

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

I understand your point of consideration.
I also know that there is no specific problem that the patch will solve.
reverted in eb5959c 5443ab5

args[i], = [x].pack("I").unpack("i") if import[i] == "I"
end
ret, = @func.call(*args)
Expand Down Expand Up @@ -157,6 +157,7 @@ def call(*args)
STD_OUTPUT_HANDLE = -11
FILE_TYPE_PIPE = 0x0003
FILE_NAME_INFO = 2
ENABLE_WRAP_AT_EOL_OUTPUT = 2
ENABLE_VIRTUAL_TERMINAL_PROCESSING = 4

# Calling Win32API with console handle is reported to fail after executing some external command.
Expand All @@ -170,7 +171,7 @@ def call(*args)
end

private def getconsolemode
mode = "\000\000\000\000"
mode = +"\0\0\0\0"
call_with_console_handle(@GetConsoleMode, mode)
mode.unpack1('L')
end
Expand Down Expand Up @@ -350,9 +351,12 @@ def get_console_screen_buffer_info

def get_screen_size
unless csbi = get_console_screen_buffer_info
return [1, 1]
return [24, 80]
end
csbi[0, 4].unpack('SS').reverse
top = csbi[12, 2].unpack1('S')
bottom = csbi[16, 2].unpack1('S')
width = csbi[0, 2].unpack1('S')
[bottom - top + 1, width]
Copy link
Member

@tompng tompng Nov 4, 2024

Choose a reason for hiding this comment

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

My understanding by reading the document is:

hidden_screen_buffer01
hidden_screen_buffer02
hidden_screen_buffer03
hidden_screen_buffer04
visible_screen_buffer1
visible_screen_buffer2
visible_sc█een_buffer3
visible_screen_buffer4

screen_buffer width = 22, screen_buffer height = 8
cursor x = 10 y = 6 (cursor coord in screen buffer, not coord in visible window)
window left=0 right=21 top=4 bottom=7

Is this right? If so, I think def cursor_pos should use CursorPos.new(x, y - top). Can you check this?

Copy link
Contributor Author

@YO4 YO4 Nov 5, 2024

Choose a reason for hiding this comment

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

You are right.
I tried to modify cursor_pos, but even the cursor moving functions needed to take csbi.srWindow.Top into consideration. Needs some more work.

Copy link
Contributor Author

@YO4 YO4 Nov 5, 2024

Choose a reason for hiding this comment

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

done in df70541 .

Copy link
Member

@tompng tompng Nov 6, 2024

Choose a reason for hiding this comment

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

Thank you 👍

end

def cursor_pos
Expand Down Expand Up @@ -399,31 +403,12 @@ def erase_after_cursor
call_with_console_handle(@FillConsoleOutputAttribute, attributes, get_screen_size.last - cursor_pos.x, cursor, written)
end

def scroll_down(val)
return if val < 0
return unless csbi = get_console_screen_buffer_info
buffer_width, buffer_lines, x, y, attributes, window_left, window_top, window_bottom = csbi.unpack('ssssSssx2s')
screen_height = window_bottom - window_top + 1
val = screen_height if val > screen_height

if @legacy_console || window_left != 0
# unless ENABLE_VIRTUAL_TERMINAL,
# if srWindow.Left != 0 then it's conhost.exe hosted console
# and puts "\n" causes horizontal scroll. its glitch.
# FYI irb write from culumn 1, so this gives no gain.
scroll_rectangle = [0, val, buffer_width, buffer_lines - val].pack('s4')
destination_origin = 0 # y * 65536 + x
fill = [' '.ord, attributes].pack('SS')
call_with_console_handle(@ScrollConsoleScreenBuffer, scroll_rectangle, nil, destination_origin, fill)
else
origin_x = x + 1
origin_y = y - window_top + 1
@output.write [
(origin_y != screen_height) ? "\e[#{screen_height};H" : nil,
"\n" * val,
(origin_y != screen_height or !x.zero?) ? "\e[#{origin_y};#{origin_x}H" : nil
].join
end
# This only works when the cursor is at the bottom of the scroll range
# For more details, see https://github.com/ruby/reline/pull/577#issuecomment-1646679623
def scroll_down(x)
return if x.zero?
# We use `\n` instead of CSI + S because CSI + S would cause https://github.com/ruby/reline/issues/576
@output.write "\n" * x
end

def clear_screen
Expand Down Expand Up @@ -472,6 +457,28 @@ def deprep(otio)
# do nothing
end

def disable_auto_linewrap(setting = true, &block)
mode = getconsolemode
if 0 == (mode & ENABLE_VIRTUAL_TERMINAL_PROCESSING)
if block
begin
setconsolemode(mode & ~ENABLE_WRAP_AT_EOL_OUTPUT)
block.call
ensure
setconsolemode(mode | ENABLE_WRAP_AT_EOL_OUTPUT)
end
else
if setting
setconsolemode(mode & ~ENABLE_WRAP_AT_EOL_OUTPUT)
else
setconsolemode(mode | ENABLE_WRAP_AT_EOL_OUTPUT)
end
end
else
block.call if block
end
end

class KeyEventRecord

attr_reader :virtual_key_code, :char_code, :control_key_state, :control_keys
Expand Down
6 changes: 6 additions & 0 deletions lib/reline/line_editor.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,11 @@ def render_finished
end

def print_nomultiline_prompt
Reline::IOGate.disable_auto_linewrap(true) if Reline::IOGate.win?
# Readline's test `TestRelineAsReadline#test_readline` requires first output to be prompt, not cursor reset escape sequence.
@output.write Reline::Unicode.strip_non_printing_start_end(@prompt) if @prompt && !@is_multiline
ensure
Reline::IOGate.disable_auto_linewrap(false) if Reline::IOGate.win?
end

def render
Expand Down Expand Up @@ -509,6 +512,7 @@ def render
# by calculating the difference from the previous render.

private def render_differential(new_lines, new_cursor_x, new_cursor_y)
Reline::IOGate.disable_auto_linewrap(true) if Reline::IOGate.win?
Copy link
Member

@tompng tompng Nov 4, 2024

Choose a reason for hiding this comment

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

Why is this needed? I think line_editor does not print line longer than terminal width.
(Maybe the case line_width == terminal_width case?)

If this is needed, moving it to Reline::Windows#prep and Reline::Windows#deprep is better

Copy link
Contributor Author

@YO4 YO4 Nov 5, 2024

Choose a reason for hiding this comment

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

The restore in deprep does not work because render_finished outputs a single, long line.
Windows console has mode flags. (https://learn.microsoft.com/en-us/windows/console/high-level-console-modes)
If ENABLE_VIRTUAL_TERMINAL_PROCESSING is set, cursor moves as expected by reline.
In environments where the flag is off or not supported, ENABLE_WRAP_AT_EOL_OUTPUT is on normally, causing a forced line break at the end of a line of output.
For example, outputting 123456 on a 6-column wide terminal would result in the following.

123456
_ #=> cursor moves next line

If ENABLE_WRAP_AT_EOL_OUTPUT is turned off, no forced line breaks occur. Instead, the cursor stays at the end of the line and subsequent characters overwrite the end of the line.
For example, outputting 12345678 on a 6-column wide terminal would result in the following.

123458 #=> cursor stays last column of line

Therefore, it was necessary to insert control of ENABLE_WRAP_AT_EOL_OUTPUT in the appropriate place.
Another reason was that if there was console output in the background, long lines could be destroyed if the flag was not restored while waiting for keystrokes.

Copy link
Member

@tompng tompng Nov 5, 2024

Choose a reason for hiding this comment

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

It makes sense. Thank you for the description 👍

rendered_lines = @rendered_screen.lines
cursor_y = @rendered_screen.cursor_y
if new_lines != rendered_lines
Expand Down Expand Up @@ -539,6 +543,8 @@ def render
Reline::IOGate.move_cursor_column new_cursor_x
Reline::IOGate.move_cursor_down new_cursor_y - cursor_y
@rendered_screen.cursor_y = new_cursor_y
ensure
Reline::IOGate.disable_auto_linewrap(false) if Reline::IOGate.win?
end

private def clear_rendered_screen_cache
Expand Down
6 changes: 4 additions & 2 deletions test/reline/yamatanooroti/test_rendering.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ def iterate_over_face_configs(&block)
config_file = Tempfile.create(%w{face_config- .rb})
config_file.write face_config
block.call(config_name, config_file)
config_file.close
ensure
config_file.close
File.delete(config_file)
end
end
Expand Down Expand Up @@ -1065,7 +1065,7 @@ def test_dialog_scroll_pushup_condition

def test_simple_dialog_with_scroll_screen
iterate_over_face_configs do |config_name, config_file|
start_terminal(5, 50, %W{ruby -I#{@pwd}/lib -r#{config_file.path} #{@pwd}/test/reline/yamatanooroti/multiline_repl --dialog simple}, startup_message: 'Multiline REPL.')
start_terminal(5, 50, %W{ruby -I#{@pwd}/lib -r#{config_file.path} #{@pwd}/test/reline/yamatanooroti/multiline_repl --dialog simple}, startup_message: /prompt>/)
write("if 1\n 2\n 3\n 4\n 5\n 6")
write("\C-p\C-n\C-p\C-p\C-p#")
close
Expand Down Expand Up @@ -1796,6 +1796,7 @@ def test_thread_safe
end

def test_stop_continue
omit if Reline.core.io_gate.win?
pidfile = Tempfile.create('pidfile')
rubyfile = Tempfile.create('rubyfile')
rubyfile.write <<~RUBY
Expand All @@ -1816,6 +1817,7 @@ def test_stop_continue
close
ensure
File.delete(rubyfile.path) if rubyfile
pidfile.close if pidfile
File.delete(pidfile.path) if pidfile
end

Expand Down