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

Make FzF options configurable #154

Merged
merged 10 commits into from
Apr 24, 2022
Merged

Conversation

Nelyah
Copy link
Contributor

@Nelyah Nelyah commented Jan 14, 2022

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

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.

Nelyah and others added 2 commits January 14, 2022 19:51
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
Copy link
Member

@mickael-menu mickael-menu left a 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 or FZF_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 by fzf. I tried FZF_DEFAULT_OPTS="--border" zk list -i from the main branch and it works.

  • I would prefer using fzf-options / FzfOptions instead of fzf-additional-args, to make it closer to FZF_DEFAULT_OPTS.

  • Some of the zk hard-coded options could actually be part of the default value for fzf-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.

Thanks again!

internal/adapter/fzf/fzf.go Outdated Show resolved Hide resolved
@Nelyah
Copy link
Contributor Author

Nelyah commented Jan 21, 2022

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 :)

@mickael-menu
Copy link
Member

@Nelyah No problem, I can relate ;)

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity.

@github-actions github-actions bot added the stale No recent activity label Feb 23, 2022
@mickael-menu mickael-menu added enhancement New feature or request and removed stale No recent activity labels Feb 23, 2022
@megalithic
Copy link

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?

@mickael-menu
Copy link
Member

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.

@Nelyah
Copy link
Contributor Author

Nelyah commented Mar 20, 2022

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.

@github-actions github-actions bot added the stale No recent activity label Apr 20, 2022
@zk-org zk-org deleted a comment from github-actions bot Apr 20, 2022
@mickael-menu mickael-menu removed the stale No recent activity label Apr 20, 2022
@megalithic
Copy link

whew, glad it's not officially stale anymore. 🙏

Copy link
Member

@mickael-menu mickael-menu left a 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.

@mickael-menu mickael-menu merged commit 94e8a0d into zk-org:main Apr 24, 2022
@mickael-menu mickael-menu mentioned this pull request Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants