builtin pager: add configuration options, change default options #5415

ilyagr · 2025-01-21T08:24:19Z

I've been going back and forth on naming of the alternate-screen option. It has many subtle effects that seem hard to describe without thinking of the alternate screen (e.g. if we call it clear-screen), at least if we want to allow the if-long-or-slow behavior. If we are OK with forbidding it, we could have a simple quit-if-one-page boolean, but I think it might just be useful enough to keep.

Streampager's naming of the corresponding interface_mode option seems more confusing to me: the Direct mode seems useless, there are two hybrid modes (one of which is called Hybrid) that are hard to distinguish unless you think in terms of the alternate screen terminal mode.

TODOs

Describe streampager keybindings (here or another PR)
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)
(not planned) I have added tests to cover my changes

cli/src/ui.rs

yuja · 2025-01-21T10:17:45Z

cli/src/config-schema.json

+                    "type": "object",
+                    "description": "':builtin' pager configuration",
+                    "properties": {
+                        "alternate-screen": {


nit: it sounds like "(use) alternate screen" (noun). How about reset-screen or clear-screen?

Oh, but never means "no init" + "quit automatically". I feel "interface-mode" is better in that case.

I changed it to clear-screen.

If we went with interface-mode, I'm not sure what the options should be so that they are not too long and clear. I don't like the choices Streampager made for these names.

But I'm unsure what's best here.

yuja · 2025-01-21T10:22:21Z

cli/src/ui.rs

+        // This uselessly reads ~/.config/streampager/streampager.toml, even
+        // though we then override the important options. TODO(ilyagr): Fix this
+        // once/if https://github.com/facebook/sapling/pull/1011 is merged.
        let mut pager = streampager::Pager::new_using_stdio()?;


I think it still make sense to load the config file. There may be key bindings, etc.?

I would prefer to copy any such config into jj, especially since streampager's current docs at https://github.com/markbt/streampager are both incomplete and seem to be incorrect (as we discovered in the other thread, setting streampager.toml to the values shown there didn't seem to work for the version of Streampager we are using).

It seems difficult at the moment to improve streampager's own configuration and docs. If the situation changes later, and if more programs start using streampager, we could change our mind.

I would prefer to copy any such config into jj,

In that case, I would add config table specific to "streampager", not generic "builtin". It's unlikely all config knobs can be generalized.

I don't have a strong opinion about that. The current config loading at streampager side is broken, but we can send a fix to upstream. OTOH, if we fixed the upstream config deserialization implementation, maybe we can reuse it to load parameters from our TOML table.

I changed the key to ui.streampager. Perhaps we should also rename the pager to :builtin-streampager, with :builtin being an alias.

OTOH, if we fixed the upstream config deserialization implementation, maybe we can reuse it to load parameters from our TOML table.

That's what I was thinking.

I changed the key to ui.streampager. Perhaps we should also rename the pager to :builtin-streampager, with :builtin being an alias.

I have no strong opinion about that. It's unlikely we would add more than one builtin pagers. FWIW, the syntax is :<internal-name>, so the canonical name would be :streampager.

OTOH, if we fixed the upstream config deserialization implementation, maybe we can reuse it to load parameters from our TOML table.

That's what I was thinking.

In that case, shouldn't we follow the upstream naming convention (e.g. streampager.interface_mode = "delayed")? Otherwise, we'll need to reimplement deserializer.

https://github.com/markbt/streampager?tab=readme-ov-file#configuration

doing it only in those cases where Streampager's names seem good for us. For example, keybindings maybe.

Ok.

Re Streampager's names:

I agree it's unclear, but I think it's more correct than clear-screen = never|always|if-long-or-slow. It's hybrid in a way that the screen is not cleared and the pager exits immediately depending on the content length. (BTW, this part is what I don't like about streampager. I just want to turn off the screen reset as less -RX would do.)

EDIT: Oh, perhaps full-screen = never|always|if-long-or-slow might work? (if-long-or-slow could be auto if you like.)

I thought clear-screen="never" is better than full-screen="never", because, for long files, you do get a full-screen interface even if you choose "never" (but the screen is indeed never cleared).

The way I think of "clear-screen" is that it determines when the screen is cleared, and then there is the additional behavior that whenever the screen is not cleared, streampager exits on short inputs. I tried explaining this in config.md, I'm not sure whether it's enough to get people to not get confused.

I find echo aa | LESS= less -RX a bit weird, especially if you press up and q after it, which leaves a ton of ~s on the screen. So, I sort of understand why streampager chose its set of tradeoffs instead of less's, and from this perspective behaving as I described in the previous paragraph sort-of makes sense to me. From other perspectives, it's a headache.

I misunderstood "never" would be mapped to Direct. So, yes, it's kinda full-screen="if-long", and there's no "never".

To me, "clear/reset screen" sounds tightly related to terminal reset behavior, and I wouldn't expect it affects the interactive UI. Maybe it can be interactive=always|if-long|if-long-or-slow or the inverse auto-quit=never|if-one-screen|if-one-screen-and-quick?

I like these better, but they do not indicate the fact that "always" always clears the screen on exit, "if-long" never clears it on exit, and "if-long-or-slow" clears it only if the pager becomes interactive. When expressed in this way, I find it hard to wrap my head around.

