bug#73016: Potential inclusion of kbd-mode, part of kmonad, in Non-GNU ELPA

[Philip Kaludercic <philipk@posteo.net>]

Jeremy Bryant <jb@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