feat: add pager to allow scrolling of long outputs in the REPL

[Snehil-Shah]

Resolves #2149

Description

What is the purpose of this pull request?

This pull request:

• adds a pager to scroll through long outputs in the terminal using arrow keys.

The pager allows scrolling in place meaning the user stays in the REPL, when scrolling through. (demo attached below)

pager.2.mp4

Related Issues

Does this pull request have any related issues?

This pull request:

• resolves [RFC]: add help() documentation pager in REPL #2149
• relates to [Idea]: building a better Node.js REPL google-summer-of-code#1

Questions

Any questions for reviewers of this pull request?

• The above is the design I had in mind. Is the design good? I feel it's better than a less type of behaviour, as the user stays in the REPL and can scroll up to see previous commands and outputs while in scroll mode.

• The current implementation also allows scrolling of all outputs rather than just the help() documentation. I felt this is a useful feature in general and not just limited to documentation (paging readFile outputs for example)

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

As mentioned in the RFC, it would be helpful to know the approach taken by @kgryte in prior attempts at implementing this...

Checklist

Please ensure the following tasks are completed before submitting this pull request.

• Read, understood, and followed the contributing guidelines.

---

@stdlib-js/reviewers

[kgryte]

@Snehil-Shah Thanks for providing a concrete implementation. However, I am not sure that it is a good idea to auto-page. There are likely to be times when paging is not what you want.

For example, if a function returns a large blog of JSON, I may want to copy the entire blob and paste in my editor for further processing. If we adopted this PR, it is not clear to me that is possible anymore when auto-paging is enabled, as the entire content is never displayed in the terminal at any one time.

And I am not of the opinion we should add "special" behavior to paging to support this use case.

Instead, I might suggest exposing the pager as a function. E.g., something along the lines of

In [1]: pager( foo( 'returns really long text...' ) )

where pager takes the results of foo and pages.

In short, the user should explicitly opt-in to paging when applied more generally. Data analysis tasks in which you want to clean-up and munge some data before copying the results for further processing elsewhere is just too common a task (I, for one, do this quite regularly) to make the behavior currently introduced in this PR the default.

[kgryte]

FWIW: exposing the pager as a separate function was my initial approach for implementing more. In that case, I experimented with adding as a separate stdlib package which could then be used by REPL utilities, just like any other stdlib package. I was interested in a separate package, as the functionality is useful on its own and outside the REPL.

And note that I did not try to implement less; I explicitly tried reimplementing more (with support for backward navigation), as it has simpler API behavior and a simpler set of supported vi commands.

[Snehil-Shah]

@kgryte with the current behaviour, hitting CTRL+X exits paging and we can see the entire output at once to copy.
But yes, I understand auto-paging isn't ideal, and a pager command would be a better approach. Will work on it.

Can I get some feedback on the in REPL scrolling UI? How is it compared to a full window less type UI?

[kgryte]

By UI, you mean the still visible command which triggered paging?

[kgryte]

In general, the advantage of more, less, most, man is that the full-window view is temporary. When I quit the view, my terminal doesn't have a bunch of text I may not be interested in keeping around. Additionally, by switching the application "mode", you run less of a risk of having clashing keyboard shortcuts. Once you switch to pager mode, it has its own defined set of commands for scrolling, searching, etc; and once you switch back, you return to normal REPL commands/shortcuts.

[Snehil-Shah]

@kgryte I think we should leave clearing the terminal to the user using the clear command.

The current implementation, allows the user to see the previous stuff that has happened in the REPL both in scroll mode and after leaving the scroll mode.
In my opinion, the pager command shouldn't inherently and magically clear the entire screen as the user might want to see previous outputs later as well. Instead it should just do what it says: allow scrolling, and when done continue using the REPL like normal..

Regarding a pager mode, the current implementation employs this using the isPaging flag. In pager mode all other keypresses and their behaviours are discarded so no clashing happens.

[Snehil-Shah]

@kgryte I am attaching some clips of the current behavior of the pager UI

