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.

Sign up for GitHub

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

Jump to bottom

switch builtin pager to streampager #4203

Merged

yuja merged 5 commits into jj-vcs:main from yuja:push-vwqpwvoxokrt

Jan 14, 2025

Contributor

yuja commented Aug 3, 2024

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

yuja commented

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

Contributor

ilyagr commented Aug 3, 2024 •

edited

Loading

I would really like a version of streampager with its dependencies updated for this (can/should we ask the Facebook people to do it? Should we fork it?). It's annoying to have to link an old version of clap (see the diff in Cargo.toml).

IIRC, streampager's interface seemed a bit annoying in parts by default, so we might want to change its default config (or try it out at least).

Apart from that, I think this might be great!

yuja mentioned this pull request

built-in pager: write a message to the user if pager doesn't start #4197

Merged

4 tasks

Contributor Author

yuja commented Aug 3, 2024

I would really like a version of streampager with its dependencies updated for this

Totally agree, and the stripped-down version exists in Sapling tree. Asked question at markbt/streampager#61

yuja marked this pull request as draft

August 3, 2024 14:30

yuja mentioned this pull request

ui: include error sources in pager warning #4206

Merged

4 tasks

yuja force-pushed the push-vwqpwvoxokrt branch from 8cb0ad3 to 193516d Compare

August 5, 2024 02:16

yuja force-pushed the push-vwqpwvoxokrt branch from 193516d to 7ea0283 Compare

August 17, 2024 03:25

yuja force-pushed the push-vwqpwvoxokrt branch 3 times, most recently from 2c07fa0 to b2e8fdf Compare

January 12, 2025 08:45

yuja changed the title ~~RFC: switch builtin pager to streampager~~ switch builtin pager to streampager

yuja marked this pull request as ready for review

January 12, 2025 09:08

Contributor Author

yuja commented Jan 12, 2025

Updated dependency to sapling-streampager.

yuja force-pushed the push-vwqpwvoxokrt branch 2 times, most recently from 097b69f to 7bf4644 Compare

January 12, 2025 10:59

martinvonz approved these changes

View reviewed changes

thoughtpolice approved these changes

View reviewed changes

yuja added 5 commits

January 14, 2025 10:00


          cargo: add missing "windows" feature to crossterm dependency

bdb5dff

Perhaps, the "windows" feature was enabled through "minus" or "scm-record". If I
removed "minus" in earlier revision, the Windows build of crossterm 0.27.0
failed.


          cargo: add sapling-streampager dependency

751a35f

The WTFPL license is added to the allow list. I've never heard about this
license, but it's basically the same as public domain according to wikipedia.


          cargo: add os_pipe dependency

be6a647


          ui: switch builtin pager to streampager

90b7b8f

According to the discussion on Discord, streampager is still maintained.
It brings more dependencies, but seems more reliable in our use case. For
example, the streampager doesn't consume inputs indefinitely whereas minus
does. We can also use OS-level pipe to redirect child stderr to the pager.


          cargo: remove minus dependency

4a50a11

yuja force-pushed the push-vwqpwvoxokrt branch from 7bf4644 to 4a50a11 Compare

January 14, 2025 01:00

yuja enabled auto-merge

January 14, 2025 01:04

yuja added this pull request to the merge queue

Merged via the queue into jj-vcs:main with commit be9483b

19 checks passed

yuja deleted the push-vwqpwvoxokrt branch

January 14, 2025 01:25

ilyagr reviewed

View reviewed changes

cli/src/ui.rs Show resolved Hide resolved

ilyagr added a commit to ilyagr/sapling that referenced this pull request


          streampager: allow running Streampager without reading its config files

ba52a57

Currently, there is no way for a calling program (like `sl` or `jj`) to
start streampager without it trying to read
`$CONFIG/streampager/streampager.toml` and the environment variables
`SP_INTERFACE_MODE`, `SP_SCROLL_PAST_EOF`, and `SP_READ_AHEAD_LINES`.
This is undesireable if we'd like to control Streampager's behavior
fully from `jj` config (or `sl` config). Sapling currently overrides
the most important Streampager config from its own config anyway.

