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

Display show_cmds's output in a pager when in TTY environment #647

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

st0012
Copy link
Member

@st0012 st0012 commented Jul 16, 2023

This can:

  • Make it easier to scroll up and down the commands list
  • Avoid pushing up users' previous output
  • Allow users to do basic search with /<word>

Demo

show_cmds-in-pager.mp4

Closes #646

@st0012 st0012 added the enhancement New feature or request label Jul 16, 2023
@st0012 st0012 requested a review from a team July 16, 2023 11:31
@st0012 st0012 self-assigned this Jul 16, 2023
@st0012 st0012 force-pushed the implement-#646 branch 4 times, most recently from fa5816a to 025892d Compare July 17, 2023 21:03
@tompng
Copy link
Member

tompng commented Jul 22, 2023

Handling Ctrl-C works great 👍

In my environment, ESC[1m is shown.
less_result
It works if I set ENV['PAGER'] = 'less -r'.

In show_doc String, rdoc is using non-ansi format = S\bSt\btr\bri\bin\bng\bg \b <\b< \b O\bOb\bbj\bje\bec\bct\bt\n\n for string_doc

@@ -1,6 +1,7 @@
# frozen_string_literal: true

require "stringio"
require "rdoc/ri/driver"
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to wrap this with begin rescue like irb is doing in completion?
Or this file is delayed loaded so we don't need to rescue it? (maybe the loading timing will change in the future?)

Copy link
Member

Choose a reason for hiding this comment

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

The pager implementation part of rdic/ri/driver is about 30 lines of 1500 lines.
Maybe we could implement it in IRB? then we can customize the command line option like

# https://github.com/ruby/rdoc/blob/5db4767dda4b4320eeac84aba152ab56ec20b839/lib/rdoc/ri/driver.rb#L1470
pagers = [ENV['RI_PAGER'], ENV['PAGER'], 'pager', 'less', 'more']
 
# IRB's ansi format pager
pagers = [ENV['RI_PAGER'], ENV['PAGER'], 'pager --foobar', 'less --raw-control-chars', 'more --raw-control-chars']

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we need to wrap this with begin rescue like irb is doing in completion?

Since we've declared rdoc as a dependency in #648, rdoc should always be installed along with irb. If that didn't happen for some reason, it's something else we need to fix.

Put it in another way: I think we can remove the rescue block in completion and other places.

@hsbt can you help me confirm that if my assumption above is correct? Or when irb is used as a default gem, rdoc could still be missing? (example)

Maybe we could implement it in IRB? then we can customize the command line option like

Generally I agree with this direction, but I found a few concerns when implementing it:

  • Should IRB::Pager also accept ENV['RI_PAGER']?
    • If it doesn't, users would get inconsistent behaviour where IRB's pager doesn't pickup their config for RI pager
    • If it does, it sounds like a weird coupling with rdoc?
  • Should IRB::Pager set different pager options (-r for example) than rdoc's?
    • This will bring similar concerns the above question

I think if we want to avoid using rdoc's pager, we should do that for all features so we can get rid off of the above issues. For example, IRB should only use rdoc to get documentation content and handle the display itself. But at the same time the effort would grow significantly.

Copy link
Member Author

@st0012 st0012 Jul 22, 2023

Choose a reason for hiding this comment

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

Because it's necessary to pass -r to less in order to correctly display IRB's ANSI escape sequences, I eventually decided to implement IRB::Pager with the support of ENV['RI_PAGER'] for now.

I think we can later replace the use of RDoc pager in show_doc and doc dialog with something like this:

name = "String"
driver = RDoc::RI::Driver.new({})
found, klasses, includes, extends = driver.classes_and_includes_and_extends_for(name)
doc = driver.class_document(name, found, klasses, includes, extends)
formatter = RDoc::Markup::ToAnsi.new
contents = doc.accept(formatter)
IRB::Pager.page do |io|
  io.puts contents
end

@st0012
Copy link
Member Author

st0012 commented Jul 22, 2023

In my environment, ESC[1m is shown.

Can you describe your environment setup? I can't reproduce it on my machine with VS Code Terminal, iTerm2, or Terminal.app

In show_doc String, rdoc is using non-ansi format = S\bSt\btr\bri\bin\bng\bg \b <\b< \b O\bOb\bbj\bje\bec\bct\bt\n\n for

Sorry I don't get this part. Do you mean you also need to set less -r for RDoc so it displays show_doc's output correctly?

@tompng
Copy link
Member

tompng commented Jul 22, 2023

I think I'm using mac pre-installed less

% less --version
less 581.2 (POSIX regular expressions)
Copyright (C) 1984-2021  Mark Nudelman

less comes with NO WARRANTY, to the extent permitted by law.
For information about the terms of redistribution,
see the file named README in the less distribution.
Home page: https://greenwoodsoftware.com/less

and I can reproduce it with

% docker run --rm -ti rubylang/rubyfarm sh
# git clone https://github.com/ruby/irb.git
# cd irb
# git switch implement-#646
# bundle install
# bin/console
irb(main):001:0> show_doc String
=> Works
irb(main):002:0> show_cmds
=> ESC[1mIRBESC[0m

show_doc works and doesn't need -r because show_doc don't use ansi escape sequence like \e[1mBOLD\e[m but uses another format B\bBO\bOL\bLD\bD.

echo "B\bBO\bOL\bLD\bD" | less
echo "\e[1mBOLD\e[m" | less
echo "\e[1mBOLD\e[m" | less -r

@st0012
Copy link
Member Author

st0012 commented Jul 22, 2023

That's weird because I'm using the same less version:

less 581.2 (POSIX regular expressions)
Copyright (C) 1984-2021  Mark Nudelman

less comes with NO WARRANTY, to the extent permitted by law.
For information about the terms of redistribution,
see the file named README in the less distribution.
Home page: https://greenwoodsoftware.com/less

And with it, these 3 examples all rendered correctly:

echo "B\bBO\bOL\bLD\bD" | less
echo "\e[1mBOLD\e[m" | less
echo "\e[1mBOLD\e[m" | less -r

If I combine the 2 versions:

echo "B\bBO\bOL\bLD\bD\n\e[1mBOLD\e[m" | less

I get:

Screenshot 2023-07-22 at 13 45 00

@st0012
Copy link
Member Author

st0012 commented Jul 22, 2023

It turns out because I use ohmyzsh, which sets LESS=-R, the output is displayed correctly on my machine.

@st0012 st0012 force-pushed the implement-#646 branch 4 times, most recently from 20505ee to 54a68ca Compare July 22, 2023 20:06
This can:

- Make it easier to scroll up and down the commands list
- Avoid pushing up users' previous output
- Allow users to do basic search with `/<word>`

if pager.first == 'less' || pager.first == 'more'
pager << '-R' unless pager.include?('-R')
end
Copy link
Member Author

Choose a reason for hiding this comment

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

If the user has PAGER=less set, we still want to make sure -R or any future flags are appended to the command so the content could be displayed properly without asking users to change things.


class << self
def page
if STDIN.tty? && pager = setup_pager
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if we should check STDIN.tty? or STDOUT.tty?. In most cases they should return the same value?

Copy link
Member

Choose a reason for hiding this comment

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

👍 It's consistent with IRB shows message Switch to inspect mode. when stdin is not a tty.

irb → both tty
irb > output_file → stdin is a tty
cat input_file | irb → stdout is a tty
cat input_file | irb > output_file → neither is tty

@tompng tompng merged commit f94e8a6 into master Jul 25, 2023
47 checks passed
@tompng tompng deleted the implement-#646 branch July 25, 2023 18:33
matzbot pushed a commit to ruby/ruby that referenced this pull request Jul 25, 2023
environment
(ruby/irb#647)

This can:

- Make it easier to scroll up and down the commands list
- Avoid pushing up users' previous output
- Allow users to do basic search with `/<word>`

ruby/irb@f94e8a66dd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

Display show_cmds's output in pager
2 participants