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

FR: selectively enable/disable pagination #2926

Open
jrobsonchase opened this issue Feb 3, 2024 · 9 comments · May be fixed by #3657 or #2927
Open

FR: selectively enable/disable pagination #2926

jrobsonchase opened this issue Feb 3, 2024 · 9 comments · May be fixed by #3657 or #2927
Labels
enhancement New feature or request

Comments

@jrobsonchase
Copy link
Collaborator

jrobsonchase commented Feb 3, 2024

Is your feature request related to a problem? Please describe.

The existing pagination config feels insufficient. One can leave ui.paginate on auto, but then you have to pass --no-pager to every command that you don't want paged every time. Or, you can set it to never, which then requires you to invoke every command that you do want paged with --color=always | less -FRX, since there's no way to turn the pager back on.

Even if there was a --pager argument to re-enable it when it's globally set to never, it would get tedious to have to pass it to every command that you always want paged every time, or awkward to have to use aliases for all of them.

The extra-frustrating bit is that all help messages are paginated, which I never want. Even if you pass --no-pager, they still get paged unless you have ui.paginate = "never". Seems like you have to pipe them to cat to get the auto-pager to turn off, which seems backwards.

My personal preferences:

  • help messages: never paged. I probably want to see the options available at the same time I'm editing my command, and flipping back and forth between the paged help and the cli isn't a good time. I want it to go straight to stdout so I can see both at the same time. If it's a really big one, I should be able to pipe it to less or add a --pager argument when I decide it's needed.
  • status, st: never paged. This one is so short, or at least should be, that paging doesn't seem worth it. I'm also just used to being able to see previous statuses in my scrollback buffer.
  • log: Depends on what I'm trying to do. If I'm hunting through history for something, I'll want it paged. If I know what I'm looking for, I probably don't want it paged so that I don't have to do the back-and-forth dance between pager and CLI to copy change IDs.
  • cat: Why is this paged in the first place? That's not what cat does. Never paged.

Describe the solution you'd like
A clear and concise description of what you want to happen.

  • a --pager flag for inverting ui.paginate = "never". "never" is strictly worse than "auto" without a way to override it the same way that --no-pager works in "auto" mode.
  • Per-command pagination configuration. Not everyone has the same preferences as me, and they likely don't have the same "Paginate ALL (or none of) the things!" preference that is the jj default, and will probably just feel the same pain I've described, just in different places. These settings are probably what should be used when ui.paginate is set to "auto".
  • ui.paginate = "always", mostly for completeness. This is effectively what "auto" means today.

Describe alternatives you've considered

Mostly described in the description of the problem.

#2621 and #1509 touch on this issue (#2621 explicitly), but the why-not reasoning in #1509 doesn't really hold for the pagination case. Since it's nonsensical to enable pagination when piping to another command, there's no chance of someone's default pagination settings breaking a script that calls jj to get some command output. The only thing that it could make a little weird is if a script is running jj specifically to present something to the user and expects a specific pagination setting. But in that case, the script should really be calling it with --(no-)-pager anyway.

@martinvonz
Copy link
Owner

Just to be sure - especially since you say that you want jj status to be not paged because it's typically short - are you aware of the -F flag for less? Our default pager is less -FRX (i.e. including that flag). That makes less exit and leave the contents on screen if it fits on one page.

@benbrittain
Copy link
Collaborator

Our default pager is less -FRX (i.e. including that flag). That makes less exit and leave the contents on screen if it fits on one page.

This explains so much and removes the friction I've been experiencing with the pager setup based upon the last few minutes of experimenting. I would recommend this to everyone who uses jj, either changing their $PAGER or setting it in the config.

@jrobsonchase
Copy link
Collaborator Author

jrobsonchase commented Feb 3, 2024

Interesting - I didn't realize my $PAGER environment variable was being used instead of the default (or that it was even set 😛). Mine was set to less, which is why things were always paging. I'm not actually sure where that's getting set, since it doesn't seem exist in my nixos configs anywhere.

Edit: Oh, it's just a nixos default 🙃

Still, better configuration seems valuable even with status never getting paged for <10 lines of output, since the pre-existing issue was unrelated to the less -F confusion. And there still remains the "auto can be overridden, but never is forever" issue.

@jrobsonchase jrobsonchase linked a pull request Feb 3, 2024 that will close this issue
4 tasks
@martinvonz
Copy link
Owner

Just setting PAGER=less is actually fine, because we also set LESS=FRX if it's not already set. So the problem is if you have LESS set but without R in it.

FYI, @benbrittain said on Discord that he's going to look into setting LESS=FRX whether or not you have LESS set.

@benbrittain
Copy link
Collaborator

#2928 Nothing complicated, but should be a small UX win

@jrobsonchase
Copy link
Collaborator Author

Just setting PAGER=less is actually fine, because we also set LESS=FRX if it's not already set. So the problem is if you have LESS set but without R in it.

That doesn't seem to be happening. I've got PAGER set but not LESS, and it still pages status.

I do have some random other less environment variables though:

LESS_TERMCAP_se=
LESS_TERMCAP_so=
LESS_TERMCAP_mb=
LESS_TERMCAP_me=
LESS_TERMCAP_md=
LESSOPEN=|/nix/store/wy0f1rp6mxc9g5k2w5gkfdm4yyzssk11-lesspipe-2.11/bin/lesspipe.sh %s
LESS_TERMCAP_ue=
LESS_TERMCAP_us=
PAGER=less
LESSKEYIN_SYSTEM=/nix/store/3aslgpqjnqlylqcxzpgdiwvxh0v1fdr6-lessconfig

@martinvonz
Copy link
Owner

That doesn't seem to be happening. I've got PAGER set but not LESS, and it still pages status.

Oh, silly me, it worked for me because I had LESS=FRX in my shell :)

@PhilipMetzger PhilipMetzger added the enhancement New feature or request label Feb 4, 2024
@fynnsu
Copy link

fynnsu commented May 9, 2024

I also just ran into this setting up jj for the first time and following the tutorial. Every command I ran would open the pager, which makes it challenging to follow the tutorial since you need to remember the change ids to rebase.

Setting ui.pager="less -FRX" fixed this for me, however, I had to do some searching to find that. I think the "initial user experience" could be improved by either changing the default config or mentioning the paging option on the installation and setup page https://martinvonz.github.io/jj/v0.17.1/install-and-setup/.

martinvonz added a commit that referenced this issue May 9, 2024
Our current default for `ui.pager` is this:

```toml
ui.pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }
```

If the user has `$PAGER` set, we take that value and replace the above
table by a scalar set to the value from the environment variable. That
means that anyone who has set `$PAGER` to just `less` will lose both
the `-FRX` and the charset, making e.g. colored output from `jj`
result in escaped ANSI codes rendered by `less`. The lack of those
options might not matter for other tools they use so they might not
have realized that they wanted those options.

This patch attempts to improve the situation by changing the default
`ui.pager` to pass `-FRX` via `$LESS`, and by setting the value from
`$PAGER` in `ui.pager.command`, so the rest of the config is left
alone.

The default config will still be ignored if you set the scalar
`ui.pager` to e.g. `less`, since that overrides the table.

Closes #2926
@martinvonz martinvonz linked a pull request May 9, 2024 that will close this issue
4 tasks
martinvonz added a commit that referenced this issue May 10, 2024
Our current default for `ui.pager` is this:

```toml
ui.pager = { command = ["less"], env_default = { LESS = "-FRX", LESSCHARSET = "utf-8" } }
```

If the user has `$PAGER` set, we take that value and replace the above
table by a scalar set to the value from the environment variable. That
means that anyone who has set `$PAGER` to just `less` will lose both
the `-FRX` and the charset, making e.g. colored output from `jj`
result in escaped ANSI codes rendered by `less`. The lack of those
options might not matter for other tools they use so they might not
have realized that they wanted those options.

This patch attempts to improve the situation by setting the value from
`$PAGER` in `ui.pager.command` so the rest of the config is left
alone.

The default config will still be ignored if you set the scalar
`ui.pager` to e.g. `less`, since that overrides the table.

Closes #2926
martinvonz added a commit that referenced this issue May 10, 2024
Our current default for `ui.pager` is this:

```toml
ui.pager = { command = ["less"], env_default = { LESS = "-FRX", LESSCHARSET = "utf-8" } }
```

If the user has `$PAGER` set, we take that value and replace the above
table by a scalar set to the value from the environment variable. That
means that anyone who has set `$PAGER` to just `less` will lose both
the `-FRX` and the charset, making e.g. colored output from `jj`
result in escaped ANSI codes rendered by `less`. The lack of those
options might not matter for other tools they use so they might not
have realized that they wanted those options.

This patch attempts to improve the situation by setting the value from
`$PAGER` in `ui.pager.command` so the rest of the config is left
alone.

The default config will still be ignored if you set the scalar
`ui.pager` to e.g. `less`, since that overrides the table.

Closes #2926
@BatmanAoD
Copy link
Collaborator

The less -FRX behavior seems good. Is there some way to make the :builtin pager have similar behavior to -F?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
6 participants