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

cli: Provide better flags for the "less" pager than default #2928

Closed
wants to merge 1 commit into from

Conversation

benbrittain
Copy link
Collaborator

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

cli/src/config.rs Outdated Show resolved Hide resolved
Copy link
Owner

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

LGTM, but it's probably good to give others a day or something to spot any problems we've missed.

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 3, 2024

I think this should be documented in https://martinvonz.github.io/jj/v0.13.0/config/#pager. The simplest thing would be to change a sentence to say:

less -FRX is the default pager in the absence of any other setting, or if $PAGER is exactly equal to less.

You could also add footnote about how to disable this behavior.

If it's a chore, this doesn't have to be a blocker; we can do it in a separate PR.

Overall, I think this idea could work; we'll see if anybody gets annoyed by it. I can imagine somebody not liking -FX, but I think these people will be few.

One thing I like about this implementation is that it won't affect people who set PAGER to less --mouse -FRX as I used to do.

@yuja
Copy link
Collaborator

yuja commented Feb 4, 2024

Overall, I think this idea could work; we'll see if anybody gets annoyed by it. I can imagine somebody not liking -FX, but I think these people will be few.

I don't like -F, but I'm not strongly against this change. User can always override the default.

fwiw, both git and hg respect $LESS if set.

@benbrittain
Copy link
Collaborator Author

I almost feel like the actual right answer is to have a built-in pager with the defaults jj would prefer, then allow users to set preferring the system pager if they want.

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 4, 2024

I almost feel like the actual right answer is to have a built-in pager with the defaults jj would prefer, then allow users to set preferring the system pager if they want.

We discussed this a few times, but unfortunately didn't create a bug for it.

https://github.com/markbt/streampager would be a nice option, but it seems no longer maintained. It's what Sapling uses. I think I found a few more Rust pagers, but they seemed very new.

There are some nice Golang pagers (moar and ov), but I don't know how to link them to a Rust program. (They are not perfect; I tried using them instead of less for a while, but there is always some minor issue that made me switch back to less. But they have better defaults).

@jyn514
Copy link
Collaborator

jyn514 commented Feb 7, 2024

There are some nice Golang pagers (moar and ov), but I don't know how to link them to a Rust program.

as a last resort, if you really want to distribute jj as a single static binary, you could embed the entire Go binary as static data in the rust binary, and then unpack it at runtime into a binary in the same directory (or run it in-memory with https://docs.rs/memfd-exec). but i know that's kinda hacky.

Comment on lines +327 to +336
// If 'less' is the value of PAGER, do not override the pager.
// Many systems ship with "R" as the only setting in $LESS, which provides
// a less than ideal experience for 'less' with jj, since they won't see the
// log history persist on the screen. While slightly inconsistent to ignore
// that setting, it should provide a better "out-of-box experience". If a user
// truly wants to change the values that are passed to "less", they can set that
// in the ui.pager jj config.
if value != "less" {
builder = builder.set_override("ui.pager", value).unwrap();
}
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe this change should be mentioned in the changelog?

@epage
Copy link

epage commented Feb 11, 2024

As I commented on the Issue, if you are interesting all of what git does, see https://github.com/epage/git-dive/blob/main/src/git_pager.rs

As for built-in pagers, one I hadn't seen mentioned is https://crates.io/crates/minus.

@ilyagr
Copy link
Collaborator

ilyagr commented Feb 11, 2024

Right, that's a good one! procs uses minus in exactly the way we would, so we can learn from them.

One annoying limitation is that minus doesn't implement any sort of in-program help.

@benbrittain
Copy link
Collaborator Author

Closing in favor of #3024 and anything else that will come from that direction of a solution instead of special casing less

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.

6 participants