More minimal bat previews, minor improvements to fzplug

[Kabouik]

No description provided.

[Kabouik]

Should solve this.

[Kabouik]

While we're at it, I'm wondering if we should set --paging=always instead of --paging=never? I did not follow discussions when those preview plugins were written, is there any performance impact for paging full files instead of just showing the beginning? This would allow scrolling the bat preview panes and possibly skipping long comment sections on some files without opening them.

[luukvbaal]

I would vote against showing the header for bat previews. Which file is previewed is obvious from the selection in nnn is it not?

[Kabouik]

Totally, I agree. I would just leave it on fzplug because (1) this plugin is not meant to be shown permanently and (2) it is built around calling scripts from multiple locations but doesn't show parent folders in the fzf list to use less space, so showing their full path above the preview pane helps discriminating them.

I made the change but am waiting for opinions on --paging=never vs. --paging=always before pushing.

[luukvbaal]

--style=plain,numbers could be replaced -n. However I'm curious how others feel about just using -p(--plain) which is what I use myself. I'm not really interested in line numbers in the preview pane usually.

You should be able to scroll the bat output (or any other command) in the preview pane regardless of the --paging=never. fifo_pager() opens the command output from preview_file() in $PAGER so if you are able to scroll in your pager you should be able to scroll in the preview pane.

As far as I'm concerned the bat command could be replaced with bat --terminal-width="$(tput cols)" -fpp.

From the manpage:

-f, --force-colorization
              Alias for '--decorations=always --color=always'

-p, --plain
              Only  show  plain  style,  no  decorations.  This  is  an  alias  for
              '--style=plain'.  When  '-p'  is used twice ('-pp'), it also disables
              automatic paging (alias for '--style=plain --pager=never').

[Kabouik]

Nice. I usually keep the human-readable flags in my scripts even if they're longer because those are most often commands I'm not used to and I prefer my scripts to be easy to edit without documentation, but this may not make sense here (unless we expect users to often customize their preview plugins without good knowledge of the associated utilities?). I do not dislike numbers, they're sometimes useful to me, particularly in combination with scrolling of the preview (thanks for clarifying that too), but I might be the only one.

So that would be bat --terminal-width="$(tput cols)" -fpp if we scrap numbers, or bat --terminal-width="$(tput cols)" -fn --paging=never if we keep them I think.

[luukvbaal]

Yeah using human readable flags has merit in scripts I guess, I don't mind either way.

On second thought I'm not so sure about passing the --style flag though. This would overwrite the BAT_STYLE env variable for people who have that exported in their profile.

[Kabouik]