I'd like there to be a way to invoke Streampager with a given config.
I carefully implemented it so that no changes to Sapling are required,
though I think it'd also benefit from using the API I introduce here
(and the old UI could be deprecated).

Another, less invasive, possibility would be to simply make
`Pager::config` public. However, this would mean that Streampager
would uselessly read `$CONFIG/streampager/streampager.toml` even
if the config is later fully overriden by `jj`.

As an aside, the docs for the config from
https://github.com/markbt/streampager#example-configuration do not seem
to be fully correct, setting `interface_mode = "delayed"` as described
there did not work for me. However, I'd rather not use that
configuration method at all for my purposes with `jj`.

Cc: jj-vcs/jj#4203 (comment),
@yuja.

As ever, thanks for making it convenient for us to use Streampager in
`jj`! :)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

6becf3b

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

deca07b

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

5114fa6

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

38e92f7

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr mentioned this pull request

FR: selectively enable/disable pagination #2926

Open

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options TODO: Look at t…

5a0e7bf

…ests?

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

bc8cc60

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options TODO: Look at t…

b171b40

…ests?

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

efd8448

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

1e1cd7b

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

0f4d128

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

0b61f94

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

69a9b5b

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

66e1351

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

00a3c23

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

66f5e4e

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

e5c94ff

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

0c2a6d9

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

32e789f

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

d72274f

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

baae872

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request


          streampager: allow running Streampager without reading its config fil…

26edb15

…es (#1011)

Summary:
This could be considered as a bug report/feature request instead of a PR.

Currently, there is no way for a calling program (like `sl` or `jj`) to start streampager without it trying to read
`$CONFIG/streampager/streampager.toml` and the environment variables `SP_INTERFACE_MODE`, `SP_SCROLL_PAST_EOF`, and `SP_READ_AHEAD_LINES`. This is undesireable if we'd like to control Streampager's behavior fully from `jj` config (or `sl` config). Sapling currently overrides the most important Streampager config from its own config anyway.

I'd like there to be a way to invoke Streampager with a given config. I carefully implemented it so that no changes to Sapling are required, though I think it'd also benefit from using the API I introduce here (and the old UI could be deprecated).

If there's a better way to achieve that than my approach here, I'd be happy to do that. Another, less invasive, possibility would be to simply make `Pager::config` public. However, this would mean that Streampager would uselessly read `$CONFIG/streampager/streampager.toml` even if the config is later fully overriden by `jj`.

As an aside, the docs for the config from
https://github.com/markbt/streampager#example-configuration do not seem to be fully correct, setting `interface_mode = "delayed"` as described there did not work for me. However, I'd rather not use that configuration method at all for my purposes with `jj`.

Cc: jj-vcs/jj#4203 (comment), yuja.

As ever, thanks for making it convenient for us to use Streampager in `jj`! :)

Pull Request resolved: #1011

Reviewed By: quark-zju

Differential Revision: D68191198

fbshipit-source-id: c1a180d8bf09a82f5439e2b32510c2f9eafc3d45

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

c165ab1

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit to ilyagr/jj that referenced this pull request


          built-in pager: allow configuring streampager options

521dca6

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in jj-vcs#4203, I didn't mention this in
the Changelog.

As discussed in jj-vcs#4203 (comment)

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

004f9f1

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

6103c94

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

1c2dc44

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

595487f

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

ilyagr added a commit that referenced this pull request


          built-in pager: allow configuring streampager options

fd31096

This also changes the default to be closer to `less -FRX`. Since this
default last changed very recently in #4203, I didn't mention this in
the Changelog.

As discussed in #4203 (comment)

I initially kept the config closer to streampager's (see
https://github.com/jj-vcs/jj/compare/main...ilyagr:jj:streamopts?expand=1), but
then decided to make it more generic, smaller, and hopefully easier to
understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet