[PATCH] speed commands: error message when a key is not associated with a command

[PATCH] speed commands: error message when a key is not associated with a command

[Juan Manuel Macías]

[-- Attachment #1: Type: text/plain, Size: 387 bytes --]

Hi,

If we have speed commands activated and we type (by mistake) a character
not associated with a command, the letter is printed at point. I think a
more appropriate behavior would be:

- key associated with a command: the command is activated

- key not associated with a command: an error message is displayed.

Attached a patch with that modification.

Best regards,

Juan Manuel 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org-keys.el-error-message-when-speed-command-is.patch --]
[-- Type: text/x-patch, Size: 1489 bytes --]

From fd380f41420aef9bc91baede9d4cd2cb7eadee0a Mon Sep 17 00:00:00 2001
From: Juan Manuel Macias <maciaschain@posteo.net>
Date: Sat, 30 Apr 2022 13:00:02 +0200
Subject: [PATCH] lisp/org-keys.el: error message when speed command is not
 defined

* (org-speed-command-activate): If a letter is not associated with a
command, an error message is displayed instead of typing the letter.
---
 lisp/org-keys.el | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/lisp/org-keys.el b/lisp/org-keys.el
index 782ffa871..a850abb4f 100644
--- a/lisp/org-keys.el
+++ b/lisp/org-keys.el
@@ -835,12 +835,14 @@ See `org-speed-commands' for configuring them."
   (when (or (and (bolp) (looking-at org-outline-regexp))
 	    (and (functionp org-use-speed-commands)
 		 (funcall org-use-speed-commands)))
-    (cdr (assoc keys
-                ;; FIXME: don't check `org-speed-commands-user' past 9.6
-                (if (boundp 'org-speed-commands-user)
-                    (append org-speed-commands
-                            org-speed-commands-user)
-                  org-speed-commands)))))
+    (let ((defined (cdr (assoc keys
+			       ;; FIXME: don't check `org-speed-commands-user' past 9.6
+			       (if (boundp 'org-speed-commands-user)
+				   (append org-speed-commands
+					   org-speed-commands-user)
+				 org-speed-commands)))))
+      (if defined defined
+	(error "Speed command not defined: \"h\" for help")))))
 
 \f
 ;;; Babel speed keys
-- 
2.36.0

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Ihor Radchenko]

Juan Manuel Macías <maciaschain@posteo.net> writes:

> If we have speed commands activated and we type (by mistake) a character
> not associated with a command, the letter is printed at point. I think a
> more appropriate behavior would be:
>
> - key associated with a command: the command is activated
>
> - key not associated with a command: an error message is displayed.

The idea is not bad, but I doubt that it can be implemented cleanly
given the current speed commands design.

> * (org-speed-command-activate): If a letter is not associated with a
> command, an error message is displayed instead of typing the letter.

Note that speed commands are not only decided by
org-speed-command-activate. Any function in org-speed-command-hook can
trigger speed command. Throwing an error in org-speed-command-activate
can potentially shadow other functions in the hook.

Best,
Ihor

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Juan Manuel Macías]

Ihor Radchenko writes:

> Note that speed commands are not only decided by
> org-speed-command-activate. Any function in org-speed-command-hook can
> trigger speed command. Throwing an error in org-speed-command-activate
> can potentially shadow other functions in the hook.

Ah, I see... I had not taken into account what you mention.

But, if I have understood correctly how this hook works, each associated
function has its "independence", right? I mean, if I have
org-speed-command-activate and org-babel-speed-command-activate
associated to this hook, and I bind the "K" key to an action in
org-babel-key-bindings, but that key does not is associated with any
action in org-speed-commands, then the error message would only be
displayed in the proper context, that is, if I hit K at the beginning of
the headline or any location defined for org-use-speed-commands.

Another possibility I can think of is, instead of returning an error
message: just do nothing when a wrong key is pressed. Something, maybe,
like this (I suppose that the same should be done in each function added
to the hook):

