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

Add "zg" and "zG" bindings #1491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

condy0919
Copy link
Contributor

@condy0919 condy0919 commented Jul 9, 2021

According to https://vimhelp.org/spell.txt.html, zg would

Add word under the cursor as a good word to the first name in 'spellfile'. A
count may precede the command to indicate the entry in 'spellfile' to be used.
A count of two uses the second entry.

In Visual mode the selected characters are added as a word (including white
space!). When the cursor is on text that is marked as badly spelled then the
marked text is used. Otherwise the word under the cursor, separated by
non-word characters, is used.

If the word is explicitly marked as bad word in another spell file the result
is unpredictable.

zG would

Like "zg" but add the word to the internal word list internal-wordlist.

ispell has no concept of internal-wordlist, but it provides a local words instead.

@condy0919
Copy link
Contributor Author

Why it failed on Emacs 24.5 only?

@condy0919 condy0919 changed the title Add evil-ispell-mark-word-as-good and "zg" binding Add "zg" and "zG" bindings Jul 9, 2021
evil-commands.el Show resolved Hide resolved
evil-commands.el Outdated Show resolved Hide resolved
evil-commands.el Outdated Show resolved Hide resolved
@condy0919
Copy link
Contributor Author

condy0919 commented Jul 11, 2021

I can't figure out why it failed only on Emacs 24.5. Download the Emacs 24.5 tarball, grep every symbol in the lisp directory, all have definitions.

evil-commands.el Outdated Show resolved Hide resolved
@wasamasa
Copy link
Member

See #1487 (comment)

@condy0919
Copy link
Contributor Author

See #1487 (comment)

I see...

@condy0919 condy0919 requested a review from wasamasa July 12, 2021 16:58
@condy0919
Copy link
Contributor Author

@wasamasa ping

@wasamasa
Copy link
Member

wasamasa commented Jul 13, 2021

More general issue: The Vim helpfile claims a few more things about those commands:

  • There may be a count to indicate which spellfile entry to use, defaulting to the first. For this you'd need to use evil-define-command with (interactive "<c>"). Does the concept of spellfile entries even make sense for ispell? What about repeatability? If not, then it should be marked as non-repeatable.
  • Visual mode is not handled at all. Some experimentation with Vim is necessary to figure out how exactly it behaves. It claims both that a selection including whitespace may be used, but that a word at point may be used as well.

@KSSysLab
Copy link

KSSysLab commented Jul 13, 2021

More general issue: The Vim helpfile claims a few more things about those commands:

  • There may be a count to indicate which spellfile entry to use, defaulting to the first. For this you'd need to use evil-define-command with (interactive "<c>"). Does the concept of spellfile entries even make sense for ispell? What about repeatability? If not, then it should be marked as non-repeatable.

No, ispell-personal-dictionary can't be a list. But I found doom-emacs provides a non-compatible mechanism which behaves like there are 3 spellfile entries (personal/buffer-local/session dict) when "zg" adding words.

internal-wordlist
The internal word list is used for all buffers where 'spell' is set. It is
not stored, it is lost when you exit Vim. It is also cleared when 'encoding'
is set.

"zG" adds the word to the internal-wordlist which is effective in session-level. Yes, this patch implementes wrong. Sorry :'(

We can't mimic vim perfectly, so which one is better is arguable.

  1. "zg" personal/buffer-local/session dict, "zG" buffer-local dict.
  2. "zg" personal/buffer-local/session dict, "zG" session dict.
  3. "zg" personal/buffer-local dict, "zG" session dict.

Personally, I prefer the third one, it's more orthogonal. Thoughts?

EDITED: Playing in vim, it's non-rerepeatable.

  • Visual mode is not handled at all. Some experimentation with Vim is necessary to figure out how exactly it behaves. It claims both that a selection including whitespace may be used, but that a word at point may be used as well.

I try to avoid this when implementating. Consider the following case: mis speelled has been inserted in the personal dict, but ispell only recognizes the speelled as CORRECT.

image

@condy0919
Copy link
Contributor Author

:'( it's me

@emacs-evil emacs-evil deleted a comment from condy0919 Jul 17, 2021
@wasamasa
Copy link
Member

No, ispell-personal-dictionary can't be a list. But I found doom-emacs provides a non-compatible mechanism which behaves like there are 3 spellfile entries (personal/buffer-local/session dict) when "zg" adding words.

Uh-oh. I'd consult with the Doom people then. Maybe they'd prefer your approach. Maybe their approach is worth using.

"zG" adds the word to the internal-wordlist which is effective in session-level. Yes, this patch implementes wrong. Sorry :'(

We can't mimic vim perfectly, so which one is better is arguable.

  1. "zg" personal/buffer-local/session dict, "zG" buffer-local dict.
  2. "zg" personal/buffer-local/session dict, "zG" session dict.
  3. "zg" personal/buffer-local dict, "zG" session dict.

Personally, I prefer the third one, it's more orthogonal. Thoughts?

Agreed, the third option makes most sense.

EDITED: Playing in vim, it's non-rerepeatable.

  • Visual mode is not handled at all. Some experimentation with Vim is necessary to figure out how exactly it behaves. It claims both that a selection including whitespace may be used, but that a word at point may be used as well.

I try to avoid this when implementating. Consider the following case: mis speelled has been inserted in the personal dict, but ispell only recognizes the speelled as CORRECT.

image

OK, so not repeatable and no handling of visual mode either. In that case it's debatable whether evil-define-cmd adds any benefit.

@condy0919
Copy link
Contributor Author

condy0919 commented Jul 18, 2021

Updated.

I found the session is also buffer-local which is different with vim. Perhaps making "zg" add words to personal and "zG" add words to buffer-local/session-buffer-local is reasonable?

@condy0919
Copy link
Contributor Author

Seems like no more comments.

image

@condy0919
Copy link
Contributor Author

Updated. string without properties will be used. @wasamasa

Implements evil-ispell-mark-word-as-good and evil-ispell-mark-word-as-locally-good
@condy0919
Copy link
Contributor Author

condy0919 commented Jul 23, 2021

Oh, I see! I took some time to read up on it and I think your proposal makes sense; zg -> personal dict, zG -> session, but I'm not sure about using the universal argument; the universal arg is generally very unvimlike. Perhaps there's another key we could use? Since evil doesn't have zu for undo, how about zu -> add to buffer-local personal dict and zU -> remove from buffer-local personal dict?

Not sure we need the old zu keys since, with ispell, I assume we can undo a zg with zG, or a zw with zW. Need to test that assumption though.

Opinions from @hlissner

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.

3 participants