Add pager #672

chooglen · 2022-10-24T16:35:03Z

Add pager support via the following config variables:

ui.pager: the program to use as the pager. Defaults to $PAGER.
ui.paginate: whether to paginate output. Supported values are 'auto',
'always' and 'never'. Defaults to 'auto'.

Support for the --pager (or should it be called --paginate?) flag is WIP.

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)

src/ui.rs

martinvonz · 2022-10-24T17:25:09Z

e.g. maybe we really like struct Ui

Yes, I think the goal should be to route clap's output through the Ui object. We have previously talked about also avoiding having clap exit the process (by calling try_get_matches_from() instead of get_matches_from()).

How we could change the default paging behavior per-command

Yes, that's what Mercurial does. For many commands (e.g. jj squash), it doesn't make sense to spawn a pager because the output is just a few lines at most.

Writes first go to a temporary buffer and the pager is spawned only if
the buffer is nonempty.

Mercurial lets the commands decide when to enable the pager (if it's configured to be used). That should remove the need for a temporary buffer.

src/ui.rs

Ralith · 2022-10-24T19:48:08Z

Whether we really need DynWriteLock or if there are simpler alternatives here

We should stream output directly to the pager rather than buffering it up in full first. A file descriptor (or ChildStdin) doesn't require locking to write to, so in that case DynWriteLock will become unnecessary.

yuja · 2022-10-25T02:52:51Z

A file descriptor (or ChildStdin) doesn't require locking to write to, so in that case DynWriteLock will become unnecessary.

Yep, I lied in the chat, sorry.

If we need line-buffered output (like stdout), I think &ChildStdin can be wrapped per stdout_formatter() call, instead of locking shared buffered writer.

chooglen · 2022-11-01T20:38:31Z

Ok this is ready for review again :)

src/main.rs

src/ui.rs

src/main.rs

src/ui.rs

lib/src/settings.rs

src/ui.rs

martinvonz

This definitely deserves a mention in the changelog! Probably also worth mentioning in docs/config.md.

src/ui.rs

We'll add a variant that isn't a pair. Also add a function to create a new UiOutput::Terminal, we will create this variant in a few places because we want to fall back to it.

Teach Ui's writing functions to write to a pager without touching the process's file descriptors. This is done by introducing UiOutput::Paged, which spawns a pager that Ui's functions can write to. The pager program can be chosen via `ui.pager`. (defaults to Defaults to $PAGER, and 'less' if that is unset (falling back to 'less' also makes the tests pass). Currently, commands are paginated if: - they have "long" output (as defined by jj developers) - jj is invoked in a terminal The next commit will allow pagination to be turned off via a CLI option. More complex pagination toggling (e.g. showing a pager even if the output doesn't look like a terminal, using a pager for shorter ouput) is left for a future PR.

chooglen · 2022-11-29T21:46:38Z

In a future PR, I want to be able to paginate jj --help. It's easy to paginate if I add request_pager() call where we handle clap errors, but the tricky part is getting jj --help --no-pager to work right, since --help short-circuits the arg parsing.

tests/test_global_opts.rs

martinvonz

Thanks!

martinvonz reviewed Oct 24, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

Ralith reviewed Oct 24, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

chooglen force-pushed the demo/pager branch 2 times, most recently from 1afc334 to 11f689d Compare November 1, 2022 20:05

chooglen changed the title ~~Demonstrate pager~~ Add pager Nov 1, 2022

chooglen force-pushed the demo/pager branch from 11f689d to 6f26dba Compare November 1, 2022 20:37

martinvonz reviewed Nov 1, 2022

View reviewed changes

src/main.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

chooglen force-pushed the demo/pager branch from 6f26dba to 6f230e6 Compare November 1, 2022 23:39

martinvonz reviewed Nov 2, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

src/main.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

chooglen marked this pull request as draft November 2, 2022 00:59

Ralith reviewed Nov 6, 2022

View reviewed changes

lib/src/settings.rs Show resolved Hide resolved

chooglen force-pushed the demo/pager branch from 6f230e6 to 863200f Compare November 9, 2022 01:00

Ralith reviewed Nov 9, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

chooglen force-pushed the demo/pager branch 5 times, most recently from 2dc4f2f to 6ef8312 Compare November 17, 2022 22:05

chooglen marked this pull request as ready for review November 17, 2022 23:13

chooglen commented Nov 17, 2022

View reviewed changes

src/ui.rs Show resolved Hide resolved

martinvonz reviewed Nov 18, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

Ralith reviewed Nov 18, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

src/ui.rs Outdated Show resolved Hide resolved

Ralith reviewed Nov 18, 2022

View reviewed changes

src/ui.rs Outdated Show resolved Hide resolved

chooglen mentioned this pull request Nov 23, 2022

cli: switch from atty crate to crossterm #783

Merged

2 tasks

chooglen added 2 commits November 29, 2022 12:56

cli: rename UiOutputPair to UiOutput

5d0c6ba

We'll add a variant that isn't a pair. Also add a function to create a new UiOutput::Terminal, we will create this variant in a few places because we want to fall back to it.

settings: move cli-specific settings to src/ui.rs

18e0e5b

chooglen force-pushed the demo/pager branch from 6ef8312 to 4efa555 Compare November 29, 2022 21:35

martinvonz reviewed Nov 29, 2022

View reviewed changes

tests/test_global_opts.rs Outdated Show resolved Hide resolved

martinvonz approved these changes Nov 29, 2022

View reviewed changes

cli: add --no-pager

3546054

chooglen force-pushed the demo/pager branch from 4efa555 to 3546054 Compare November 29, 2022 22:02

chooglen merged commit 0df5f7c into jj-vcs:main Nov 29, 2022

chooglen deleted the demo/pager branch November 29, 2022 22:14

martinvonz mentioned this pull request Dec 4, 2022

Add support for pager #9

Closed

mbStavola mentioned this pull request Aug 11, 2023

configs: add the ability to disable paging altogether via ui.paginate #2035

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pager #672

Add pager #672

chooglen commented Oct 24, 2022 •

edited

Loading

martinvonz commented Oct 24, 2022

Ralith commented Oct 24, 2022 •

edited

Loading

yuja commented Oct 25, 2022 •

edited

Loading

chooglen commented Nov 1, 2022

martinvonz left a comment

chooglen commented Nov 29, 2022

martinvonz left a comment

Add pager #672

Add pager #672

Conversation

chooglen commented Oct 24, 2022 • edited Loading

Checklist

martinvonz commented Oct 24, 2022

Ralith commented Oct 24, 2022 • edited Loading

yuja commented Oct 25, 2022 • edited Loading

chooglen commented Nov 1, 2022

martinvonz left a comment

Choose a reason for hiding this comment

chooglen commented Nov 29, 2022

martinvonz left a comment

Choose a reason for hiding this comment

chooglen commented Oct 24, 2022 •

edited

Loading

Ralith commented Oct 24, 2022 •

edited

Loading

yuja commented Oct 25, 2022 •

edited

Loading