Skip to content

Conversation

@vegerot
Copy link
Contributor

@vegerot vegerot commented May 21, 2025

I think this is a bug fix instead of a new feature. The reason for this is
that the current behavior overrides user choice of FZF_PREVIEW_COMMAND. No
matter what preview command they provide, we never run it on binary files.
However, if the user supplies no preview command, then we'll run it through
bat which produces an error. This sounds like a mistake.

This commit makes the behavior when no preview command is supplied the same as
the behavior when a preview command is given.

Before:
image

After:
image

@junegunn
Copy link
Owner

Do you prefer the "After:" output with mime types?

I didn't pay much attention to this because I was fine with the error message from bat, or maybe I even preferred it because it clearly showed the [bat warning] with color highlighting.

@junegunn
Copy link
Owner

We could maybe improve the output for binary files

[warning] foobar300 is a binary file
- info
- more info

I think this is a bug fix instead of a new feature.  The reason for this is
that the current behavior _overrides user choice_ of `FZF_PREVIEW_COMMAND`. No
matter what preview command they provide, we never run it on binary files.
However, if the user supplies _no_ preview command, then we'll run it through
`bat` which produces an error.  This sounds like a mistake.

This commit makes the behavior when no preview command is supplied the same as
the behavior when a preview command is given.

Before:
<img width="1251" alt="image" src="https://github.com/user-attachments/assets/d5f66828-5a17-439d-bd62-b670e6e9e2e9" />

After:
<img width="1232" alt="image" src="https://github.com/user-attachments/assets/dd1d811d-fdc0-4023-8bfa-e97e5b9fdca5" />
@vegerot
Copy link
Contributor Author

vegerot commented May 24, 2025

I didn't pay much attention to this because I was fine with the error message from bat,

Good point. Either way the current behavior is inconsistent. I agree with the suggestion in your second message.

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.

2 participants