Update transient.el. Don't intern transient symbols #273
Conversation
See Emacs bug#68568
|
I played with a backport to 0.5.2 and on master. I still see all of the commands in my completions. If that's the motivation for change, what are we doing? (intern-soft "transient:tsc-cowsay:-b") ; transient:tsc-cowsay:-bSo the interning is happening anyway. I'm not sure that an uninterned symbol can be used in a keymap, and if not, then the only implementations use private command names like these or re-implement everything on top repeated calls to |
Likely the commands were already there from previous internations. Try the patch from scratch or use this to unintern the existing ones: |
|
I definitely did not just check for symbols on dirty Emacsen. You want to look at your obarray after loading GPTel etc, which just defines the prefixes. |
|
I saw the bug and was gonna reply soon. Because I am on a productive bug squashing spree in another package and wanted to maintain the flow, I didn't reply immediately. But I should sooner have helped you make your Emacs usable again: (setq read-extended-command-predicate 'command-completion-default-include-p)Transient does not do that itself, because "thou shalt not change options behind the users back". But I am now considering adding this: (defun transient-command-completion-non-suffix-only-p (symbol _buffer)
"Say whether SYMBOL should be offered as a completion.
If SYMBOL is only intended to be called as suffix command from
a transient prefix commands, return nil, t otherwise. Consider
using `command-completion-default-include-p' instead."
(if (get symbol 'completion-predicate)
;; An explicit completion predicate takes precedence.
(funcall (get symbol 'completion-predicate) symbol buffer)
(not (eq (car (command-modes symbol)) 'not-a-mode))))
(when (and (boundp 'read-extended-command-predicate)
(not read-extended-command-predicate))
(setq read-extended-command-predicate
'transient-command-completion-non-suffix-only-p)) |
|
You're right, this is not enough for all symbols. It is somehow enough for the really long ones |
|
Definitely don't set any options in user Emacs. I don't want to set that option: and I think it's perfectly legitimate to not want to and still be able to use completion elsewhere without monstrous useless transient commands following me around. So could you explain why those symbols need to be interned in the obarray: (1) at compile-time (2) at load time (3) at run-time. Also could you explain why some symbols need to have the actual description in their symbol names and why something like |
I think you misunderstand what my predicate does. It excludes only those symbols that you don't want included. (Okay okay, it might also exclude symbols with non-nil Right now you only have a choice between setting that option to (defun transient-command-completion-non-suffix-only-p (symbol _buffer)
"Say whether SYMBOL should be offered as a completion.
If SYMBOL is only intended to be called as suffix command from
a transient prefix commands, return nil, t otherwise. Consider
using `command-completion-default-include-p' instead."
(not (eq (car (command-modes symbol)) 'not-a-mode))), live with the noise, or stop using transient. (Choosing the first option will achieve your goal (and nothing else), so I hope you will reconsider.)
Yes of course, but it will take some time because I will have to correct your misconceptions and because it is non-trivial in the first place. I hope that the presented solution is acceptable to you, at least as a workaround for another two days, while I finish the work I am currently immersed in. |
|
Alright, i understand why interned symbols are needed finally. There are two keymaps with an indirection which is the symbol. So, here's a better patch, not much more complicated: diff --git a/lisp/transient.el b/lisp/transient.el
index f9060f5ba85..1ee19d9944f 100644
--- a/lisp/transient.el
+++ b/lisp/transient.el
@@ -1098,6 +1098,10 @@ transient--parse-group
(cl-mapcan (lambda (s) (transient--parse-child prefix s))
spec))))))
+(defvar transient-ginormous-interned-symbols nil)
+(defun transient--intern-maybe (name)
+ (if transient-ginormous-interned-symbols (intern name) (gensym "transient:")))
+
(defun transient--parse-suffix (prefix spec)
(let (level class args)
(cl-symbol-macrolet
@@ -1127,7 +1131,7 @@ transient--parse-suffix
((and (commandp car)
(not (stringp car)))
(let ((cmd pop)
- (sym (intern
+ (sym (transient--intern-maybe
(format "transient:%s:%s"
prefix
(let ((desc (plist-get args :description)))
@@ -1156,7 +1160,7 @@ transient--parse-suffix
(when-let ((shortarg (transient--derive-shortarg arg)))
(setq args (plist-put args :shortarg shortarg)))
(setq args (plist-put args :argument arg))))
- (setq sym (intern (format "transient:%s:%s" prefix arg)))
+ (setq sym (transient--intern-maybe (format "transient:%s:%s" prefix arg)))
(setq args (plist-put
args :command
`(prog1 ',symBefore the patch: Will bring up gibberish. After the patch it won't. You can customize
No, I'm pretty sure I understand it fixes the problem. For me (so thanks!). But this isn't about me. This is about preventing a library -- that isn't exactly an opt-out -- from polluting the obarray with commands that by definition don't need to be there except perhaps (but that depends on the implementation) for the time the transient menus are available. in other words, this is a plain bug in my sight. We mustn't tell users of the Are you sure there's no way to do exactly what transient does today while keeping the API and avoiding this pollution?
Sure, take your time, and don't hold back on the technical arguments. |
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
|
Unlike @psionic-k does at 68568, I agree with @joaotavora, that Transient should actively prevent the commands, that are only intended to be used from a transient prefix, from being presented as completion choices in But I still think that the way to do that is by setting |
|
@tarsius I didn't understand you meant to set this user-visible variable from transient.el's . I'm fairly sure there are other ways to go about this, but fair enough. Anyway, if you want to do this, shouldn't you be using |
I was vaguely aware of this being a thing, but have never done it before, and am unclear on when one should and should not do that. Sounds like a good idea. (But it is more expensive.) So I would do this, right?: (defun transient--command-completion-not-suffix-only-p (fn symbol buffer)
"Return nil if SYMBOL is only useful as a transient suffix command.
Otherwise, if that is non-nil, call FN and return its value.
FN is the value of `read-extended-command-predicate'.
If that is nil, return t."
(cond ((eq (get symbol 'completion-predicate) 'transient--suffix-only)
nil)
(fn (funcall fn symbol buffer))
(t)))
(static-if (boundp 'read-extended-command-predicate)
(add-function :around read-extended-command-predicate
#'transient--command-completion-not-suffix-only-p))Hm, the Custom interface (and I would have hoped that the user would be able to change the "core" value of the option, without discarding the advise. I.e., it probably would be better if |
I had exactly the same expectation. Maybe @monnier can shine some light here. Maybe it's not hard to fix...
Yup. IIUC this is one of the things |
|
> I would have hoped that the user would be able to change the "core" value
> of the option, without discarding the advise
I had exactly the same expectation. Maybe @monnier can shine some light
here. Maybe it's not hard to fix...
The `C-h` output should show the `cl-print` pretty printed advice, which
is a bit better than the raw bytecode.
As for the user modifying this "core" value, while `nadvice.el` clearly
supports it for `advice-add` (via `defalias`), there isn't
a corresponding operation for `add-function`.
Instead, users would usually just *add* to the value with `:after-while`.
BTW, you can simplify your code to:
(defun transient--command-completion-not-suffix-only-p (symbol buffer)
(not (eq (get symbol 'completion-predicate) 'transient--suffix-only)))
(static-if (boundp 'read-extended-command-predicate)
(add-function :after-while read-extended-command-predicate
#'transient--command-completion-not-suffix-only-p))
BTW, I still don't see why those symbols need to be interned, so at the
very least, the code deserves a comment explaining why we have to do
that (and then have to do the `read-extended-command-predicate` dance)
[ Oh, BTW, the above code won't work because
`read-extended-command-predicate` defaults to nil (i.e. its type is
not "function" but (or function null)), which is an all too
common mistake. It should default to a function (e.g. `ignore`),
otherwise `add-function` can't be used with it. ]
|
I am glad you used the term "dance" here, because that's exactly how I feel about this request. I have already said that it would be possible to avoid interning these symbols. That is not actually true though -- but it would be possible to avoid making them fbound. Every "suffix" consists of two parts: a command and an object. These two sides of the suffix have to somehow be linked to each other, and I decided that I would use a symbol to achieve that.
However, because I wanted to avoid polluting No longer did Transient have to bend over backward to avoid polluting To get a symbol that can be used to locate the corresponding object, (defun transient--suffix-symbol (arg)
"Return a symbol representing ARG.
ARG must be a command and/or a symbol. If it is a symbol,
then just return it. Otherwise return the symbol whose
`transient--infix-command' property's value is ARG."
(or (cl-typep arg 'command)
(cl-typep arg 'symbol)
(signal 'wrong-type-argument `((command symbol) ,arg)))
(if (symbolp arg)
arg
(let* ((obj (transient-suffix-object))
(sym (oref obj command)))
(if (eq (get sym 'transient--infix-command) arg)
sym
(catch 'found
(mapatoms (lambda (sym)
(when (eq (get sym 'transient--infix-command) arg)
(throw 'found sym)))))))))Nearly all commands that change the value of some argument can be implemented exactly the same: (lambda ()
(interactive)
(let ((obj (transient-suffix-object)))
(transient-infix-set obj (transient-infix-read obj)))
(transient--show))But for that to work I think adding a little function to But let's assume for a moment that we go back to the old kludge anyway. Some infix argument commands are not defined anonymously within the definition of a prefix command, because they are shared between multiple prefixes. For example: (transient-define-argument magit:--gpg-sign ()
:description "Sign using gpg"
:class 'transient-option
:shortarg "-S"
:argument "--gpg-sign="
:allow-empty t
:reader #'magit-read-gpg-signing-key)This is used by Finally, I don't think that Transient defining commands that are only useful in the context of a certain transient prefix command (and nevertheless putting these commands in the global namespace), is all that different, from a package defining commands that are only useful in the context a certain major-mode (and nevertheless putting these commands in the global namespace). If it is acceptable for these major-mode specific commands to "pollute M-x", then shouldn't it be acceptable for the transient-prefix specific commands to do the same? The pollution is a problem -- in both cases -- and |
|
I am glad you used the term "dance" here, because that's exactly how
I feel about this request.
OK, reading&thinking some more, we shouldn't need to fiddle with the
global `read-extended-command-predicate`. Instead you should be able to
set your symbols' `completion-predicate` properties to something like `ignore`.
Every "suffix" consists of two parts: a command and an object. These
two sides of the suffix have to somehow be linked to each other, and
I have very early on decided that I would use a symbol to
achieve that.
[ Maybe I'm suffering from the "man with a hammer" syndrome, but the above
words make me think that maybe what you want is an OClosure,
i.e. something that's both a command and an object at the same time. ]
`(transient-suffix-object this-command)` and `(oref obj command)` are
used to look up the other side of the suffix. The object is stored in
a property I made up for that purpose, `transient--suffix`. For the
command I chose to use the symbol's function slot.
However, because I wanted to avoid polluting `M-x`, I did initially
use another slot for "anonymous" commands. I was not happy with that
from the start and only saw it as a temporary work-around --
a "dance" -- only to be used until a proper solution came along, which
eventually did: `read-extended-command-predicate`.
That still doesn't explain why you need them to be interned.
No longer did Transient have to bend over backward to avoid polluting
`M-x`. As mentioned before, a suffix has to sides the command and the
object, and they are linked via a symbol. While the old M-x-avoidance
hack was in place, a second dichotomy was necessary; the command
existed in a "call-interactive-able" form and in
a "symbol-for-identification" form.
To get a symbol that can be used to locate the corresponding object,
`this-command` had to be passed though this:
``` emacs-lisp
(defun transient--suffix-symbol (arg)
"Return a symbol representing ARG.
ARG must be a command and/or a symbol. If it is a symbol,
then just return it. Otherwise return the symbol whose
`transient--infix-command' property's value is ARG."
(or (cl-typep arg 'command)
(cl-typep arg 'symbol)
(signal 'wrong-type-argument `((command symbol) ,arg)))
(if (symbolp arg)
arg
(let* ((obj (transient-suffix-object))
(sym (oref obj command)))
(if (eq (get sym 'transient--infix-command) arg)
sym
(catch 'found
(mapatoms (lambda (sym)
(when (eq (get sym 'transient--infix-command) arg)
(throw 'found sym)))))))))
```
OK, this would indeed require the symbol to be interned for `mapatoms`
to find it, but AFAICT you don't use that code any more.
But for that to work `transient-suffix-object` has to be able to
determine the corresponding object. With the current implementation
that is done with `(get this-command 'transient--object)`.
But (get this-command 'transient--object) should work as well whether
`this-command` holds an interned symbol or non-interned one.
Finally, I don't think that Transient defining commands that are only
useful in the context of a *certain transient prefix command* (and
nevertheless putting these commands in the global namespace), is all
that different, from a package defining commands that are only useful
in the context a *certain major-mode* (and nevertheless putting these
commands in the global namespace).
Indeed. There's nothing fundamentally wrong with it.
It's just more often inconvenient for some end-users, maybe because of
the number of those commands, or their length (which makes it more
likely that they'll match some search pattern).
|
|
Okay. I'll experiment. |
See Emacs bug#68568
I don't use Magit. This is a just a fix for a very annoying bug where completion gets completely broken just because I use a package that uses transient.
YOu can push to my master of magit any time. The patch is trivial.