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.

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

windows.rb cannot reproduce the way vterm.rb receives startup_message #26

Open
YO4 opened this issue Oct 10, 2024 · 3 comments
Open

windows.rb cannot reproduce the way vterm.rb receives startup_message #26

YO4 opened this issue Oct 10, 2024 · 3 comments

Comments

@YO4
Copy link

YO4 commented Oct 10, 2024

vterm.rb recieves startup message as stream, but windows.rb can not do same way.
This difference can be critical when the window size is short.
For example,

 +-------------------------------------------- scrollback area
 |Multiline REPL.
 +-------------------------------------------- visible area
 |prompt>
 |        Ruby is...
 |        A dynamic, open source programming
 |        language with a focus on simplicity
 |        and productivity. It has an elegant
 +-------------------------------------------- bottom of view

windows.rb can not recieve startup message as stream, can only retrieve_screen().
Use the Command Prompt Window, we can access whole screen buffer, so a solution exists.
On the other hand, when using the Windows Terminal, only the displayed area can be accessed.

In this situation, there is no way to get start_terminal() with startup_message: 'Multiline REPL.' to work.

Another problem exists, but mitigation exists for that one. f0ee26b
I believe that this case cannot be handled by windows-only code solely.

@tompng
Copy link
Member

tompng commented Oct 10, 2024

I think using the screen in both windows and vterm is one of the good options we can choose.

start_terminal(h, w, cmd, startup_message:)
# Make ↑ and ↓ almost the same
start_terminal(h, w, cmd)
assert_screeen(startup_message)

Stream startup message is not easy to use.
Diff in https://github.com/ruby/irb/pull/1001/files

  def test_nomultiline
-   write_irbrc <<~'LINES'
-     puts 'start IRB'
-   LINES
-   start_terminal(25, 80, %W{ruby -I#{@pwd}/lib #{@pwd}/exe/irb --nomultiline}, startup_message: 'start IRB')
+   start_terminal(25, 80, %W{ruby -I#{@pwd}/lib #{@pwd}/exe/irb --nomultiline}, startup_message: /irb\(main\)/)
    ...
  end

puts 'start IRB' does not mean that program is ready for input. Ideally, we want to wait for the prompt to be shown.
However, the stream contains escape sequences like "start IRB\n\e[1G▽\e[6n\e[1G\e[Kprompt>", and before #10, startup message stream was "start IRB\n\e[1G▽\e[6n[stucks forever]".
Maybe IRB's test was using "start IRB" for startup message just because of a restriction of yamatanooroti's implementation.

@YO4
Copy link
Author

YO4 commented Oct 13, 2024

My problem can be solved by referring to that patch.
YO4/reline@ac3f1e8 , in test/reline/yamatanooroti/test_rendering.rb

     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 ...
         ...
      end
    end

@YO4
Copy link
Author

YO4 commented Oct 13, 2024

The current implementation of starup_message has environment-dependent behavior, and passing a string can cause unintended problems.

The function from retryable_screen_assertion_with_proc() with assert_proc.call removed could be used as wait_startup_message(), but the behavior when a string is specified is different from the current startup_message processing. Nevertheless, I think it makes sense to change it for consistency.

The fact that problems are always found in tests and alternatives are provided may not justify the amount of work required to change the specification. It may be enough to deprecate the string specification.

Currently I am neutral on whether to change the behavior.

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

No branches or pull requests

2 participants