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

feat: add environment variables to change fzf keys #87

Merged
merged 8 commits into from
Jul 16, 2024

Conversation

benelan
Copy link
Contributor

@benelan benelan commented Jul 15, 2024

Add GH_NOTIFY_<ACTION>_KEY environment variables so the fzf keys can be changed. The extension remaps some of fzf's defaults (e.g., ctrl-p) and my muscle memory was making it hard to use. Let me know if you'd like any name or code changes, thanks!

@LangLangBart
Copy link
Collaborator

Thanks for the PR.

I would suggest changing the assignment to use the following colon (:) syntax1:

: "${ENV_VAR:=default}"

instead of:

ENV_VAR="${ENV_VAR:=default}"

This change avoids writing the environment variable name twice during the assignment. Exporting the variable is not needed, as it will not be used in any child processes.

# Customize the fzf keys using environment variables
: "${GH_NOTIFY_MARK_ALL_READ_KEY:=ctrl-a}"
: "${GH_NOTIFY_OPEN_BROWSER_KEY:=ctrl-b}"
: "${GH_NOTIFY_VIEW_DIFF_KEY:=ctrl-d}"
: "${GH_NOTIFY_VIEW_PATCH_KEY:=ctrl-p}"
: "${GH_NOTIFY_RELOAD_KEY:=ctrl-r}"
: "${GH_NOTIFY_MARK_READ_KEY:=ctrl-t}"
: "${GH_NOTIFY_COMMENT_KEY:=ctrl-x}"
: "${GH_NOTIFY_RESIZE_PREVIEW_KEY:=btab}"
: "${GH_NOTIFY_VIEW_KEY:=enter}"
: "${GH_NOTIFY_TOGGLE_PREVIEW_KEY:=tab}"
: "${GH_NOTIFY_TOGGLE_HELP_KEY:=?}"

It would probably be good to include a concise example in the README for reassigning a key:

To avoid conflicting with user keybindings, redefine them, e.g., change `ctrl-p` to `ctrl-u`.

```sh
GH_NOTIFY_VIEW_PATCH_KEY="ctrl-u" gh notify
```

**NOTE:** The assigned key must be a valid key listed under `AVAILABLE KEYS` in the `fzf` man page.

Footnotes

  1. Bash Reference Manual - Shell Parameter Expansion

readme.md Outdated Show resolved Hide resolved
@benelan
Copy link
Contributor Author

benelan commented Jul 16, 2024

Thanks for the review! I made the suggested changes, let me know if anything else is needed on my end.

@LangLangBart
Copy link
Collaborator

Thanks, GitHub seems to be having some trouble executing the workflow run.

https://github.com/meiji163/gh-notify/actions/runs/9952278435

I will push a small change to see if the issue is specific to a user or happens in general.

@benelan
Copy link
Contributor Author

benelan commented Jul 16, 2024

Not sure if it's related, but in the past I've resolved GitHub Action problems for pull requests from forks using the pull_request_target event instead.

@LangLangBart
Copy link
Collaborator

pull_request_target

Can you make the change and push it to see if this might resolve the issue?

@benelan
Copy link
Contributor Author

benelan commented Jul 16, 2024

You may need to manually run the workflows since it is my first time contributing

@LangLangBart
Copy link
Collaborator

LangLangBart commented Jul 16, 2024

You may need to manually run the workflows since it is my first time contributing

I approved that earlier and it should run, maybe one has to apply it to the main branch before taking effect. I will make commit to main branch with pull_request_target.

* upstream:
  ci: enable pull_request_target (meiji163#88)
@benelan
Copy link
Contributor Author

benelan commented Jul 16, 2024

looks like that fixed it!

@LangLangBart
Copy link
Collaborator

looks like that fixed it!

Thanks for helping out. I will use it a bit more and merge it in one hour.

@LangLangBart LangLangBart merged commit 3722449 into meiji163:main Jul 16, 2024
1 check passed
@LangLangBart
Copy link
Collaborator

@benelan Thanks for the PR. You were right to export it initially. I forgot about the help page (?) inside fzf.

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