Windows fix #775

YO4 · 2024-11-04T13:05:38Z

This is part of supporting windows for reline CI using yamatanooroti.
This PR contains the following changes. There are independent to windows specific yamatanooroti changes.

Windows specific test fix, not related yamatanooroti.
Changes for conhostV1 (Command Prompt Window, legacy mode ON), includes bug fix.
Follows reline changes.
Supporting frozen string literal.

Exception occurred when interrupted with Ctrl+C on legacy conhost

return [window height, buffer width] insted of [buffer height, buffer width]

…acy console)

They internally duplicate arguments so api functions write to another place. This breaks the console mode detection with ruby-head.

YO4 · 2024-11-04T13:22:36Z

#775 #776 does not have CI changes.
Currently, CI is being tested in another branch.
https://github.com/YO4/reline/tree/ci_for_windows

tompng · 2024-11-04T16:44:41Z

lib/reline/io/windows.rb

@@ -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"


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

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?

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

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 .

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?

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

tompng · 2024-11-04T17:08:41Z

lib/reline/line_editor.rb

@@ -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?


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

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.

It makes sense. Thank you for the description 👍

tompng · 2024-11-04T17:27:10Z

lib/reline/io/windows.rb

+    top = csbi[12, 2].unpack1('S')
+    bottom = csbi[16, 2].unpack1('S')
+    width = csbi[0, 2].unpack1('S')
+    [bottom - top + 1, width]


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?

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.

done in df70541 .

Thank you 👍

argument repacking and return value tweaking is not needed for Reline::Windows requirements.

This reverts commit 060ba14.

…rgument" This reverts commit 93a23bc.

tompng

Looks good 👍
Thank you

* test_yamatanooroti: close tempfile before unlink * test_yamatanooroti: omit because of windows does not support job control * test_yamatanooroti: change startup message detection for windows * windows.rb: can call win32api using nil as NULL for pointer argument Exception occurred when interrupted with Ctrl+C on legacy conhost * windows.rb: fix get_screen_size return [window height, buffer width] insted of [buffer height, buffer width] * windows.rb: import scroll_down() from ansi.rb * windows.rb: add auto linewrap control if VT output not supported (legacy console) * unfreeze WIN32API pointer arguments They internally duplicate arguments so api functions write to another place. This breaks the console mode detection with ruby-head. * remove useless code from Win32API#call argument repacking and return value tweaking is not needed for Reline::Windows requirements. * Correctly handle top of console viewport * Revert "remove useless code from Win32API#call" This reverts commit ruby/reline@060ba140ed43. * Revert "windows.rb: can call win32api using nil as NULL for pointer argument" This reverts commit ruby/reline@93a23bc5d0c9. ruby/reline@47c1ffbabe

YO4 · 2024-11-08T14:14:44Z

I'm glad to hear it.
Thank you for your time.

YO4 added 8 commits October 31, 2024 21:46

test_yamatanooroti: close tempfile before unlink

3e1cfa2

test_yamatanooroti: omit because of windows does not support job control

8050239

test_yamatanooroti: change startup message detection for windows

235367d

windows.rb: can call win32api using nil as NULL for pointer argument

93a23bc

Exception occurred when interrupted with Ctrl+C on legacy conhost

windows.rb: fix get_screen_size

3875659

return [window height, buffer width] insted of [buffer height, buffer width]

windows.rb: import scroll_down() from ansi.rb

9b3ffd8

windows.rb: add auto linewrap control if VT output not supported (leg…

053bbdb

…acy console)

unfreeze WIN32API pointer arguments

2429165

They internally duplicate arguments so api functions write to another place. This breaks the console mode detection with ruby-head.

tompng reviewed Nov 4, 2024

View reviewed changes

YO4 added 4 commits November 5, 2024 22:30

remove useless code from Win32API#call

060ba14

argument repacking and return value tweaking is not needed for Reline::Windows requirements.

Correctly handle top of console viewport

df70541

Revert "remove useless code from Win32API#call"

eb5959c

This reverts commit 060ba14.

Revert "windows.rb: can call win32api using nil as NULL for pointer a…

5443ab5

…rgument" This reverts commit 93a23bc.

tompng approved these changes Nov 8, 2024

View reviewed changes

tompng merged commit 47c1ffb into ruby:master Nov 8, 2024
46 checks passed

tompng mentioned this pull request Jan 2, 2025

Disable autocomplete on Windows as default ruby/irb#435

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows fix #775

Windows fix #775

YO4 commented Nov 4, 2024

YO4 commented Nov 4, 2024

tompng Nov 4, 2024

YO4 Nov 5, 2024

tompng Nov 5, 2024

YO4 Nov 5, 2024

tompng Nov 6, 2024

YO4 Nov 7, 2024

tompng Nov 4, 2024

YO4 Nov 5, 2024

tompng Nov 5, 2024

tompng Nov 4, 2024

YO4 Nov 5, 2024

YO4 Nov 5, 2024

tompng Nov 6, 2024

tompng left a comment

YO4 commented Nov 8, 2024

Windows fix #775

Windows fix #775

Conversation

YO4 commented Nov 4, 2024

YO4 commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tompng left a comment

Choose a reason for hiding this comment

YO4 commented Nov 8, 2024