-
Notifications
You must be signed in to change notification settings - Fork 118
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
Make FzF options configurable #154
Conversation
This adds the ability to configure the FzF options used by the NewNoteFilter. Here are the sources for the FzF options, in order of precedence (first precedes the second): 1. tool.fzf-additional-args in the note directory config file 2. Default arguments set by zk in `internal/adapter/fzf/fzf.go` 3. Environment variable FZF_DEFAULT_OPTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for tackling this, I appreciate it!
A few comments:
-
I found an issue while testing: if any of
fzf-additional-args
orFZF_DEFAULT_OPTS
are not set or empty, it will error out with:$ zk list -i unknown option: zk: error: failed to filter interactively the output with fzf, try again without --interactive or make sure you have a working fzf installation: exit status 2
I think using
shellquote
like described in my other comment should solve the problem. -
I don't think you need to add
FZF_DEFAULT_OPTS
manually, it should be handled automatically byfzf
. I triedFZF_DEFAULT_OPTS="--border" zk list -i
from themain
branch and it works. -
I would prefer using
fzf-options
/FzfOptions
instead offzf-additional-args
, to make it closer toFZF_DEFAULT_OPTS
. -
Some of the
zk
hard-coded options could actually be part of the default value forfzf-options
, when unset. Some can't be unset by additional arguments (like--no-hscroll
) and most of them are tuned to my taste. I suggest:- Keeping only the essential hard-coded options:
--delimiter
and--ansi
. - Adding to the default value of
fzf-options
:--tiebreak begin --exact --tabstop 4 --height 100% --layout reverse --no-hscroll --color hl:-1,hl+:-1 --preview-window wrap
- Look for
LineTemplate.OrString(defaultLineTemplate).String()
for an example of handling a default value for an option.
- Look for
- Keeping only the essential hard-coded options:
Thanks again!
Thank you very much for the detailed @mickael-menu! I'm sorry I haven't had much time these past few weeks to really get to this, but I haven't forgotten about it :) |
@Nelyah No problem, I can relate ;) |
This pull request has been automatically marked as stale because it has not had recent activity. |
Hello friends; just checking in to see if this still has some traction/working towards it. Hope you are all well during these crazy times. Are there work-arounds for the fzf key binding overrides in the meantime? |
Hi @megalithic, I intend to finish the work eventually but I've been really busy the past few weeks so I'm not sure when I'll find the time yet. |
Hi, apologies for letting this without update for so long. If you do have the time now to get to it, feel free to. Otherwise I will also try to finish it when I get the time. |
whew, glad it's not officially |
Remove `FZF_DEFAULT_OPTS` handling
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @Nelyah for kickstarting this PR!
@megalithic I changed the default key binding to create a new note to Ctrl-E to avoid conflicts. But you can choose a different one with:
[tool]
fzf-bind-new = "Ctrl-C"
I plan to make a new release very soon.
This adds the ability to configure the FzF options used by the
NewNoteFilter.
Here are the sources for the FzF options, in order of precedence (first
precedes the second):
internal/adapter/fzf/fzf.go
This should solve #137
Let me know if I can improve on it. As I mentioned in the issue those are my first lines of Go so it's still a bit of a struggle.