...
(let ((defined (cdr (assoc keys
                               ;; FIXME: don't check `org-speed-commands-user' past 9.6
                               (if (boundp 'org-speed-commands-user)
                                   (append org-speed-commands
                                           org-speed-commands-user)
                                 org-speed-commands)))))
      (if defined defined
        'ignore))))

I don't know if this would avoid unexpected results...

what do you think?

Best regards,

Juan Manuel

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Juan Manuel Macías]

> Ihor Radchenko writes:
>
>> Note that speed commands are not only decided by
>> org-speed-command-activate. Any function in org-speed-command-hook can
>> trigger speed command. Throwing an error in org-speed-command-activate
>> can potentially shadow other functions in the hook.

This other, more general solution has also occurred to me, although I
don't know if it's too tricky :-). The idea is to define an
org-speed-command-strict-hook. Something like this (I haven't tested it
much), and probably have its drawbacks:

#+begin_src emacs-lisp
  (defcustom org-speed-command-strict-hook
    nil
    "TODO"
    :group 'org-structure
    :version "24.1"
    :type 'hook)

  (defun org-speed-command-strict-function (_)
    (pcase (run-hook-with-args-until-success 'org-speed-command-strict-hook _)
      ('nil (error "Command not defined: \"?\" for help"))
      (symbol (run-hook-with-args-until-success 'org-speed-command-strict-hook _))))

  ;; no strict speed commands (default values):

    ;; org-speed-command-strict-hook: nil
    ;; org-speed-command-hook: (org-speed-command-activate org-babel-speed-command-activate)

 ;; when strict speed commands:

  (setq org-speed-command-strict-hook '(org-speed-command-activate org-babel-speed-command-activate))

  (setq org-speed-command-hook '(org-speed-command-strict-function))

  (defun my-org-toggle-speed-commands ()
    (interactive)
    (if org-use-speed-commands
	(progn
	  (setq org-use-speed-commands-on org-use-speed-commands)
	  (setq org-use-speed-commands nil)
	  (message "speed commands off"))
      (setq org-use-speed-commands org-use-speed-commands-on)
      (message "speed commands on")))
#+end_src

Best regards,

Juan Manuel 

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Ihor Radchenko]

Juan Manuel Macías <maciaschain@posteo.net> writes:

> Ihor Radchenko writes:
>
>> Note that speed commands are not only decided by
>> org-speed-command-activate. Any function in org-speed-command-hook can
>> trigger speed command. Throwing an error in org-speed-command-activate
>> can potentially shadow other functions in the hook.
>
> Ah, I see... I had not taken into account what you mention.
>
> But, if I have understood correctly how this hook works, each associated
> function has its "independence", right? I mean, if I have
> org-speed-command-activate and org-babel-speed-command-activate
> associated to this hook, and I bind the "K" key to an action in
> org-babel-key-bindings, but that key does not is associated with any
> action in org-speed-commands, then the error message would only be
> displayed in the proper context, that is, if I hit K at the beginning of
> the headline or any location defined for org-use-speed-commands.

You are mostly correct, with one potential caveat.
Consider a user who wants to define extra speed command that should run
at the beginning of level 1 headlines. This context also matches the
context of org-babel-speed-command-activate and your proposed patch will
make it harder for the user to achieve the described behaviour.

On the other hand, the user could simply add to front of the
org-speed-command-hook given that blocking behaviour of
org-babel-speed-command-activate is well-documented.

> Another possibility I can think of is, instead of returning an error
> message: just do nothing when a wrong key is pressed. Something, maybe,
> like this (I suppose that the same should be done in each function added
> to the hook):

This would not solve the problem of shadowing.
It may be better idea to provide a custom variable controlling
org-babel-speed-command-activate: do nothing or throw an error.
This custom variable should also be described in the docstring of
org-speed-command-hook to warn about potential shadowing.

To summarise, your idea will be reasonable if:
1. The new behaviour can be customized
2. The new behaviour is documented in org-speed-command-hook

Best,
Ihor

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Juan Manuel Macías]

Ihor Radchenko writes:

> This would not solve the problem of shadowing.
> It may be better idea to provide a custom variable controlling
> org-babel-speed-command-activate: do nothing or throw an error.
> This custom variable should also be described in the docstring of
> org-speed-command-hook to warn about potential shadowing.
>
> To summarise, your idea will be reasonable if:
> 1. The new behaviour can be customized
> 2. The new behaviour is documented in org-speed-command-hook

Ok, I agree that this is the most reasonable direction. What do you
think about the idea that I outlined in the last post of this thread
(https://list.orgmode.org/87v8uqmkc0.fsf@posteo.net/)?: it consists in
defining a new hook (by default with value nil) where the user can store
those functions that he wants to have a 'strict' behavior: some functions
or all. And then the user should add the
org-speed-command-strict-function to org-speed-command-hook.

If this idea sounds too hacky, I think a defcustom for
org-babel-speed-command-activate, as you suggest, might suffice.

Best regards,

Juan Manuel 

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Ihor Radchenko]

Juan Manuel Macías <maciaschain@posteo.net> writes:

> Ok, I agree that this is the most reasonable direction. What do you
> think about the idea that I outlined in the last post of this thread
> (https://list.orgmode.org/87v8uqmkc0.fsf@posteo.net/)?: it consists in
> defining a new hook (by default with value nil) where the user can store
> those functions that he wants to have a 'strict' behavior: some functions
> or all. And then the user should add the
> org-speed-command-strict-function to org-speed-command-hook.
>
> If this idea sounds too hacky, I think a defcustom for
> org-babel-speed-command-activate, as you suggest, might suffice.

It is more complex and I do not see a clear benefit of introducing a
whole new hook. It would only be useful for people who define a large
number of extra speed command handlers.

Disclaimer: I do not use speed commands myself, so my opinion is
somewhat theoretical. If other users who are actively using speed
command hook have something to say, I'd encourage them to join the
discussion.

Best,
Ihor

Re: [PATCH] speed commands: error message when a key is not associated with a command

[Juan Manuel Macías]

Ihor Radchenko writes:

> It is more complex and I do not see a clear benefit of introducing a
> whole new hook. It would only be useful for people who define a large
> number of extra speed command handlers.

Yes, I agree. Also I'm afraid that this idea of mine added a drawback as
a bonus track: it would be necessary to modify all the functions that
activate the speed commands because, otherwise, it would return an error
anywhere outside the activation contexts. Precisely because of this:

    (pcase (run-hook-with-args-until-success 'org-speed-command-strict-hook _)
      ('nil (error "Command not defined: \"?\" for help"))
      (symbol (run-hook-with-args-until-success
      'org-speed-command-strict-hook _)))

And I suppose that the functions should consist of a conditional of the
type: "if the context is given -> activate the speed commands, otherwise
-> self-insert-command". Or something like that. Too hacky.

On the other hand, I now seriously doubt whether a patch would be worth
it, even if a defcustom was added. I think you're also right about what
you said at the beginning: considering how speed commands are designed,
it's hard to implement a clean, general solution. I think I'm going to
leave this patch in the fridge for now, though I also encourage other
potential users of speed commands to join the discussion.

(Perhaps, instead of a patch, some tips could be added to the manual (or
to any other relevant site) so that users who want to extend this
functionality apply this type of hacks, as I have done in my particular
case...).

Best regards,

Juan Manuel