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

Use minus as a builtin pager #3024

Merged
merged 1 commit into from
Feb 12, 2024
Merged

Conversation

benbrittain
Copy link
Member

@benbrittain benbrittain commented Feb 11, 2024

Added a built-in pager since the solution in #2928 felt hacky. Also should address #2044

Checklist

If applicable:

  • [X ] I have updated CHANGELOG.md
  • [X ] I have updated the documentation (README.md, docs/, demos/)
  • [X ] I have updated the config schema (cli/src/config-schema.json)

@benbrittain benbrittain marked this pull request as draft February 11, 2024 12:24
@benbrittain benbrittain force-pushed the minus-pager branch 3 times, most recently from 8e0b39b to 7d77274 Compare February 11, 2024 15:33
@benbrittain
Copy link
Member Author

This is currently using a branch of mine so that I could see if I could keep the -X flag behavior which was the default before. AMythicDev/minus#128

@benbrittain benbrittain force-pushed the minus-pager branch 3 times, most recently from 14d1022 to d963300 Compare February 11, 2024 15:54
@benbrittain benbrittain marked this pull request as ready for review February 11, 2024 15:56
@benbrittain
Copy link
Member Author

One downside is that minus does not support the less -F equivalent in dynamic mode (not even starting the pager if everything will fit on the screen). I did try using the static mode but it's noticeably slower (not actually measured, just user perception based).

The minus docs say this will never be supported due to the impossibility of knowing when the dynamic output is going to end, we have more information on when that will occur as the caller of the library, maybe we can get that information propagated through?

@benbrittain
Copy link
Member Author

I've been using jj with this for a little bit, there is definitely fine tuning left to do. jj show is a noticeably degraded experience right now since it winds up taking over the whole window.

@benbrittain benbrittain force-pushed the minus-pager branch 6 times, most recently from 841e468 to e71d2da Compare February 11, 2024 19:11
Copy link
Contributor

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

LGTM; I'll give this a spin on my Windows box later tonight if nobody else gets to it in the meantime, but I'm excited to see this hole plugged up a bit.

As a side note, I'd personally prefer if we tried to avoid using forks via cargo git dependencies (it's just so easy to let them sit around for too long, in my experience, and it makes the Nix build more annoying and fiddlier); so just a follow up patch would be best, IMO. Or just hold off for a day or so if it'll get merged soon anyway.

CHANGELOG.md Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
@benbrittain benbrittain force-pushed the minus-pager branch 2 times, most recently from 0e7a475 to 9ada916 Compare February 11, 2024 19:32
@benbrittain
Copy link
Member Author

Very supportive of not pinning git repos 👍. I think if for some reason (which I don't think will happen) we can't work with minus upstream the right think to do is just republish the crate or vendor it. I think we should land this patch with upstream and iterate.

Also, after actively using it for a couple hours I think it's actually better without my patch until I figure out how to do early exit with minus. Losing the whole scroll back for things like jj status is pretty annoying.

Do you wanna give it a try on Windows first or should we merge as is?

cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
cli/src/ui.rs Outdated Show resolved Hide resolved
@benbrittain benbrittain force-pushed the minus-pager branch 3 times, most recently from 3482afd to c3fd9ed Compare February 12, 2024 02:42
@AMythicDev
Copy link

Hey guys! So I hope I am not being an unwanted guest here. @benbrittain actually mentioned about this in a PR. So I came here to take a look at the actual code being written. While reading your comments, I found this.

The minus docs say this will never be supported due to the impossibility of knowing when the dynamic output is going to end, we have more information on when that will occur as the caller of the library, maybe we can get that information propagated through?

I don't think it's any true as of today. I had written that comment for <=v4.x.x releases which used a completely different model for text data that didn't allow this functionality for dynamic mode. As for v5.x.x releases this should be possible. I didn't implement this because it never crossed my mind earlier. So if you guys want this, just throw out a issue atleast.

@benbrittain
Copy link
Member Author

@arijit79 you are more than welcome here! Thank you for writing minus! I'm really excited to hear that about 5.x I'll file an issue later this evening 😄

@benbrittain benbrittain merged commit d3699c2 into jj-vcs:main Feb 12, 2024
15 checks passed
@benbrittain benbrittain deleted the minus-pager branch February 13, 2024 00:27
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.

5 participants