Good point; I don't have a BAT_STYLE set. Let's use short aliases if they are overridden by BAT_STYLE when it is set. Just waiting for additional input on numbers until tomorrow (I'm off to bed). I'd lean towards keeping them because I use them, but this is very personal and perhaps I am the only one.

[luukvbaal]

The short aliases overwrite the BAT_STYLE as well. Perhaps it's worth it though as I do agree the grid looks pretty awful. I would prefer having a more plain view as the default.

We would have to come to a consensus on the style used (header/numbers or not) and anyone who has different preference would have to edit the script which I guess is fine. Paging @leovilok to see if they want to chime in.

[jarun]

@leovilok please merge this when you guys are done.

[jarun]

@luukvbaal please update the ToDo list after this is merged so users have an idea of what went it from the release notes. (I use the ToDo list of generate the release notes).

[luukvbaal]

@luukvbaal please update the ToDo list after this is merged so users have an idea of what went it from the release notes. (I use the ToDo list of generate the release notes).

Sure. Do you have an opinion on the issue btw? Either we use a more plain default(in which case we would have to decide whether or not to include line numbers) with the concession that BAT_STYLE won't work anymore. Or we suggest users to export BAT_STYLE for a more minimal preview.

[luukvbaal]

Just realized we can also test if BAT_STYLE is exported and if it is not pass the --style flag... Should be the best option.

[Kabouik]

How about adding an example BAT_STYLE in the plugins, uncommented by default, so users just have to comment it in the plugins if they already have one in their rc file? There are already multiple variables set just for the plugins. This would make the plugins more independent but still easy to adapt to a pre-existing env variable.

[Edit] Or that.

[luukvbaal]

Yeah just use `--style="${BAT_STYLE:-numbers}". Should be fine to merge then imo.

[luukvbaal]

For clarity the full bat command should then be:
bat --terminal-width="$(tput cols)" --force-colorization --style="${BAT_STYLE:-numbers}" "$@" 2>/dev/null &

[Kabouik]

I am trying to use case to cover both situations but this seems to always use the first case:

            if exists bat; then
                case "$BAT_STYLE" in
                    "")
                        bat --terminal-width="$(tput cols)" --color=always --style=plain --paging=never "$@" 2>/dev/null &
                        ;;
                    *)
                        bat --terminal-width="$(tput cols)" --color=always --style="${BAT_STYLE:-numbers}" --paging=never "$@" 2>/dev/null &
                        ;;
                esac
            else

What am I missing? I thought case was looking for exact matches so the first case should be used only when $BAT_STYLE is not set. I'm sorry if this is an easy one, I'm learning.

Note that there is no --force-colorization on my end (bat 0.15.4) which is why I used --color=always on my end. I reintroduced paging=never because it defaults to auto, but I'm happy either way.

[luukvbaal]

You don't need the case statement --style="${BAT_STYLE:-numbers}" is sufficient. Uses BAT_STYLE if it is found inv env and substitutes numbers otherwise.

[Kabouik]

Oh, I was overcomplicating then. Thanks.

[luukvbaal]

Yeah if --force-colorization is not available on older versions of bat use bat --terminal-width="$(tput cols)" --decorations=always --color=always --paging=never --style="${BAT_STYLE:-numbers}" "$@" 2>/dev/null & instead.

[jarun]

@Kabouik general feedback, please to NOT write comments in caps as you did in some of the plugins.

[Kabouik]

Sure, I'll edit fzplug and unmount-parent accordingly in this PR.

[jarun]

No, I am doing it. Please mind the spaces too.

[Kabouik]

@luukvbaal with this command, $BAT_STYLE set in env still seems to be overridden by numbers. I tried to force header,grid in my $BAT_STYLE but I only see plain with numbers.

[luukvbaal]

You are using kitty right? For kitty BAT_STYLE would have to be added to the --env list.

i.e. add it here

kitty @ launch --no-response --title "nnn preview" --keep-focus \
            --cwd "$PWD" --env "PATH=$PATH" --env "NNN_FIFO=$NNN_FIFO" \
            --env "PREVIEW_MODE=1" --env "PAGER=$PAGER" --env "TMPDIR=$TMPDIR" \
            --env "USE_SCOPE=$USE_SCOPE" --env "SPLIT=$SPLIT" --env "TERMINAL=$TERMINAL"\
            --env "USE_PISTOL=$USE_PISTOL" --env "PAGERPID=$PAGERPID"  \
            --env "IMGPID=$IMGPID" --env "FIFO_UEBERZUG=$FIFO_UEBERZUG" \
            --env "ICONLOOKUP=$ICONLOOKUP" --env "NNN_PREVIEWDIR=$NNN_PREVIEWDIR" \
            --env "NNN_PREVIEWWIDTH=$NNN_PREVIEWWIDTH" --env "NNN_PREVIEWHEIGHT=$NNN_PREVIEWHEIGHT" \
            --env "CURSEL=$CURSEL" --location "${SPLIT}split" "$0" "$1" >/dev/null

in startpreview()

[jarun]

@Kabouik please leave a note when done.

[Kabouik]

Ah. Yes I was using kitty. Just started using it days ago, I didn't know about --env, damn.

Sure @jarun. I'm sorry this takes longer than expected, it was initially a very simple change so I felt bravehearted enough for the PR (and it admittedly still is simple, but now it challenges my rather limited scripting knowledge :<).

[luukvbaal]

Does it work with kitty when passing --env BAT_STYLE="$BAT_STYLE"?

[Kabouik]

That seems to work only if my $BAT_STYLE contains only one word and no comma, else it'll break and nothing will be previewed. Still testing other situations.

Seems to work if BAT_STYLE is exported in env, but not if it is not.

[luukvbaal]

For me it also works in kitty without passing BAT_STYLE in --env but I'm not sure how the kitty pane environment works so do add it if it seems necessary.

[luukvbaal]

Seems to work if BAT_STYLE is exported in env, but not if it is not.

In that case, and if it indeed seems necessary to pass BAT_STYLE try using --env BAT_STYLE="${BAT_STYLE:-numbers} although for me it seems to honor BAT_STYLE in kitty also without passing it to --env.

Edit: I'm testing with BAT_STYLE exported in my profile, that might be the difference.

[Kabouik]

I exported BAT_STYLE in my rc file too just to be sure and it works with --env "BAT_STYLE=$BAT_STYLE" in the plugin, but still face an issue when no BAT_STYLE is set. See here:

preview-tui.mp4

Relevant lines in the script:

        kitty @ launch --no-response --title "nnn preview" --keep-focus \
            --cwd "$PWD" --env "PATH=$PATH" --env "NNN_FIFO=$NNN_FIFO" \
            --env "PREVIEW_MODE=1" --env "PAGER=$PAGER" --env "TMPDIR=$TMPDIR" \
            --env "USE_SCOPE=$USE_SCOPE" --env "SPLIT=$SPLIT" --env "TERMINAL=$TERMINAL"\
            --env "USE_PISTOL=$USE_PISTOL" --env "BAT_STYLE=$BAT_STYLE" --env "PAGERPID=$PAGERPID" \
            --env "GIFPID=$GIFPID" --env "FIFO_UEBERZUG=$FIFO_UEBERZUG" \
            --env "CURSEL=$CURSEL" --location "${SPLIT}split" "$0" "$1" >/dev/null

            if exists bat; then
                bat --terminal-width="$(tput cols)" --decorations=always --color=always --paging=never --style="${BAT_STYLE:-numbers}" "$@" 2>/dev/null &
            else

It seems this patch fixes it:

-            --env "USE_PISTOL=$USE_PISTOL" --env "BAT_STYLE=$BAT_STYLE" --env "PAGERPID=$PAGERPID" \
+            --env "USE_PISTOL=$USE_PISTOL" --env "BAT_STYLE=${BAT_STYLE:-numbers}" --env "PAGERPID=$PAGERPID" \

[luukvbaal]

Yeah that's what I suggested in my previous comment.

[Kabouik]

Phew. Tested in kitty + kitty preview and xterm + tmux preview, with and without env BAT_STYLE.

I had to add -e "BAT_STYLE=${BAT_STYLE:-numbers}" in the tmux section of preview-tui* too, else pre-existing BAT_STYLE would not be honored in tmux preview panes.

[luukvbaal]

Thanks, looks good me for merging.

I had to add -e "BAT_STYLE=${BAT_STYLE:-numbers}" in the tmux section of preview-tui* too, else pre-existing BAT_STYLE would not be honored in tmux preview panes.

Again, I have BAT_STYLE exported in my .(z)profile (which should be the common use case imo) so for me this isn't true. It doesn't hurt to pass it though so thanks for testing and adding it.

[jarun]

@luukvbaal I am merging it. Please test and confirm things are good.

@Kabouik Thank you!

[jarun]

@luukvbaal please update the ToDo list when you have the time.

[luukvbaal]

Sure, done. See if it passes your style for the release notes.

[jarun]

Thanks @luukvbaal!

[luukvbaal]

@luukvbaal please update the ToDo list when you have the time.

Did you remove the ToDo list item I added or did I forget to submit it? I would have sworn I submitted but I don't see it in the edit history lol.

[jarun]

Did you remove the ToDo list item I added

I didn't remove anything.

[luukvbaal]

Guess I didn't submit the edit then lol, added it now