Skip to content

feat(search): make cursor style configurable#1595

Merged
ellie merged 2 commits intoatuinsh:mainfrom
akinomyoga:keymap_cursor
Jan 22, 2024
Merged

feat(search): make cursor style configurable#1595
ellie merged 2 commits intoatuinsh:mainfrom
akinomyoga:keymap_cursor

Conversation

@akinomyoga
Copy link
Contributor

@akinomyoga akinomyoga commented Jan 21, 2024

The vim mode introduced in #1553 changes the terminal's cursor style on a mode change, but the current implementation has the following issues.

  • The terminal's cursor style set by the Atuin search remains even after Atuin exits. This causes an inconsistency with the shell's settings for the cursor style.
  • Also, the cursor style for each keymap mode is currently hardcoded in the source code, which is not necessarily consistent with the user's cursor-style settings in the shell.
  • Since the current implementation does not set the cursor style for the initial keymap mode but only sets the cursor style when the keymap mode is changed, it also causes inconsistency in the cursor style and the actual keymap when the shell's keymap and Atuin's initial keymap mode are different.

This patch solves those issues by introducing an opt-in configuration variable keymap_cursor. By default, the vim mode does not change the cursor style because there is no way to determine the cursor style consistent with the shell settings automatically. We enable the feature only when the user specifies the preferred cursor style in each mode in their config. Also, the cursor style is set on the startup of the Atuin search (based on the initial keymap mode) and is reset on the termination of the Atuin search (based on the shell's keymap mode that started the Atuin search).

edit: let me ping @YummyOreo, who introduced the feature

The vim mode of the interactive Atuin search changes the cursor style
on a mode change, but the current implementation has the following
issues.

* The terminal's cursor style set by the Atuin search remains after
  Atuin exits.  This causes an inconsistency with the shell's setting
  for the cursor style.

* Also, the cursor style for each keymap mode is currently hardcoded
  in the source code, which is not necessarily consistent with the
  user's cursor-style setting in the shell.

* Since the current implementation does not set the cursor style for
  the initial keymap mode but only sets the cursor style when the
  keymap mode is changed, it also causes inconsistency in the cursor
  style and the actual keymap when the shell's keymap and Atuin's
  initial keymap mode are different.

This patch solves those issues by introducing an opt-in configuration
variable `keymap_cursor`.  By default, the vim mode does not change
the cursor style because there is no way to automatically determine
the cursor style consistent with the shell settings.  We enable the
feature only when the user specifies the preferred cursor style in
each mode in their config.  Also, the cursor style is set on the
startup of the Atuin search (based on the initial keymap mode) and is
reset on the termination of the Atuin search (based on the shell's
keymap mode that started the Atuin search).
@ellie ellie enabled auto-merge (squash) January 22, 2024 10:55
@ellie ellie merged commit e484a68 into atuinsh:main Jan 22, 2024
@akinomyoga akinomyoga deleted the keymap_cursor branch January 22, 2024 10:57
@akinomyoga
Copy link
Contributor Author

Thank you!

@akinomyoga
Copy link
Contributor Author

@ellie By the way, do you have a plan soon to do the refactoring mentioned in #1553 (review) and #1570 (comment) by yourself? I'd like to refactor it a bit. Maybe, these should be ultimately solved by a keymap table like HashMap<KeyCode, someFunctionType>, but I'd like to sort it a bit and factorize common parts before that.

@ellie
Copy link
Member

ellie commented Jan 22, 2024

I was originally planning on doing so, but if you'd like to take a look that would be fantastic!

My plan was

  1. Bring out a map of KeyCode -> Action. Actions would be InsertKey(char), MoveTo, Delete... etc.
  2. Refactor the UI so that it passes the keycode to (1), then handles the resulting action. Probs break it down to vim/emacs/etc
  3. Once this is there, make it configurable like you say. It would be good to be able to express "ctrl-p: moveup(1)" or similar.

But feel free to do this however you think is best. Probably worth opening something to track the work

@akinomyoga
Copy link
Contributor Author

Thank you for your thoughts! I was afraid of some conflicts. I'll submit a PR, but that PR doesn't actually change the current structure. I just sort and factorize some match branches.

@akinomyoga
Copy link
Contributor Author

I created a PR at #1606.

ellie pushed a commit to atuinsh/docs that referenced this pull request Jan 22, 2024
This describes config `keymap_cursor` added in
atuinsh/atuin#1595.

Let me also add typo fixes to my previous PR #8 in commit a823801.
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