Allow users to set keys to be bind, before sourcing the script, using environment variables #123

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

Jump to bottom

Open

ericbn wants to merge 1 commit into zsh-users:master

from ericbn:bind_keys

Open

Allow users to set keys to be bind, before sourcing the script, using environment variables #123

ericbn wants to merge 1 commit into zsh-users:master from ericbn:bind_keys

Conversation

ericbn

Copy link

Contributor

@ericbn ericbn commented Aug 3, 2021

This will save users from some common boilerplate code in their own .zshrc file.

Fixes #107

ericbn

ericbn commented

Aug 3, 2021

View reviewed changes

README.md Outdated

* `HISTORY_SUBSTRING_SEARCH_VICMD_DOWN_KEYS` is a global array that defines the

VI mode keys to be bind to the `history-substring-search-down` command.

Its default value is `('j')`, which is the code for the `j` key.

Copy link

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's lots of repeating words in the above paragraphs, but I tried to keep the bullets layout. Maybe this could be reformatted as a table?

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I think the repetition is fine. If you really want to reformat this as a table, please make sure to do the same for all other configuration variables too.

Copy link

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the repetition here then, as you're fine with it. I can try a table layout later in a separate PR...

ericbn

ericbn commented

Aug 3, 2021

View reviewed changes

zsh-history-substring-search.zsh

if (( ! $+HISTORY_SUBSTRING_SEARCH_PREFIXED )); then

typeset -g HISTORY_SUBSTRING_SEARCH_PREFIXED=''

Copy link

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also fixing the issue with WARN_CREATE_GLOBAL, as I commented here: #108 (comment)

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #119 seems simpler than this implementation. I'm leaning toward merging it instead.

Copy link

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, that implementation is simpler. Let me update the PR and use that here.

Copy link

Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The typeset -g VAR="${VAR:=value}" construct does not seem correct to me, as is does the assignment twice: at ${VAR:=value} and also at typeset -g VAR="value", so I changed it to what seems more correct.

sunaku

sunaku reviewed

Aug 5, 2021

View reviewed changes

README.md Outdated

receive globally unique search results only once, then use this

configuration variable, or use `setopt HIST_IGNORE_ALL_DUPS`.

The following variables must be overridden before having loaded this script

Copy link

Member

@sunaku sunaku Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hesitant about the "must" requirement here: it's helpful to provide these key arrays to simplify the new-user experience but currently, we don't require users to bind our widgets. They can choose to simply load our plugin and never actually use its functionality. This kind of basic "null" use-case should be preserved, I feel.

Copy link

Contributor Author

@ericbn ericbn Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so all these properties will be empty by default, in order to preserve the current "null" use-case. Let me change that and update the README accordingly.

sunaku

sunaku requested changes

Aug 5, 2021

View reviewed changes

Copy link

Member

@sunaku sunaku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. Please see my feedback in the code review.

@ericbn ericbn force-pushed the bind_keys branch from d8e51d8 to 8f9c924 Compare

August 5, 2021 17:54

@ericbn

Copy link

Contributor Author

ericbn commented Aug 5, 2021

@sunaku, PR updated.

@ericbn ericbn force-pushed the bind_keys branch from 8f9c924 to d8f7532 Compare

August 11, 2021 01:32

@ericbn ericbn requested a review from sunaku

August 13, 2021 21:54

@ericbn ericbn changed the title ~~(削除) Bind keys and allow users to override the default ones (削除ここまで)~~ (追記) Allow users to set keys to be bind, before sourcing the script, using environment variables (追記ここまで)

Aug 23, 2021

@ericbn


 Bind keys and allow users to override the default ones

8d695d9

This will save users from some common boilerplate code in their own
.zshrc file.
Fixes zsh-users#107

@ericbn ericbn force-pushed the bind_keys branch from d8f7532 to 8d695d9 Compare

August 31, 2021 03:56

This was referenced Nov 7, 2021

Consider setting up some default key bindings #107

Open

feature: Pre-/post module init scripts zimfw/zimfw#364

Closed

@lnicola

Copy link

lnicola commented Nov 7, 2021

Nit: CONTROL is such a weird way to spell Ctrl.

@ericbn

Copy link

Contributor Author

ericbn commented Nov 7, 2021

It was being spelled as "Control" and "C" and also "^" before. I tried to unify it to a common format ("CONTROL"), as we have other keys in all caps too (e.g. "UP", "DOWN") in the same document.

Fun fact: my keycaps actually spell it as "Control" :- )
keycaps

@sunaku, what do you prefer?

Labels

None yet

3 participants

@ericbn @lnicola @sunaku

Allow users to set keys to be bind, before sourcing the script, using environment variables #123

Are you sure you want to change the base?

Allow users to set keys to be bind, before sourcing the script, using environment variables #123

Uh oh!

Conversation

@ericbn ericbn commented Aug 3, 2021

Uh oh!

@ericbn ericbn Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

@sunaku sunaku Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ericbn ericbn Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ericbn ericbn Aug 3, 2021

Choose a reason for hiding this comment

Uh oh!

@sunaku sunaku Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ericbn ericbn Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ericbn ericbn Aug 31, 2021

Choose a reason for hiding this comment

Uh oh!

@sunaku sunaku Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@ericbn ericbn Aug 5, 2021

Choose a reason for hiding this comment

Uh oh!

@sunaku sunaku left a comment

Choose a reason for hiding this comment

Uh oh!

ericbn commented Aug 5, 2021

Uh oh!

lnicola commented Nov 7, 2021

Uh oh!

ericbn commented Nov 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants