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

Implement basic pager using TerminalMenus #38956

Merged
merged 3 commits into from
Apr 9, 2021

Conversation

MichaelHatherly
Copy link
Member

This is just a quick initial draft of a terminal pager built with TerminalMenus that can be used to display objects that might take up more than a screen's worth of space vertically. See linked demo video for an example usage. If there's interest in having something like this in base then I'll put some docs, tests, etc in.

asciicast

Somewhat related: #6921, #36460, #34226

@fingolfin
Copy link
Contributor

Nice! How hard would it be to implement a few more conveniences, in particular hotkeys for ...

  • scrolling a full/half page back/forward (usual hotkeys: space, shift-space, page up/down, ...);
  • jump to start/end;
  • search forward/backwards.

@MichaelHatherly
Copy link
Member Author

scrolling a full/half page back/forward (usual hotkeys: space, shift-space, page up/down, ...);

PageUp and PageDown keys are already provided for free by AbstractMenu.

jump to start/end;

Same for these, Home and End keys also already just work.

search forward/backwards.

I was also wondering whether it would be worth implementing this as well. Probably slightly more tricky to implement (as in not already provided 🤣).

@fingolfin
Copy link
Contributor

Nice!

For me the most important really would be space and shift-space (can't easily type pageup/down without leaving the homerow on my keyboard.

@MichaelHatherly
Copy link
Member Author

space and shift-space (can't easily type pageup/down without leaving the homerow on my keyboard.

I think those should probably be a separate PR/discussion since they're generic to scrolling any kind of menu. I'm also not sure how to capture shift-space either to be honest.

Copy link
Sponsor Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

Looks great. Just a couple of comments. Tests would be nice!

stdlib/REPL/src/TerminalMenus/Pager.jl Outdated Show resolved Hide resolved
print(buf, pager.lines[idx])
end

function writeLine(buf::IOBuffer, pager::Pager{<:Dict}, idx::Int, cursor::Bool)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Since this is a new menu type, do you need to support the legacy interface? There wouldn't be any callers of it out in the wild, and no one using Julia 1.5 or earlier will be able to use the pager. So I think you can cleanly target the new API.

In case you're unfamiliar, see #35915.

stdlib/REPL/src/TerminalMenus/Pager.jl Outdated Show resolved Hide resolved
@MichaelHatherly
Copy link
Member Author

Thanks for the review @timholy, I'll address the suggestions once I next get some spare time.

@fingolfin
Copy link
Contributor

I think those should probably be a separate PR/discussion since they're generic to scrolling any kind of menu.

Actually, for a menu, I'd expect space to toggle the currently "selected" item; so I am not sure this is separate? Anyway, of course things can be added later! I just think that it might be useful to have a look at the less help screen and implement as many of those as one can with reasonable effort; the more of these there are, the more people will likely be happy. But again, of course this can be done later, and also by others!

I'm also not sure how to capture shift-space either to be honest.

Indeed, that seems to be a problem. But actually less also doesn't support it (web browsers do, though). Nothing to worry about, anyway :-).

stdlib/REPL/src/TerminalMenus/Pager.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2021
@MichaelHatherly
Copy link
Member Author

Thanks for updating this @vtjnash, I'm just going to add some tests to it now before it gets merged.

@StefanKarpinski
Copy link
Sponsor Member

This is cool but a bit buggy, at least under iTerm2. I'm sure it will get sorted out if we start using it though 😁

@carstenbauer
Copy link
Member

carstenbauer commented Apr 19, 2021

For reference, there is a related discussion on discourse revolving around the new package TerminalPager.jl (which also supports horizontal scrolling etc. However, it has dependencies like Crayons.jl

@carstenbauer
Copy link
Member

carstenbauer commented Apr 19, 2021

Eventually, I think it would be cool and useful to have a pager option in our help REPL mode (?)!

@simeonschaub simeonschaub removed the status:merge me PR is reviewed. Merge when all tests are passing label Apr 28, 2021
ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
johanmon pushed a commit to johanmon/julia that referenced this pull request Jul 5, 2021
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

Successfully merging this pull request may close these issues.

None yet

7 participants