There might be some way to present this information in a memorable way, similar to my attempt described in #5415 (comment) (that relied on focusing the user's attention on whether the screen is cleared, but is IMO more memorable if the user is willing to go along with that -- though I don't know how many will agree). Then, I'd be happy to switch. I don't have it in mind right now, but we have a few days to think about it.

One way that comes to mind, but is probably not great, is interactive=always|if-long-and-never-clear-screen|if-long-or-slow.

but they do not indicate the fact that "always" always clears the screen on exit,

Correct. I just assumed UI difference is more important to users than screen reset.

"if-long" never clears it on exit, and "if-long-or-slow" clears it only if the pager becomes interactive.

Oh, weird. Maybe better to provide two separate flags then (and error out on unsupported combination)?

(I also think copying the upstream interface-mode is okay given that this flag is super abstract and not possible to describe in one word.)

yuja · 2025-01-21T10:23:21Z

cli/src/ui.rs

+            Always => InterfaceMode::FullScreen,
+            Never => InterfaceMode::Hybrid,
+            IfLongOrSlow => InterfaceMode::Delayed(std::time::Duration::from_millis(
+                self.long_or_slow_delay_millis,


nit: no idea if we can generalize this mode. If it's not common, maybe we can omit the Delayed mode?

I'm undecided.

One concern I have is that even if we remove it now, if we also simplify the config option names accordingly, it would be difficult to add the Delayed mode back in if we need it. Perhaps this could be solved somehow.

For me, the upshot of the Delayed mode is that it addresses the downside of the default mode when using it to read long texts (e.g. manual pages) where it leaves the last page of what you were reading on on the screen, while still being useable for short outputs. I am not sure how many people would like/want this tradeoff.

A part of this problem could be addressed by #5311, making the Delayed mode less important.

ilyagr · 2025-01-21T22:43:49Z

Thank you for looking this over!

In the future, we could consider adding a few more special configs, e.g. `:pagerenv` that strictly uses the `PAGER` environment variable, and `:unix` that mimics Git's algorithm of trying `PAGER`, defaulting to `less`, and setting `LESS`.

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.

I mostly focused on: - keys for absolute beginners - keys for features for which it's not obvious they *have* a key binding

This was referenced Jan 21, 2025

config: add --when.commands scope #5311

Merged

FR: selectively enable/disable pagination #2926

Open

ilyagr force-pushed the ig/streamopts branch 3 times, most recently from efd8448 to 1e1cd7b Compare January 21, 2025 08:53

yuja reviewed Jan 21, 2025

View reviewed changes

ilyagr force-pushed the ig/streamopts branch 4 times, most recently from 66f5e4e to 0c2a6d9 Compare January 22, 2025 01:20

pager: refactor pager config (no-op)

5dbc4f1

In the future, we could consider adding a few more special configs, e.g. `:pagerenv` that strictly uses the `PAGER` environment variable, and `:unix` that mimics Git's algorithm of trying `PAGER`, defaulting to `less`, and setting `LESS`.

ilyagr force-pushed the ig/streamopts branch from 0c2a6d9 to ea6d4eb Compare January 29, 2025 04:00

ilyagr marked this pull request as ready for review January 29, 2025 04:02

ilyagr marked this pull request as draft January 29, 2025 04:02

ilyagr force-pushed the ig/streamopts branch from ea6d4eb to 6f41640 Compare January 29, 2025 04:11

ilyagr marked this pull request as ready for review January 29, 2025 04:13

ilyagr force-pushed the ig/streamopts branch 5 times, most recently from 017048a to 4d886aa Compare January 29, 2025 04:29

ilyagr force-pushed the ig/streamopts branch from 4d886aa to 4abca6c Compare January 29, 2025 04:30

built-in pager: document key bindings

cb63713

I mostly focused on: - keys for absolute beginners - keys for features for which it's not obvious they *have* a key binding

ilyagr force-pushed the ig/streamopts branch from 4abca6c to cb63713 Compare January 29, 2025 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

builtin pager: add configuration options, change default options #5415

builtin pager: add configuration options, change default options #5415

ilyagr commented Jan 21, 2025 •

edited

Loading

yuja Jan 21, 2025

ilyagr Jan 29, 2025

yuja Jan 21, 2025

ilyagr Jan 21, 2025 •

edited

Loading

yuja Jan 22, 2025

ilyagr Jan 29, 2025

yuja Jan 29, 2025

yuja Jan 29, 2025 •

edited

Loading

ilyagr Jan 29, 2025 •

edited

Loading

yuja Jan 30, 2025

ilyagr Jan 30, 2025 •

edited

Loading

yuja Jan 30, 2025

yuja Jan 21, 2025

ilyagr Jan 21, 2025

ilyagr commented Jan 21, 2025

builtin pager: add configuration options, change default options #5415

Are you sure you want to change the base?

builtin pager: add configuration options, change default options #5415

Conversation

ilyagr commented Jan 21, 2025 • edited Loading

TODOs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuja Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

ilyagr Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilyagr commented Jan 21, 2025

ilyagr commented Jan 21, 2025 •

edited

Loading

ilyagr Jan 21, 2025 •

edited

Loading

yuja Jan 29, 2025 •

edited

Loading

ilyagr Jan 29, 2025 •

edited

Loading

ilyagr Jan 30, 2025 •

edited

Loading