• Only the UP/DOWN keys, CTRL+X and SIGINT triggers work when paging

pager.2.mp4

• You can still scroll up using the mouse as nothing is cleared

allow_mouse.1.mp4

• Upon CTRL+X the entire output is printed and the next prompt is presented, meaning the user can scroll back up, view, copy and do their thing

ctrlx.1.mp4

[Snehil-Shah]

@kgryte OK so I researched and tried some stuff, and it might need some discussion

1. pager(foo()), function foo console logs some text to the screen and also returns some value. With the current implementation both are printed to stream as

text
Out[n]: value

We can control the printing of the return value as we explicitly do it ourselves and so can intercept it easily to check for paging.

The problem comes with the write operations from the function. Currently compiledCode.runInContext( <repl's context> ) is responsible for printing the output. The repl's context has a property context.console which is set to ostream and that is where such outputs are printed.

2. To avoid intercepting the write method to check for long outputs, the other way of doing this can be making a new context just for executing expressions we receive with the pager command. This context would have a different console which we will intercept fully. This would work as expected for user-defined functions/expressions in the REPL that use console.log. Now there are problems with this approach too.

The problem: REPL-specific commands like help() etc, don't rely on the context provided in compiledCode.runInContext( <repl's context> ), instead it writes to the REPL instance's ostream. This same REPL instance stores these commands in it's own cmds array property.

3. So to actually implement this, we will have to make a separate REPL and context to execute the expressions we recieve inside the pager or change the way REPL commands write to output.

4. We can do this, but I feel it's a bit far-fetched.
   The straightforward way would be to execute the expressions in the same REPL context and use the same stream to listen for tall outputs. If encountered, we enter paging. We are only overriding the write method to listen for tall outputs that too only when we are inside the pager() command, and we are using the same stream to write the paged outputs as well (as we should). So I don't clearly see where this approach could be a problem. Although I am not the expert here. Would love a concrete example where this approach might be an issue.

If there are issues with this approach too, then we can stick to the original RFC of just paging help() documentation (straighforward) and postpone the pager command for now.

What do you think?

[kgryte]

@Snehil-Shah Thanks for investigating and summarizing. I don't have a good sense at the moment what the "right" (if there is such a thing) approach should be without doing a bit of digging myself. With a bit of luck, I'll get a bit of time this weekend to poke around at the REPL implementation and circle back.

[kgryte]

One other addendum: there's also the scenario where a user resizes a terminal to less than 80 columns. This will cause text to wrap to the next line and muck with rendering. For clarity, I don't think we should do anything about this. All of the documentation assumes at least 80 columns (i.e., the default terminal width), and accommodating narrower views would require reflowing text (and examples!), which is definitely beyond the scope of this PR.

[kgryte]

Another something I noticed is that when doing

In [1]: pager( readFile.sync( './README.md' ).toString() )

the rendering seems to get messed up. This may be due to text wrapping beyond the number of terminal columns and can be considered related to my previous comment.

I suppose this is a more general problem. Suppose a function returns a very long string with no line breaks and which takes up more rows that currently visible within the viewport. Currently, we base paging on line breaks but that is not the only way to get long output. We should probably make this more robust, but this should be left to a follow-up PR.

[Snehil-Shah]

@kgryte Made some changes (avoiding welcome text to trigger paging and a SIGINT check to exit paging) and fixed some bugs related to viewport height and resizing the terminal. It's working as expected now at all terminal heights.

The readline's completer doesn't seem happy with the pager and I can't seem to figure out why.

Each completion is on a new line. Moreover, if the list of tab completions are taller than the terminal, nothing is printed and we presumably enter paging mode (as the page is blank and cursor is hidden). Only after pressing q, all the completions are printed as normal and we get the next prompt.

I don't think we need to stress about it now as we have written our own completer in #1855 and we can change it to use the _wstream to display completions. Once this PR is merged, I will do some updates to the new completer engine like listening to resize events, moving TAB auto-completion logic to completionPreviews (along with -> and ENTER keys) etc., and then that PR should be done (will ping once done, for review). The completer should be fixed after that...

7. Moved viewport querying up to the REPL. It's not perfect. Atm, a user could provide a non-TTY stream and then invoke the pager() command and we'd presumably choke as there will likely not be a viewport. We can consider this an edge case and punt on making more robust.

Can't we add a check in the pager command to check for isTTY? If it's not TTY then just print the string as usual.
But now that I think of it, the user can also go to settings and enable autoPage and create problems. We need to find a way to hide TTY-specific settings in non-TTY contexts or at least check for isTTY implicitly inside the implementations (of completionPreviews or autoPage) itself and avoid doing anything if non-TTY.

Regarding tests:

I know we shouldn't be accessing internal properties in tests, but this was the only way I could get it to enter paging. After it enters paging this is how the data array looks like:

I can't see the 'Use up/down arrows...' help text in there, but we know it's paging as we don't see the entire output of the help command. Doing the same thing with autoPage disabled writes the entire output.

Should I move ahead with the tests using this?

[kgryte]

@Snehil-Shah Yeah, testing the pager seems tricky. One thing you could try is to put the output stream in "object mode".

Regarding internal properties, I'd approach this differently. Rather than access via repl._wstream.columns, just mimic a TTY output stream when passing to the repl() constructor. E.g.,

var inspectSinkStream = require( '@stdlib/streams/node/inspect-sink' );
var repl = require( './fixtures/repl.js' );

// ...

var ostream = inspectSinkStream( onWrite ); // <= could also put into object mode
ostream.columns = 80;
ostream.rows = 42;
var opts = {
    'input': new DebugStream({...}),
	'output': ostream,
    // ...
};
var r = repl( opts, onClose );

[kgryte]

Can't we add a check in the pager command to check for isTTY? If it's not TTY then just print the string as usual.

I think we can punt on this for now. I think it is enough to make the default behavior TTY-specific. If a user starts mucking about and fiddling, for now, it is fine to leave it "buyer beware".

[kgryte]

Should I move ahead with the tests using this?

Yes, actually that test output seems reasonable. If you join( '' ) it, strip of ANSI escape codes, and compare to expected, I think you should be fine.

Also, to avoid test fixtures changing out from beneath you (e.g., we should not assume the contents of any API's help text), I suggest using userDoc() (see help( userDoc ) in the REPL) to create REPL test fixtures. E.g.,

var fixture = '...';
istream.write( 'userDoc( "foo", "' + fixture + '" );' );
istream.write( 'help( "foo" )' );

[kgryte]

Re: TAB completion. Indeed, that is strange. Not sure why it formats differently. And agreed, we can punt on this in this PR and address in a follow up PR.

[Snehil-Shah]

@kgryte Wrote tests for the pager, let me know if they are okay or needs changes...

Btw, when I was trying to push my changes, I was getting some errors by the pre-push hook:

> git push origin repl-pager:repl-pager
Checking if remote branch exists...
Remote branch exists.
Checking for commits to push...
Checking licenses...
No dependency licensing issues detected.
Running JavaScript test files...

Running test: /workspaces/stdlib/lib/node_modules/@stdlib/repl/test/integration/fixtures/repl.js


make: *** [/workspaces/stdlib/tools/make/lib/test/javascript.mk:144: test-javascript-files-local] Error 1

Encountered an error when running JavaScript tests.

error: failed to push some refs to 'https://github.com/Snehil-Shah/stdlib.git'

I think it's also trying to run fixture files as test files..

[kgryte]

@Snehil-Shah Thanks for working on the tests. I'll try and take a look in the next day or so.

[kgryte]

Btw, when I was trying to push my changes, I was getting some errors by the pre-push hook

Thanks for catching. I've found the error in our hook script and will push a patch shortly.

Although it is not clear why attempting to "run" the fixture file should be problematic, given that the file has no side effects.

[kgryte]

LGTM. Thanks, @Snehil-Shah!