transient-append-suffix can't add multiple keys with different conditions

[sheijk]

When adding new keys using transient-append-suffix it is not possible to have the same key several times with different conditions:

(transient-define-prefix shk-bug-repro
  "Bug"
  ["Things"
   ("q" "quit" transient-quit-all)])

(transient-append-suffix 'shk-bug-repro (list 0 -1)
  '("s" "flyspell [on]" flyspell-mode :if (lambda () flyspell-mode)))
(transient-append-suffix 'shk-bug-repro (list 0 -1)
  '("s" "flyspell [off]" flyspell-mode :if-not (lambda () flyspell-mode)))

Now we have only one entry with a key called "s". See the output of (prog1 nil (pp (transient-get-suffix 'shk-bug-repro (list 0)))):

[1 transient-column
   (:description "Things")
   ((1 transient-suffix
       (:key "q" :description "quit" :command transient-quit-all))
    (1 transient-suffix
       (:key "s" :description "flyspell [off]" :command flyspell-mode :if-not
             (lambda nil flyspell-mode))))]

Doing the same using only transient-define-prefix is possible as running this code shows:

(transient-define-prefix shk-bug-repro
  "Bug"
  ["Things"
   ("q" "quit" transient-quit-all)
   ("s" "flyspell [on]" flyspell-mode :if (lambda () flyspell-mode))
   ("s" "flyspell [off]" flyspell-mode :if-not (lambda () flyspell-mode))])
(prog1 nil (pp (transient-get-suffix 'shk-bug-repro (list 0))))

producing the expected result

[1 transient-column
   (:description "Things")
   ((1 transient-suffix
       (:key "q" :description "quit" :command transient-quit-all))
    (1 transient-suffix
       (:key "s" :description "flyspell [on]" :command flyspell-mode :if
             (lambda nil flyspell-mode)))
    (1 transient-suffix
       (:key "s" :description "flyspell [off]" :command flyspell-mode :if-not
             (lambda nil flyspell-mode))))]

I would expect the two ways to define this menu to be equivalent.

The use case in this example might seem a bit hackish (using something as a suffix which might map better to an infix argument/toggle) but I find it useful for a quick sub mode to toggle minor modes where I'd prefer to not having to prefix everything with an extra "-". Another use case for several "overloads" of the same key would be org-mode where different functions need to be called to navigate depending on what kind of syntax we're on (table, header, etc.).

Transient version is transient-20201102.2024 (via Melpa), Emacs is 27.1 (up to date Manjaro Linux), Magit (if it matters?) is magit-20201116.1057 (via Melpa)

[sheijk]

It looks like this might be related to the change done for bug #58. The check for duplicate keys which seems to cause the issue is at 1054 in transient--insert-suffix:

          (if (equal (transient--kbd key)
                     (transient--kbd (transient--spec-key elt)))
              (setq action 'replace)

[tarsius]

Thanks for a nice bug report. I especially appreciate that you posted a test case.

It looks like this might be related to the change done for bug #58.

It might be related in the sense that this commit fixes and/or adds bugs which may affect how this function behaves with regards to what we are discussing here, but if that is so, then that was unintentional at the time.

Going throw the trace log of this function is quite interesting but also embarrassing.

I would expect the two ways to define this menu to be equivalent.

That's a reasonable expectation and I agree but unfortunately this conflicts with another reasonable (and older) expectation. In other words things are actually behaving as expected and I am not sure how to reconcile the two conflicting expectations.

Initially "conflicting" bindings were not allowed, there were no :if* predicates or some other mechanism to resolve such conflicts. transient--insert-suffix tried to detect such conflicts from the very beginning. Briefly the conflicts were detected by comparing the bound commands. That probably was unintentional and was switched to detecting key conflicts instead.

How the old conflicting binding was removed has changed a few times to handle edge-cases and fix bugs that were introduced in the process.

But "conflicting bindings" were intentionally removed from the very beginning (even when using the insert/append commands, not just when using the replace command). The reason is documented in one of the commits that touch this function:

When inserting a suffix and another suffix is bound to same key, then
remove the other suffix. I.e. more or less do what define-key would
do.

By now (seemingly) conflicting bindings are allowed, but that does not mean that any such shared binding is intentional, some are real conflicts and I don't know yet how to tell them apart.

For example the very same transient-insert-suffix form could be evaluated twice, in which the duplication would be unwelcome. (Of course this particular case could be detected by checking whether the two bindings are identical; but this may not always be the case.)

[sheijk]

I see. How about adding another possible flag for this? Like
("s" "flyspell [on]" flyspell-mode :allow-conflicting-keys)?

[tarsius]

I've decided to go with a heuristic for now.