Skip to content

Commit

Permalink
Use a dedicated symbol to hide from read-extended-command
Browse files Browse the repository at this point in the history
By using a dedicated symbol, which uses our package prefix, we ensure
that we can set `read-extended-command-predicate', without affecting
any unrelated package.

`transient-command-completion-not-suffix-only-p' only prevents
"anonymously defined" transient suffix commands from being offered as
`read-extended-command' completion candidates.  Furthermore, because
we only set `read-extended-command-predicate' iff it is non-nil, this
does not mess with any user customization of this option.  Also note
that if the user uses `command-completion-default-include-p' instead,
then the suffix-only commands are also filtered out.

IMO it is reasonable for a package to use this feature to declare
that certain commands should absolutely never be offered as completion
candidates.  The interface of this feature is not optimized for this
use-case, but I predict that `read-extended-command-predicate' will
one day be a hook, at which point I would not have to write this
explanation in the hope that it convincingly makes the case for why
daring to us it to achieve the goal that it was designed to achieve
is not Wrong™, not even if it is a library that does it, as opposed
to a user.

It has been suggested that Transient should instead avoid `intern'ing
any symbols for these (admittedly not fully) anonymous suffix
commands.  The proposed patch does not actually work, because these
symbols *have to be* interned, and for backward compatibility reasons
`transient--init-suffix' ensures that they are.  So initially (at
load-time) these suffix command symbols would not be interned but as
the user invokes transient commands, more and more of them would
become interned.

We could avoid interning anonymously defined suffix commands
altogether, but that would have unfortunate consequences.  In fact
Transient used to do that because `read-extended-command-predicate'
did not exist yet.  But I sure wished it did, and I was considering
implementing something like that myself.  So of course once that
feature appear, I was happy to take advantage, in [1: 226db67].

Going back to not binding every suffix command to an interned symbol,
would complicate matters considerably.  It would lead to bugs, and
would slow down development, because I would constantly have to double
check changes, to reduce the risk of introducing such bugs.  The code
would be more difficult to understand.  Frankly, doing so would mean
intentionally adding accidental complexity.  I very much want to avoid
that oxymoron.

Furthermore, not interning anonymously defined suffixes, would do
nothing to hide infix arguments that are explicitly defined using
`transient-define-infix', but those should also not be offered as
completion candidates.  The approach I have chosen does.

Instead of #273.

1: 2023-08-12 226db67
   All suffix commands now must be accessed through fbound symbols
  • Loading branch information
tarsius committed Jan 21, 2024
1 parent a678d61 commit 522b625
Showing 1 changed file with 18 additions and 8 deletions.
26 changes: 18 additions & 8 deletions lisp/transient.el
Original file line number Diff line number Diff line change
Expand Up @@ -1007,7 +1007,7 @@ keyword.
`(progn
(defalias ',name #'transient--default-infix-command)
(put ',name 'interactive-only t)
(put ',name 'completion-predicate #'ignore)
(put ',name 'completion-predicate #'transient--suffix-only)
(put ',name 'function-documentation ,docstr)
(put ',name 'transient--suffix
(,(or class 'transient-switch) :command ',name ,@slots)))))
Expand Down Expand Up @@ -1037,7 +1037,8 @@ this case, because the `man-page' slot was not set in this case."
(transient-infix-set obj (transient-infix-read obj)))
(transient--show))
(put 'transient--default-infix-command 'interactive-only t)
(put 'transient--default-infix-command 'command-predicate #'ignore)
(put 'transient--default-infix-command 'command-predicate
#'transient--suffix-only)

(eval-and-compile
(defun transient--expand-define-args (args &optional arglist)
Expand Down Expand Up @@ -1148,7 +1149,7 @@ this case, because the `man-page' slot was not set in this case."
args :command
`(prog1 ',sym
(put ',sym 'interactive-only t)
(put ',sym 'command-predicate #'ignore)
(put ',sym 'command-predicate #'transient--suffix-only)
(defalias ',sym
,(if (eq (car-safe cmd) 'lambda)
cmd
Expand All @@ -1171,7 +1172,7 @@ this case, because the `man-page' slot was not set in this case."
args :command
`(prog1 ',sym
(put ',sym 'interactive-only t)
(put ',sym 'command-predicate #'ignore)
(put ',sym 'command-predicate #'transient--suffix-only)
(defalias ',sym #'transient--default-infix-command))))
(cond ((and car (not (keywordp car)))
(setq class 'transient-option)
Expand Down Expand Up @@ -1209,16 +1210,25 @@ this case, because the `man-page' slot was not set in this case."
(and (string-match "\\`\\(-[a-zA-Z]\\)\\(\\'\\|=\\)" arg)
(match-string 1 arg))))

(defun transient-command-completion-not-ignored-p (symbol _buffer)
(defun transient-command-completion-not-suffix-only-p (symbol _buffer)
"Say whether SYMBOL should be offered as a completion.
If the value of SYMBOL's `completion-predicate' property is
`ignore', then return nil, otherwise return t."
(not (eq (get symbol 'completion-predicate) 'ignore)))
`transient--suffix-only', then return nil, otherwise return t.
This is the case when a command should only ever be used as a
suffix of a transient prefix command (as opposed to bindings
in regular keymaps or by using `execute-extended-command')."
(not (eq (get symbol 'completion-predicate) 'transient--suffix-only)))

(defalias 'transient--suffix-only #'ignore
"Ignore ARGUMENTS, do nothing, and return nil.
Also see `transient-command-completion-not-suffix-only-p'.
Only use this alias as the value of the `command-predicate'
symbol property.")

(static-if (and (boundp 'read-extended-command-predicate) ; since Emacs 28.1
(not read-extended-command-predicate))
(setq read-extended-command-predicate
'transient-command-completion-not-ignored-p))
'transient-command-completion-not-suffix-only-p))

(defun transient-parse-suffix (prefix suffix)
"Parse SUFFIX, to be added to PREFIX.
Expand Down

0 comments on commit 522b625

Please sign in to comment.