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

Disable preview when fuzzy-finding #163

Merged
merged 1 commit into from Oct 11, 2019

Conversation

@OJFord
Copy link
Contributor

OJFord commented Jul 20, 2019

The user may have global settings that enable the preview pane in fzf.

Whatever the preview command is set as, it probably doesn't render
anything meaningful for kubens - I can't think what would be.

For kubectx, the context yaml itself would maybe be helpful, but it
likely contains secrets, so I don't personally think I'd find it useful
enough to get into.

This commit thus disables the preview, so that if the user did have it
enabled, there's now no pane where there would previously have probably
been an error, such as:

[bat error]: '<namespace>': No such file or directory (os error 2)
@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Jul 20, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no label Jul 20, 2019
@OJFord

This comment has been minimized.

Copy link
Contributor Author

OJFord commented Jul 20, 2019

I signed it!

@googlebot

This comment has been minimized.

Copy link
Collaborator

googlebot commented Jul 20, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Jul 20, 2019
@OJFord

This comment has been minimized.

Copy link
Contributor Author

OJFord commented Sep 22, 2019

Sorry to ping @ahmetb, do you agree with the principle here? WIll fix conflicts if you're happy. 🙂

@ahmetb

This comment has been minimized.

Copy link
Owner

ahmetb commented Oct 5, 2019

Sorry for the silence here. I got busy lately. I am still trying to understand this.

What is the preview pane? Can you show a screenshot?
Why do people have it globally enabled? Is this a significant % of the users?

@OJFord

This comment has been minimized.

Copy link
Contributor Author

OJFord commented Oct 8, 2019

@ahmetb No problem. This is a screenshot of cd /etc ; fzf:
fzf with preview pane

On the right hand side you can see the 'preview pane' that I have globally configured (via --preview='bat <options>' (bat is a bit like cat but has syntax highlighting and such) in $FZF_DEFAULT_OPTS) to preview file contents.

By default fzf is operating over files, so it makes sense that default options assume files, but for kubectx & kubens this is not the case. Maybe you might want to preview the k8s resources in each namespace - something like --preview='kubectl --namespace="{}" get all' - but you certainly wouldn't have that in $FZF_DEFAULT_OPTS, because it wouldn't make sense in any other place you used fzf.

I have no idea what percentage of users might have it configured, but it seems reasonably likely for any that use fzf outside of kubectx/kubens (which is everyone once they discover how great it is?!) and for those kubens currently looks like:
kubens with nonsensical fzf preview
(or if not a bat error an error or nonsensical output from whatever they're using for --preview)

vs. with this patch: (edit - ignore the change in highlight colour, that's just because I didn't actually switch to the patched version for the screenshot, I piped to fzf --no-preview.)
kubens with disabled fzf preview

@ahmetb

This comment has been minimized.

Copy link
Owner

ahmetb commented Oct 10, 2019

I don't think this is an option most fzf users know about or use actively.
That said, since the change is minimal and makes it "defensive", we can add it. Feel free to resolve the conflicts.

The user may have global settings that enable the preview pane in fzf.

Whatever the preview command is set as, it probably doesn't render
anything meaningful for kubens - I can't think what would be.

For kubectx, the context yaml itself would _maybe_ be helpful, but it
likely contains secrets, so I don't personally think I'd find it useful
enough to get into.

This commit thus disables the preview, so that if the user did have it
enabled, there's now no pane where there would previously have probably
been an error, such as:

    [bat error]: '<namespace>': No such file or directory (os error 2)
@OJFord OJFord force-pushed the OJFord:no-preview branch from 077f0be to d269d62 Oct 11, 2019
@OJFord

This comment has been minimized.

Copy link
Contributor Author

OJFord commented Oct 11, 2019

Resolved just by dropping 06374a3, since same change applied to master, so no real change.

@ahmetb

This comment has been minimized.

Copy link
Owner

ahmetb commented Oct 11, 2019

Thanks. 👍

@ahmetb ahmetb merged commit 00a1e12 into ahmetb:master Oct 11, 2019
2 checks passed
2 checks passed
cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@OJFord OJFord deleted the OJFord:no-preview branch Oct 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.