search

sharkdp / bat

A cat(1) clone with wings.
Apache License 2.0
49.73k stars 1.25k forks source link

Use `minus` as a paging library #1540

Open AMythicDev opened 3 years ago

AMythicDev commented 3 years ago

I don't know what bat currently uses for paging (looks like less maybe) but it proposes two main problems. First is cross-compatiblity, bat cannot be packaged for Windows and requires less (which requires Mingw) to be installed. Second is less is binary and it's use as a library is not very perfect solution. These are the main problem minus solved. It's a pure rust library for paging output in a terminal. It provides great customisation of output which also being very compact

I wanted to know whether the bat team could implement this pager inside bat

Disclosure: I am the author of minus

sharkdp commented 3 years ago

Thank you for posting here! I already had a closer look at minus before and I was already considering to use it as a solution to #1053. It looks great.

Maybe it would make sense to move the discussion to that ticket?

In particular, I would be interested if it could solve all/some of the listed bullet points.

You mentioned cross-compatibility. Does minus support non-ASCII terminals?

AMythicDev commented 3 years ago

I don't think minus implments much of the things in the checklist. In my opinion there are some things there that's not part of a usual pager. Anyway if you want you could always fork and do some edits or if you're fine with what's already there then it could be implemented

AMythicDev commented 3 years ago

I had gone through a very quick and dirty implementation of minus within bat and it works perfectly. Although if you want to get the features of that checklist I think you probably need to fork minus

AMythicDev commented 1 year ago

Hii @sharkdp! It has been a long time here. All this time, minus has grown a lot. It now has:-

and much more...

Things like incremental searching and more are also on their way.

So I just wanted to ask whether can reconsider minus to be included inside bat.

academician commented 2 months ago

I did a quick integration of Minus into bat to see if I could, and it was pretty easy. It seems to work very well, in fact, and I rather like the idea of not having to depend on an external pager.

I'm not making a PR just yet because I'm not sure if this is up to the standards of the project (I didn't add any tests...), but I'd be happy to address feedback if we want to get it over the finish line.

https://github.com/sharkdp/bat/compare/master...academician:bat:minus-integration

There are a few open questions that I had to make choices on. For example:

academician commented 2 months ago

One call-out is that adding Minus seems to effectively add about 27 dependencies (cargo tree --locked | wc -l goes from 277 to 304). This increases the resulting binary size on my system (Ubuntu Noble x64 + Rust 1.80.1) from 5.9MB to 6.3MB, about a 6% increase. Seems acceptable to me based on the value gained, personally.

Another thing we might want to do is default to "Minus" if we don't find less, so that the OOTB experience of bat is improved.

From my understanding from CONTRIBUTING.md, here's a checklist for the work that remains for an MVP:

CosmicHorrorDev commented 2 months ago

(just a drive by comment)

The default cargo tree output will redisplay lines for common dependencies. The best check I know of is vv although there's probably some toml program you could run to count lock file entries instead

$ cargo tree --no-dedupe --prefix none | sort -u | wc -l
143
keith-hall commented 2 months ago

@academician nice, thanks for taking the initiative on this 🙂

This increases the resulting binary size on my system (Ubuntu Noble x64 + Rust 1.80.1) from 5.9MB to 6.3MB, about a 6% increase. Seems acceptable to me based on the value gained, personally.

I agree, that isn't so bad 👍

  • How the pager is selected - I chose to detect --pager=builtin/BAT_PAGER=builtin, but that could be wrong.

I like that solution and would probably have suggested the same thing. The chances of someone actually using a pager whose executable is called builtin seems low and that conflict could easily be fixed by the user. 😉

One extra step that is often easily missed is updating the completions and the manpage as well. (Completions I personally don't understand so well especially for shells I don't use... So feel free to skip it if uninteresting or too challenging and someone else can always help out later. Perfection is the enemy of progress after all 😉)

  • Should we use Minus's built-in line numbering, or bat's?

I suspect that here it might get a bit trickier. Ideally I think we would want to let minus handle it, because I have seen feature requests for being able to toggle the line numbering on and off while viewing a file. But, I can predict that maintaining separate code paths for external and builtin pagers isn't going to be fun... There's lots that tie into it like line wrapping, chopping long lines, having areas with no line numbers when bat is outputting a header for the next file etc and having numbering reset.

So probably let's keep things simple to start with and keep bat's line numbering for now.

academician commented 2 months ago

@CosmicHorrorDev Thanks! I had noticed that, and tried a couple other tools but couldn't find a better solution quickly which is why I specified what I did :P After using yours, the dependency different becomes 9 (143 vs 152), which seems much more manageable. For extra credit, here are the new dependencies when I diff the list:

crossbeam-channel v0.5.13
crossbeam-utils v0.8.20
crossterm v0.27.0
minus v5.6.1
mio v0.8.11
signal-hook v0.3.17
signal-hook-mio v0.2.4
signal-hook-registry v1.4.2
textwrap v0.16.1

@keith-hall Thanks for the feedback! I'll add completions and man page to the list, shouldn't be too difficult. I'm a little surprised clap doesn't generate the man page for you, but it looks like that's an extension.

Regarding the line numbers, I rather agree, and I'm going to at least leave it using bat's line numbers for now. I think making that work might make the code path a bit tortured for questionable benefit. Can always add it in later if we need to.

einfachIrgendwer0815 commented 2 months ago

@academician Personally, I think it is a great idea to have bat bring its pager with it. There is just one concern I would like to note.

I agree that --pager=builtin is a matching name. However, I am currently working on an implementation for #1053 (custom builtin pager for bat). I think that that will give more flexibility (also more maintenance work though). The point is, I currently chose to use --pager=bat for that. Even if a custom pager is merged in the future, I think having a "simple" paging alternative (such as using minus) at hand would be great for the user. The problem I am seeing is that potentially having builtin and bat, but them meaning different things, would be confusing users.

So, I would suggest to treat builtin as being a namespace instead of a specific name. So, minus would be called builtin-minus instead of just builtin. This would leave space open for future additions (whatever those might look like).

academician commented 2 months ago

@einfachIrgendwer0815 Ah! I was completely unaware of your parallel efforts. If we're planning on having multiple built-in pager options, then that definitely is a wrinkle.

I can make that change for now, renaming "builtin" to "builtin-minus". I do think eventually we might want one of the pager options to be the "default", because I doubt end users care that much about the implementation of the pager if they're not bringing their own. But I'm fine with deferring that decision.

I'll make some of the revisions we've discussed this week and cut a PR as soon as I can make the time. Thanks, all!