general.el icon indicating copy to clipboard operation
general.el copied to clipboard

Add :general-config and add FAQ entry on avoiding performance issues

Open noctuid opened this issue 2 years ago • 14 comments

Fixes #502.

noctuid avatar Apr 09 '22 15:04 noctuid

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (9651024) 69.40% compared to head (b1090d7) 69.19%. Report is 3 commits behind head on master.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #503      +/-   ##
==========================================
- Coverage   69.40%   69.19%   -0.21%     
==========================================
  Files           1        1              
  Lines        1000     1003       +3     
==========================================
  Hits          694      694              
- Misses        306      309       +3     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 09 '22 15:04 codecov-commenter

By the way, I'm curious what you've used to measure startup time if you don't mind sharing. I've been out of the loop for a while and curious what options are out there. I was trying this fork of benchmark-init.el since the original was broken, but I don't recall if it was all that useful.

blaenk avatar Apr 10 '22 02:04 blaenk

Yes, I just used benchmark-init.

noctuid avatar Apr 10 '22 17:04 noctuid

@noctuid Hi, If I bind keys in a prefix map, the which-key settings are not working. current:

  (general-define-key
   :states '(normal insert motion emacs)
   :keymaps 'override
   :prefix-map 'tyrant-map
   :prefix "SPC"
   :non-normal-prefix "M-SPC")

  (general-create-definer tyrant-def :keymaps 'tyrant-map)
  (tyrant-def "" nil)
  (tyrant-def
    "b"       '(:ignore t :which-key "buffers")
    "bb"      'switch-to-buffer
    "bB"      'ibuffer
    "bd"      'kill-current-buffer
    "bm"      'switch-to-messages-buffer
    "bs"      'switch-to-scratch-buffer
    "bu"      'reopen-killed-buffer
    "bx"      'kill-buffer-and-window)

previous:

  (general-create-definer tyrant-def
    :states '(normal insert motion emacs)
    :keymaps 'override
    :prefix "SPC"
    :non-normal-prefix "M-SPC")
  (tyrant-def "" nil)
  (tyrant-def
    "b"       '(:ignore t :which-key "buffers")
    "bb"      'switch-to-buffer
    "bB"      'ibuffer
    "bd"      'kill-current-buffer
    "bm"      'switch-to-messages-buffer
    "bs"      'switch-to-scratch-buffer
    "bu"      'reopen-killed-buffer
    "bx"      'kill-buffer-and-window)

tshu-w avatar Apr 21 '22 12:04 tshu-w

Prefer not using :which-key wherever possible and instead use keymap-based replacements. In this case, I think you can bind "b" to something like (cons "buffers" (make-sparse-map)) instead of to '(:ignore t :which-key "buffers"). Try that and let me know if it works.

At some point I will add a new keyword :desc that will allow you to write "b" '(:desc "buffers"), but for now use my suggestion above or use which-key-add-keymap-based-replacements. Unlike :which-key, :desc will only accept a string replacement. If this is simple enough, I will add it to this PR.

I will update the readme to discourage using :which-key (if it doesn't already) unless you need a complex replacement. It is slower and less precise.

noctuid avatar Apr 21 '22 12:04 noctuid

@noctuid (cons "buffers" (make-sparse-map)) works.

I have another question: I want to create a major mode prefix definer, how can I do that. currently this is not working.

  (general-define-key
   :states '(normal insert motion emacs)
   :keymaps 'override
   :prefix-map 'despot-map
   :major-modes t
   :prefix "SPC m"
   :non-normal-prefix "M-SPC m")

  (general-create-definer despot-def :keymaps 'despot-map)
  (despot-def "" nil)

previous works:

  (general-create-definer despot-def
    :states '(normal insert motion emacs)
    :keymaps 'override
    :major-modes t
    :prefix "SPC m"
    :non-normal-prefix "M-SPC m")
  (despot-def "" nil)

tshu-w avatar Apr 21 '22 13:04 tshu-w

If I understand correctly, you will use this definer with different keymaps, in which case you should remove the :keymaps 'override. The suggestion to use a prefix map will only work for a single keymap. For a major-mode leader, you will need one prefix map per keymap. Try this (untested) to automatically generate the prefix keymaps:

(general-create-definer despot--prefix-def
  :states '(normal insert motion emacs)
  :prefix "SPC m"
  :non-normal-prefix "M-SPC m")

(defmacro despot-def (keymap &rest args)
  "Define keys using major-mode leader prefixes (\"SPC m\" or \"M-SPC m\").
KEYMAP should be an unquoted symbol. ARGS should be `general-def' args."
  (let ((prefix-map (intern (format "general-despot-%s" keymap))))
    `(progn
       (unless (boundp ',prefix-map)
         (despot--prefix-def ,keymap :prefix-map ',prefix-map))
       (general-def ,prefix-map ,@args))))

;; example usage; if you use a a syntax different from this, some changes to despot-def will be necessary
(despot-def emacs-lisp-mode-map "d" #'eval-defun)

This example code has some limitations/caveats. It will only work for a single keymap per use. Let me know if you use despot-def with multiple keymaps or a different syntax from the one here, and I can provide a better example.

I'll have to think about the best way to handle more complex use cases and add support for them. Eventually, there will be some way to opt-in to creating prefix maps automatically (e.g. can put :prefix-map t in a general-create-definer to automatically create/use a new prefix map based on the keymap name).

noctuid avatar Apr 21 '22 16:04 noctuid

This example code has some limitations/caveats. It will only work for a single keymap per use. Let me know if you use despot-def with multiple keymaps or a different syntax from the one here, and I can provide a better example.

Thanks, sorry I was a little vague, but you got it. This example is enough.

tshu-w avatar Apr 21 '22 16:04 tshu-w

Thanks to @tshu-w for asking about the which-key prefixes as I actually had not noticed that the which-key labels weren't working anymore, and thanks @noctuid for providing the solution.

For anyone else who comes by reading this, note that it's actually (make-sparse-keymap) not (make-sparse-map) (ran into this myself).

Maybe it's something worth noting in the readme @noctuid as it's something that could easily be overlooked at the time when making these changes only to notice it later and not be super clear on what may have caused it.

blaenk avatar Apr 23 '22 20:04 blaenk

Yeah once I have some time I will update the documentation for these things and possibly add some helpers to simplify things.

I also forgot to mention that I also used profile-dotemacs, which is a lot more fine-grained/useful than benchmark-init for code that you have in your init (instead of just requires).

noctuid avatar Apr 27 '22 13:04 noctuid

Ah I see, thanks!

For others:

  • https://www.emacswiki.org/emacs/ProfileDotEmacs
  • Mirrored/ELPA: https://github.com/raxod502/profile-dotemacs

blaenk avatar Apr 27 '22 16:04 blaenk

Hi, I'm facing another issue that I cannot define for minor-mode after using prefix-map: currently:

  (general-define-key
   :states '(normal insert motion emacs)
   :keymaps 'override
   :prefix-map 'tyrant-map
   :prefix "SPC"
   :non-normal-prefix "M-SPC")

  (general-create-definer tyrant-def :keymaps 'tyrant-map)
  (tyrant-def "" nil)

  (tyrant-def eglot--managed-mode :definer 'minor-mode
    "ce"  (cons "eglot" (make-sparse-keymap))
    "cea" 'eglot-code-actions
    "ceb" 'eglot-events-buffer
    "cer" 'eglot-rename
    "ceR" 'eglot-reconnect
    "cex" 'eglot-shutdown
    "ceX" 'eglot-shutdown-all
    "ce=" 'eglot-format)

previous:

  (general-create-definer tyrant-def
    :states '(normal insert motion emacs)
    :keymaps 'override
    :prefix "SPC"
    :non-normal-prefix "M-SPC")
  (tyrant-def "" nil)
  (tyrant-def eglot--managed-mode :definer 'minor-mode
    "ce"  (cons "eglot" (make-sparse-keymap))
    "cea" 'eglot-code-actions
    "ceb" 'eglot-events-buffer
    "cer" 'eglot-rename
    "ceR" 'eglot-reconnect
    "cex" 'eglot-shutdown
    "ceX" 'eglot-shutdown-all
    "ce=" 'eglot-format)

tshu-w avatar May 20 '22 07:05 tshu-w

I think the issue is you're reusing your global/override-mode-map definer for non-global keybindings. This worked before because there was no :prefix-map but is still a little confusing. I assume you didn't bind c to anything else with tyrant-def? Otherwise, I don't think this would work at all.

You should create a separate definer that doesn't have :prefix-map. You can use the same strategy as despot-def to still use a prefix keymap.

Alternatively, if you never need to use SPC c for anything else, you can just bind in general-override-mode-map:

(tyrant-def
  :predicate '(bound-and-true-p eglot--managed-mode)
  :infix "c"
  "ea" #'eglot-code-actions
  ...)

The predicate isn't technically necessary but will prevent errors.

noctuid avatar May 22 '22 12:05 noctuid

I forget to remove c when simplify bindings to show what I want.

You should create a separate definer that doesn't have :prefix-map. You can use the same strategy as despot-def to still use a prefix keymap.

I got it, thx a lot.

tshu-w avatar May 23 '22 10:05 tshu-w