Jeremy Bryant <j...@jeremybryant.net> writes: > Hi, > > kmonad is a keyboard configuration tool under MIT license. > https://github.com/kmonad/kmonad > > There is an Emacs major mode to edit the configuration file, based on > s-expressions. The mode is under GPLv3. > https://github.com/kmonad/kbd-mode
I don't see a reason to add the package in principle. > On behalf of the author, Tony Zorman, I would like to request > consideration to include it in NON-GNU ELPA. Just for the sake of the protocol, is there a reason against adding the package to GNU ELPA? > The author is conscious that the following snippet should be improved > and we are soliciting recommendations on how to improve it. > ;; HACK > (defadvice redisplay (after refresh-font-locking activate) > (when (derived-mode-p 'kbd-mode) > (font-lock-fontify-buffer)))) I agree, we should get rid of that. But first, what is the intention? What breaks if we just remove this advice? > Furthermore if there are any code reviews or recommendations, I attach > the current version. > > I can volunteer some time for some of the changes. > > ;;; kbd-mode.el --- Font locking for kmonad's .kbd files -*- lexical-binding: > t -*- > > ;; Copyright 2020–2022 slotThe > ;; URL: https://github.com/kmonad/kbd-mode > ;; Version: 0.0.1 > ;; Package-Requires: ((emacs "24.3")) > > ;; This file is free software; you can redistribute it and/or modify > ;; it under the terms of the GNU General Public License as published by > ;; the Free Software Foundation; either version 3, or (at your option) > ;; any later version. > ;; > ;; This file is distributed in the hope that it will be useful, > ;; but WITHOUT ANY WARRANTY; without even the implied warranty of > ;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > ;; GNU General Public License for more details. > ;; > ;; You should have received a copy of the GNU General Public License > ;; along with this program. If not, see <http://www.gnu.org/licenses/>. > > ;;; Commentary: > > ;; This file adds basic font locking support for `.kbd' configuration > ;; files. > ;; > ;; To use this file, move it to a directory within your `load-path' and > ;; require it. For example --- assuming that this file was placed > ;; within the `~/.config/emacs/elisp' directory: > ;; > ;; (add-to-list 'load-path "~/.config/emacs/elisp/") > ;; (require 'kbd-mode) > ;; > ;; If you use `use-package', you can express the above as > ;; > ;; (use-package kbd-mode > ;; :load-path "~/.config/emacs/elisp/") Installation instructions are usually out-of-place in the commentary section. Perhaps add a `;;; Installation:` section instead, so that we keep it apart from the documentation that describe-package displays? > ;; > ;; By default we highlight all keywords; you can change this by > ;; customizing the `kbd-mode-' variables. For example, to disable the > ;; highlighting of already defined macros (i.e. of "@macro-name"), you > ;; can set `kbd-mode-show-macros' to `nil'. > ;; > ;; For keybindings, as well as commentary on the `kbd-mode-demo-mode' > ;; minor mode, see the associated README.md file. > > ;;; Code: > > (require 'compile) > > (defgroup kbd nil > "Major mode for editing `.kbd' files." > :group 'languages) > > (defgroup kbd-demo nil > "A minor mode to test your configuration." > :group 'kbd) > > ;;;; Custom variables > > (defgroup kbd-highlight nil > "Syntax highlighting for `kbd-mode'." > :group 'kbd) > > (defcustom kbd-mode-kexpr > '("defcfg" "defsrc" "defalias") > "A K-Expression." This documentation should be expanded on. > :type '(repeat string) > :group 'kbd-highlight) Where is this group defined? The applies below. > > ;; HACK > (defcustom kbd-mode-function-one > '("deflayer") > "Tokens that are treated as functions with one argument." > :type '(repeat string) > :group 'kbd-highlight) > > (defcustom kbd-mode-tokens > '(;; input tokens > "uinput-sink" "send-event-sink" "kext" > ;; output tokens > "device-file" "low-level-hook" "iokit-name") > "Input and output tokens." > :type '(repeat string) > :group 'kbd-highlight) > > (defcustom kbd-mode-defcfg-options > '("input" "output" "cmp-seq-delay" "cmp-seq" "init" "fallthrough" > "allow-cmd") > "Options to give to `defcfg'." > :type '(repeat string) > :group 'kbd-highlight) > > (defcustom kbd-mode-button-modifiers > '("around-next-timeout" "around-next-single" "around-next" "around" > "tap-hold-next-release" "tap-hold-next" "tap-next-release" "tap-hold" > "tap-macro-release" "tap-macro" "multi-tap" "tap-next" "layer-toggle" > "layer-switch" "layer-add" "layer-rem" "layer-delay" "layer-next" > "cmd-button") > "Button modifiers." > :type '(repeat string) > :group 'kbd-highlight) > > (defcustom kbd-mode-show-string > '("uinput-sink" "device-file" "cmd-button") > "Syntax highlight strings in S-expressions. > When an S-expression begins with any of these keywords, highlight > strings (delimited by double quotes) inside it." > :type '(repeat string) > :group 'kbd-highlight) > > (defcustom kbd-mode-show-macros t > "Whether to syntax highlight macros inside layout definitions. > Default: t" Documenting the default is usually not necessary, as customise already remembers is. > :type 'boolean > :group 'kbd-highlight) > > (defcustom kbd-mode-magic-focus nil > "Whether to enable magic focus. > Whenever the `kbd-mode-demo-mode' buffer gets focused, > automatically start try to start a new process for the config > file. When switching back to the config file, kill that process. > > Default: nil" > :type 'boolean > :group 'kbd-demo) > > (defcustom kbd-mode-kill-kmonad nil > "How to kill (or suspend) a running kmonad instance. > This is used when invoking `kbd-mode-start-demo' and, in general, > when entering `kbd-mode-demo-mode' because keyboards can't be > grabbed twice." > :type 'string > :group 'kbd-demo) > > (defcustom kbd-mode-start-kmonad nil > "How to restart (or resume) kmonad. > If there was an active kmonad instance running, which was killed > by `kbd-mode-kill-kmonad', then this (re)starts kmonad with the > given command upon exiting `kbd-mode-demo-mode'." > :type 'string > :group 'kbd-demo) > > ;;;; Faces > > (defgroup kbd-highlight-faces nil > "Faces used for highlighting in `kbd-mode'." > :group 'kbd-highlight) > > (defface kbd-mode-kexpr-face > '((t :inherit font-lock-keyword-face)) > "Face for a K-Expression." > :group 'kbd-highlight-faces) > > (defface kbd-mode-token-face > '((t :inherit font-lock-function-name-face)) > "Face for input and output tokens." > :group 'kbd-highlight-faces) > > (defface kbd-mode-defcfg-option-face > '((t :inherit font-lock-builtin-face)) > "Face for options one may give to `defcfg'." > :group 'kbd-highlight-faces) > > (defface kbd-mode-button-modifier-face > '((t :inherit font-lock-function-name-face)) > "Face for all the button modifiers." > :group 'kbd-highlight-faces) > > (defface kbd-mode-variable-name-face > '((t :inherit font-lock-variable-name-face)) > "Face for a variables, i.e. layer names, macros in layers,..." > :group 'kbd-highlight-faces) > > (defface kbd-mode-string-face > '((t :inherit font-lock-string-face)) > "Face for strings." > :group 'kbd-highlight-faces) > > ;;;; Functions > > (defun kbd-mode--show-macros? (show-macros) A -p would be more conventional for Elisp. > "Decide whether to font-lock macros. > If the argument SHOW-MACROS is non-nil, font-lock macros of the > form `@MACRO-NAME' with `kbd-mode-variable-name-face'." > (let ((macro-regexp '(("\\(:?\\(@[^[:space:]]+\\)\\)" > (1 'kbd-mode-variable-name-face))))) > (if show-macros > (font-lock-add-keywords 'kbd-mode macro-regexp) > (font-lock-remove-keywords 'kbd-mode macro-regexp)))) > > ;;; Vars > > (defvar kbd-mode-syntax-table > (let ((st (make-syntax-table))) > ;; Use ;; for regular comments and #| |# for line comments. > (modify-syntax-entry ?\; ". 12b" st) > (modify-syntax-entry ?\n "> b" st) > (modify-syntax-entry ?\# ". 14" st) > (modify-syntax-entry ?\| ". 23" st) > ;; We don't need to highlight brackets, as they're only used inside > ;; layouts. > (modify-syntax-entry ?\[ "." st) > (modify-syntax-entry ?\] "." st) > ;; We highlight the necessary strings ourselves. > (modify-syntax-entry ?\" "." st) > st) > "The basic syntax table for `kbd-mode'.") > > (defvar kbd-mode--font-lock-keywords > (let ((kexpr-regexp (regexp-opt kbd-mode-kexpr > 'words)) > (token-regexp (regexp-opt kbd-mode-tokens > 'words)) > (defcfg-options-regexp (regexp-opt kbd-mode-defcfg-options > 'words)) > (button-modifiers-regexp (regexp-opt kbd-mode-button-modifiers > 'words)) > (function-one-regexp > (concat "\\(?:\\(" > (regexp-opt kbd-mode-function-one) > "\\)\\([[:space:]]+[[:word:]]+\\)\\)")) > ;; Only highlight these strings; configuration files may explicitly > ;; use a " to emit a double quote, so we can't trust the default > ;; string highlighting. > (string-regexp > (concat "\\(['\(]" > (regexp-opt kbd-mode-show-string) > "\\)\\(\\S)+\\)\)"))) > `((,token-regexp (1 'kbd-mode-token-face )) > (,kexpr-regexp (1 'kbd-mode-kexpr-face )) > (,button-modifiers-regexp (1 'kbd-mode-button-modifier-face)) > (,defcfg-options-regexp (1 'kbd-mode-defcfg-option-face )) > (,function-one-regexp > (1 'kbd-mode-kexpr-face ) > (2 'kbd-mode-variable-name-face)) > (,string-regexp > ("\"[^}]*?\"" > (progn (goto-char (match-beginning 0)) (match-end 0)) > (goto-char (match-end 0)) > (0 'kbd-mode-string-face t))))) > "Keywords to be syntax highlighted.") > > ;;; Define Major Mode > > ;; Because the configuration language is in a lispy syntax, we can > ;; inherit from any lisp mode in order to get good parenthesis handling > ;; for free. > > (defvar kbd-mode-map > (let ((map (make-sparse-keymap))) > (define-key map (kbd "C-c C-c") #'kbd-mode-start-demo) > (define-key map (kbd "C-c C-z") #'kbd-mode-switch) > map)) > > ;;;###autoload > (define-derived-mode kbd-mode emacs-lisp-mode "Kbd" > "Major mode for editing `.kbd' files. > > For details, see `https://github.com/kmonad/kmonad'." > (set-syntax-table kbd-mode-syntax-table) > (use-local-map kbd-mode-map) > (font-lock-add-keywords 'kbd-mode kbd-mode--font-lock-keywords) > (kbd-mode--show-macros? kbd-mode-show-macros) > ;; HACK > (defadvice redisplay (after refresh-font-locking activate) > (when (derived-mode-p 'kbd-mode) > (font-lock-fontify-buffer)))) I didn't realise that this was being evaluated every time the major mode is initialised... > > ;; Associate the `.kbd' ending with `kbd-mode'. > ;;;###autoload > (add-to-list 'auto-mode-alist '("\\.kbd\\'" . kbd-mode)) > > ;;;; Demo Minor Mode > > (defvar kbd-mode-demo-file nil > "Path to the users configuration file. > This is used in `kbd-mode-demo-mode' for deciding what > configuration to compile.") > > (defvar kbd-mode-had-kmonad? nil > "Whether the user had a running kmonad instance. > This controls whether kmonad will be restarted by mean of > `kbd-mode-start-kmonad' after exiting `kbd-mode-demo-mode'.") > > (defvar kbd-mode-demo-mode-map > (let ((map (make-sparse-keymap))) > (define-key map (kbd "C-c C-c") #'kbd-mode-stop-demo) > (define-key map (kbd "C-c C-z") #'kbd-mode-switch) > map)) > > ;;;###autoload > (define-minor-mode kbd-mode-demo-mode > "Toggle kmonad demo mode. > This is a minor mode, in which users can test their > configurations." > :lighter " kbd-demo" > :keymap kbd-mode-demo-mode-map This should be inferred. > > (when kbd-mode-demo-mode > (unless (kbd-mode--valid-config?) > (kbd-mode--show-error))) > > ;; Handle toggle > (when kbd-mode-magic-focus > (cond (kbd-mode-demo-mode > (add-hook 'window-selection-change-functions > #'kbd-mode--toggle-demo nil t) > (add-hook 'focus-in-hook #'kbd-mode--create-kmonad-process nil t) > (add-hook 'focus-out-hook #'kbd-mode--kill-demo-process nil t)) > (t > (remove-hook 'window-selection-change-functions > #'kbd-mode--toggle-demo t) > (remove-hook 'focus-in-hook #'kbd-mode--create-kmonad-process t) > (remove-hook 'focus-out-hook #'kbd-mode--kill-demo-process t))))) > > ;;;; Interactive Functions > > ;;;###autoload > (defun kbd-mode-start-demo () > "Try the current configuration in a demo buffer. > Use `kbd-mode-stop-demo' to stop the demo. If the configuration > file has errors, the demo will not start and an error buffer will > be shown instead." > (interactive) > (setq kbd-mode-demo-file > (kbd-mode--find-kbd-file (buffer-file-name (current-buffer)))) > (if (not (kbd-mode--valid-config?)) > (kbd-mode--show-error) > (when (shell-command "ps -C kmonad") Shell-command is usually used for interactive stuff. And I don't know what you are trying to test the return value for here. (shell-command "true") ;;=> 0 (shell-command "false") ;;=> 1 (shell-command "foo") ;;=> 127 As the documentation says: In Elisp, you will often be better served by calling ‘call-process’ or ‘start-process’ directly, since they offer more control and do not impose the use of a shell (with its need to quote arguments). > (setq kbd-mode-had-kmonad? t) > (kbd-mode--kill-kmonad)) > (kbd-mode--create-demo-buffer) > (pop-to-buffer "*kmonad-demo*") > (kbd-mode--create-kmonad-process) > (kbd-mode-demo-mode t))) > > (defun kbd-mode-stop-demo () > "Stop the currently running demo." > (interactive) > (with-current-buffer "*kmonad-demo*" > (kbd-mode-demo-mode 0) > (kill-buffer-and-window) > (kbd-mode--kill-demo-process) > (when kbd-mode-had-kmonad? > (kbd-mode--start-kmonad)))) > > (defun kbd-mode-switch () > "Switch between the demo window and the config file." > (interactive) > (select-window (get-buffer-window > (if (and (equal (buffer-name) "*kmonad-demo*") > kbd-mode-demo-mode) > (get-file-buffer kbd-mode-demo-file) > "*kmonad-demo*")))) > > ;;;; Helper Functions > > (defun kbd-mode--create-demo-buffer () > "Create the *kmonad-demo* buffer." > (unless (get-buffer "*kmonad-demo*") > (display-buffer (get-buffer-create "*kmonad-demo*") > '(display-buffer-at-bottom > (window-height . 0.15))))) > > (defun kbd-mode--find-kbd-file (&optional file) > "Find the config file. > If the optional argument FILE is given, use it instead. > Otherwise, prompt the user for a choice." > (if (and file (string= (file-name-extension file) "kbd")) > file > (expand-file-name (read-file-name "Choose configuration file")))) > > (defun kbd-mode--valid-config? () > "Check if the current configuration is valid." > (let ((command (kbd-mode--get-config-validation-command))) > (eq 0 (shell-command command)))) > > (defun kbd-mode--create-kmonad-process () > "Start the kmonad demo process in a dedicated buffer." > (when (get-buffer-process "*kmonad*") > (kbd-mode--kill-demo-process)) > (start-process "kmonad-emacs" "*kmonad*" "kmonad" kbd-mode-demo-file)) > > (defun kbd-mode--kill-demo-process () > "Kill demo kmonad process, if possible." > (when (get-buffer-process "*kmonad*") > (kill-process "*kmonad*"))) There might be a race condition here. > > (defun kbd-mode--kill-kmonad () > "Kill (or suspend) a running kmonad instance. > The command used to kill kmonad is given by the > `kbd-mode-kill-kmonad' variable." > (if kbd-mode-kill-kmonad > (shell-command kbd-mode-kill-kmonad) > (error "To kill the running kmonad instance, customize the > `kbd-mode-kill-kmonad' variable!"))) > > (defun kbd-mode--start-kmonad () > "Start (or resume) a new kmonad process. > The command used to start kmonad is given by the > `kbd-mode-start-kmonad' variable." > (if kbd-mode-kill-kmonad > (call-process-shell-command > ;; Force the command to be executed asynchronously. > (if (eq (aref kbd-mode-start-kmonad If you always know that you are comparing character codes, I'd use = to be more specific and trigger an error if something else occurs. > (1- (length kbd-mode-start-kmonad))) > ?&) > kbd-mode-start-kmonad > (concat kbd-mode-start-kmonad "&"))) > (error "To restart kmonad, customize the `kbd-mode-start-kmonad' > variable!"))) > > (defun kbd-mode--toggle-demo (&optional _window) > "Toggle the kmonad demo process. > When the users exits the demo window, kill the demo process and > start a \"normal\" kmonad process instead. When re-entering the > demo window, do the opposite; i.e., kill the running kmonad > instance and spawn a demo process." > (cond ((kbd-mode--was-demo?) > (kbd-mode--kill-demo-process) > (kbd-mode--start-kmonad)) > ((kbd-mode--valid-config?) > (kbd-mode--kill-kmonad) > (kbd-mode--create-kmonad-process)) > (t > (kbd-mode--start-kmonad) > (kbd-mode--show-error)))) > > (defun kbd-mode--was-demo? () > "Was the previous buffer the kmonad demo buffer?" > (equal (window-buffer (previous-window)) eq should be enough here. > (get-buffer "*kmonad-demo*"))) > > (defun kbd-mode--show-error () > "Show configuration errors in a compilation buffer." > (when kbd-mode-demo-mode > (quit-window 'kill "*kmonad-demo*")) > (compile (kbd-mode--get-config-validation-command))) > > (defun kbd-mode--get-config-validation-command () > "Get validation command for `kbd-mode-demo-file'." > (concat "kmonad -d " kbd-mode-demo-file)) > > ;;;; Integration with `compilation-mode' > > (add-to-list 'compilation-error-regexp-alist 'kbd) > (add-to-list 'compilation-error-regexp-alist-alist > '(kbd "^kmonad: Parse error at \\([0-9]+\\):\\([0-9]+\\)" nil 1 > 2)) > > (provide 'kbd-mode) > > ;;; kbd-mode.el ends here All in all it looks fine, most of my superficial comments are nitpicks. -- Philip Kaludercic on siskin
