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

Improve performance, add typing detection #12

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

LemonBreezes
Copy link
Collaborator

From the README:

  • 0.8.2 (2025-03-15)
    Use float-times instead of timers for typing detection
  • 0.8.1 (2025-03-11)
    Add typing detection to prevent accidental chord triggering
    Add minimum delay for double-tap chords
    Performance improvements

@LemonBreezes
Copy link
Collaborator Author

LemonBreezes commented Mar 16, 2025

@BlueDrink9 @conao3 @irigone Hey, would you please check out my branch to see if it resolves your issues with key-chord? I have access to merge this PR myself but need feedback before I do so.

key-chord.el Outdated
@@ -54,6 +54,12 @@ sure when executing whether two keys were typed quickly or slowly
when recorded.)"
:type 'boolean)

(defcustom key-chord-min-delay 0.05
Copy link
Member

Choose a reason for hiding this comment

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

This new variable isn't being used. I assume you start using it in a later commit. These commits should be squashed into a single commit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hm you are somehow reviewing an old commit. Yup, I have not cleaned up the commit history. Maybe I should squash them all into 1?

key-chord.el Outdated
@@ -173,7 +179,8 @@ Commands. Please ignore that."
(cond
((and (not (eq first-char key-chord-last-unmatched))
(key-chord-lookup-key (vector 'key-chord first-char)))
(let ((delay (if (key-chord-lookup-key
(let ((start-time (current-time))
Copy link
Member

Choose a reason for hiding this comment

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

Likewise this isn't being used (yet?).

@tarsius
Copy link
Member

tarsius commented Mar 16, 2025

(Lifting this to the top-level.)

Hm you are somehow reviewing an old commit.

Those are the commits you pushed, so that's what I have to review.

Yup, I have not cleaned up the commit history. Maybe I should squash them all into 1?

Yes, you should cleanup the commit history. I don't think squashing into a single commit is appropriate. The addition of .gitignore should be a separate commit for example. Metadata should be a separate commit. Cleanup such as

-  (if (/= 2 (length keys))
-      (error "Key-chord keys must have two elements"))
+  (when (/= 2 (length keys))
+    (error "Key-chord keys must have two elements"))

should be a separate commit. The functional change(s?) add a lot of new code, making review harder. By moving unrelated things out of the feature commit(s?) you make reviewing them easier.

Some style feedback:

  • Lines should stay below 80 characters like the old code and readme.
  • First line of docstrings should be a sentence ending with a period.
  • Make sure not to add trailing whitespace.
  • Add space between top-level forms.
  • Capitalize fix: This consistently (or consistently not).

@LemonBreezes LemonBreezes force-pushed the master branch 3 times, most recently from 52e28e8 to a433780 Compare March 17, 2025 22:58
@LemonBreezes
Copy link
Collaborator Author

LemonBreezes commented Mar 17, 2025

Okay. I have done the stylistic changes and squashed a few of the commits and moved some around. It's surprisingly difficult to rewrite the commit history! Even the AI can't do this well.

@tarsius
Copy link
Member

tarsius commented Mar 19, 2025

I cannot review this properly and because I don't use this myself I cannot even try it out to test whether it feels snappier than before. The same was true for #2 and I made the mistake of merging that, which resulted in regressions, which took a while to fix (by reverting).

I don't want to repeat that mistake. I hope we can find some users who are willing to test this for an extended period of time, before we merge.

@LemonBreezes
Copy link
Collaborator Author

I cannot review this properly and because I don't use this myself I cannot even try it out to test whether it feels snappier than before. The same was true for #2 and I made the mistake of merging that, which resulted in regressions, which took a while to fix (by reverting).

I don't want to repeat that mistake. I hope we can find some users who are willing to test this for an extended period of time, before we merge.

Ok. I made a Reddit post here https://www.reddit.com/r/emacs/comments/1jfa2bp/looking_for_users_to_test_a_new_version_of/. If no one takes a look there, I can post it in other places too.

@LemonBreezes
Copy link
Collaborator Author

LemonBreezes commented Mar 22, 2025

Pushed some changes to fix some subtle issues with how the typing detection works as well as a subtle error. Also made it so that key-chord is completely bypassed when read-key is active. It just seemed like common sense because the key won't make a chord anyways, but it was causing an issue with the typing detection.

@g-gundam
Copy link

I tried it out on Emacs 30.1 using the following configuration.

(defun c/keybindings ()
  ;; key-chord - https://github.com/emacsorphanage/key-chord
  ;; + Chords should use both hands for maximum efficiency.
  ;;   + Practically all 2 hand chords work.
  ;;   + Some single-hand chords may work,
  ;;     but be careful not to contort your fingers.
  (key-chord-mode 1)
  (setq key-chord-two-keys-delay 0.07)
  ;; evil
  (key-chord-define evil-insert-state-map "jj" 'evil-normal-state) ; https://stackoverflow.com/a/13543550
  ;; unicode / quotes
  (key-chord-define-global "z'" "")      ; curly single quotes
  (key-chord-define-global "x'" "")
  (key-chord-define-global "q'" [#x201c]) ; curly double quotes
  (key-chord-define-global "w'" [#x201d])
  ;; unicode / symbols
  (key-chord-define-global "a=" [#x0fd5]) ;
  (key-chord-define-global "b=" [#x0fd6]) ;
  (key-chord-define-global "c0" [#x00a9]) ; © copyright
  (key-chord-define-global "y=" [#x00a5]) ; ¥ yen
  (key-chord-define-global "l=" [#x00a3]) ; £ pound
  (key-chord-define-global "3." [#x2764]) ; ❤ heart
  ;; unicode / letters
  (key-chord-define-global "n`" [#x00f1]) ; ñ enye
  (key-chord-define-global "a'" [#xe1])   ; á
  (key-chord-define-global "e'" [#xe9])   ; é
  (key-chord-define-global "i'" [#xed])   ; í
  (key-chord-define-global "o'" [#xf3])   ; ó
  ;; julia programming
  (key-chord-define-global "z\\" [#x7c #x3e]) ; |>
  ;; org-mode
  (key-chord-define-global "t0" 'i/org-time-stamp-inactive)
  (key-chord-define-global "28" (u/org-insert-datestamped-heading-fn 2))
  (key-chord-define-global "38" (u/org-insert-datestamped-heading-fn 3))
  )

(use-package key-chord
  :vc (:url "https://github.com/LemonBreezes/key-chord" :branch "master")
  :config
  (c/keybindings)
  ;; key-chord international hack
  (advice-add
   'toggle-input-method
   :after
   (lambda (&rest r)
     (if (equal current-input-method nil)
	 (progn
	   (c/keybindings)
	   (message "rebinding"))
       (message current-input-method)))
   '((name . keychords))))

The only difference from my normal config is the use-package where instead of :vc it normally says :ensure t.

I did not notice any regressions.

  • Everything in my config worked as expected.
  • I didn't really notice any differences in feel.

PS: This way of setting up key-chord is my way of working around the limitations mentioned in the README about key-chord and internationalization packages not working well together. The advice that's added can help them coexist. While using international input, you can't use key-chord, but when you toggle back to regular input, you get all your key-chord config back again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants