bug#139: describe-key vs. widget red tape

["Basil L. Contovounesios" <contovob@tcd.ie>]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Find in the attached patch my idea to implement this.

Thank you for working on this!

> I wrote a command, describe-actions, so the user does not have to care
> if the element to describe is either a button, a widget, or whatever.

Isn't the name describe-actions too vague/ambiguous?

> And then, the libraries that define this kind of elements should add
> themselves to `describe-actions-functions'.

SGTM, but what if, in the future, we decide we want to describe more
properties of these elements?  Instead of specifically describing
"actions", why not "describe buttons", "describe widgets", or "describe
button-like elements" (i.e. both buttons and widgets) in general,
leaving the precise properties described open to future change?

> Then I implemented the functions for the button.el buttons and the
> widgets.
>
> There are some things I would like to ask:
> - I would like to put an initial value of nil to
> `describe-actions-functions', and let each library add the function to
> this list.  Is it OK to use `with-eval-after-load' here?

I think it's best to avoid it, especially in preloaded libraries like
button.el.

> Is there another way?

I think the way it is now is fine.  Alternatively, you could move the
button- and widget-specific code to help-fns.el, but I can't really tell
if either approach is superior.

> - I put an autoload cookie in `seq-find', in order to use it in
> `describe-actions'.  Is it OK?

I think so, but you could also (require 'seq) where it's needed or use
the already loaded cl-lib library instead.

[...]

> diff --git a/lisp/button.el b/lisp/button.el
> index 04e77ca..66bc12f 100644
> --- a/lisp/button.el
> +++ b/lisp/button.el
> @@ -538,6 +538,64 @@ backward-button
>    (interactive "p\nd\nd")
>    (forward-button (- n) wrap display-message no-error))
>  
> +(defun button--describe-actions-internal (type action mouse-down-action)

The internal nature of the function is already conveyed by the double
hyphen '--' in its name, so there is no need for the '-internal' suffix,
which is usually used in C definitions.

> +  "Describe a button's TYPE, ACTION and MOUSE-DOWN-ACTION in a *Help* buffer.
> +This is a helper function for `button-describe-actions', in order to be possible
> +to use `help-setup-xref'."
> +  (with-help-window (help-buffer)
> +    (with-current-buffer (help-buffer)
> +      (insert "This button's type is ")
> +      (princ type)
> +      (insert "\n\n")

Shouldn't this sentence end with a full stop?

Perhaps the type should also be `quoted' and passed through
format-message or similar?

> +      (when (functionp action)
> +	(insert (propertize "Action" 'face 'bold))

According to the top-level dir-locals-file in the Emacs repository,
Elisp should be indented with indent-tabs-mode disabled.

> +	(insert "\nThe action of this button is ")
> +	(if (symbolp action)
> +	    (progn
> +	      (princ action)

This function symbol should probably be `quoted' and passed through
format-message or similar.

> +	      (insert ",\nwhich is ")
> +	      (describe-function-1 action))
> +	  (insert "\n")

If 'action' is not a symbol, then the previous call to 'insert' will
leave a trailing space character before this newline.

> +	  (princ action)))
> +      (when (functionp mouse-down-action)

There should be a newline or two between action and mouse-down-action.

> +	(insert (propertize "Mouse-down-action" 'face 'bold))
> +	(insert "\nThe mouse-down-action of this button is ")
> +	(if (symbolp mouse-down-action)
> +	    (progn
> +	      (princ mouse-down-action)
> +	      (insert ",\nwhich is ")
> +	      (describe-function-1 mouse-down-action))
> +	  (insert "\n")
> +	  (princ mouse-down-action))))))

The logic is identical for action and mouse-down-action, and the
argument list of this function feels a bit too rigid.  Why not accept an
alist of useful properties as a single argument instead?  Then you could
write something like the following:

  (defun button--describe-actions (properties)
    "Describe a button's PROPERTIES (an alist) in a *Help* buffer.
  This is a helper function for `button-describe-actions', intended
  to be used with `help-setup-xref'."
    (with-help-window (help-buffer)
      (with-current-buffer (help-buffer)
        (insert (format-message "This button's type is `%s'."
                                (alist-get 'type properties)))
        (dolist (prop '(action mouse-down-action))
          (let ((name (symbol-name prop))
                (val  (alist-get prop properties)))
            (when (functionp val)
              (insert "\n\n"
                      (propertize (capitalize name) 'face 'bold)
                      "\nThe " name " of this button is")
              (if (symbolp val)
                  (progn
                    (insert (format-message " `%s',\nwhich is " val))
                    (describe-function-1 val))
                (insert "\n")
                (princ val))))))))

> +(defun button-describe-actions (&optional button-or-pos)
> +  "Describe the actions associated to the button at point.
                           ^^^^^^^^^^^^^
"associated with", here and in other places below.

> +Displays a *Help* buffer with a description of the actions.
> +
> +When called from Lisp, pass BUTTON-OR-POS as the button to describe, or a
> +buffer position where a button is present.  If BUTTON-OR-POS is nil, the
> +button at point is the button to describe."
> +  (interactive "d")
> +  (let ((button (cond ((numberp button-or-pos)
> +		       (button-at button-or-pos))
> +		      ((markerp button-or-pos)
> +		       (with-current-buffer (marker-buffer button-or-pos)
> +			 (button-at button-or-pos)))

These two clauses can be joined using integer-or-marker-p, since
button-at accepts either type of buffer position.

> +		      ((null button-or-pos)
> +		       (button-at (point)))
> +		      (t
> +		       button-or-pos)))
> +	action mouse-down-action type)
> +    (when button
> +      (setq type (button-type button)
> +            action (button-get button 'action)
> +	    mouse-down-action (button-get button 'mouse-down-action))
> +      (help-setup-xref
> +       (list #'button--describe-actions-internal type action mouse-down-action)
> +       (called-interactively-p 'interactive))
> +      (button--describe-actions-internal type action mouse-down-action)
> +      t)))

As suggested previously, why not pass all interesting properties to the
helper function as a single argument?  For example:

  (defun button-describe-actions (&optional button-or-pos)
    "Describe the actions associated with the button at point.
  Displays a *Help* buffer with a description of the actions.

  When called from Lisp, BUTTON-OR-POS can be a buffer position
  where a button (including overlay buttons) is present, a button
  object (e.g., an overlay), or nil, meaning the button at point."
    (interactive "d")
    (let* ((button (cond ((integer-or-marker-p button-or-pos)
                          (button-at button-or-pos))
                         ((null button-or-pos)
                          (button-at (point)))
                         (t
                          button-or-pos)))
           (props (and button
                       (mapcar (lambda (prop)
                                 (cons prop (button-get button prop)))
                               '(type action mouse-down-action)))))
      (when props
        (help-setup-xref (list #'button--describe-actions props)
                         (called-interactively-p 'interactive))
        (button--describe-actions props)
        t)))

>  (provide 'button)
>  
>  ;;; button.el ends here

[...]

> diff --git a/lisp/help-fns.el b/lisp/help-fns.el
> index e9e2818d..e9d2139 100644
> --- a/lisp/help-fns.el
> +++ b/lisp/help-fns.el
> @@ -1530,6 +1530,48 @@ describe-categories
>  	(while (setq table (char-table-parent table))
>  	  (insert "\nThe parent category table is:")
>  	  (describe-vector table 'help-describe-category-set))))))
> +\f
> +;; Actions.
> +
> +(defvar describe-actions-functions '(button-describe-actions
> +                                     widget-describe-actions)
> +  "A list of functions for `describe-actions' to call.
> +Each function should take one argument, a position in the buffer, and return
> +non-nil if it described the actions of an element at that position.
> +The argument passed might be nil, which indicates to describe the actions of
> +the element at point.")

Why should these hook functions accept a nil argument?
Why not always call them with a buffer position as argument?

> +;;;###autoload
> +(defun describe-actions (&optional pos)
> +  "Describe the actions associated to an element at a buffer position POS.
> +Actions are functions that get executed when the user activates the element,
> +by clicking on it, or pressing a key.  Typically, actions are associated to
> +a button (e.g., links in a *Help* buffer) or a widget (e.g., buttons, links,
> +editable fields, etc., of the customization buffers).
> +
> +Interactively, click on an element to describe its actions, or hit RET
> +to describe the actions of the element at point.
> +
> +When called from Lisp, POS may be a buffer position, or nil, to describe the
> +actions of the element at point.
> +
> +Traverses the list `describe-action-functions', until one of the functions
                                     ^^
Elsewhere you say "actions" rather than "action".

Instead of "traverses the list" I would say more specifically "runs the
hook" or "calls each function in turn".

> +returns non-nil."
> +  (interactive
> +   (list
> +    (let ((key
> +           (read-key
> +	    "Click an element, or hit RET to describe the element at point")))
> +      (cond ((eq key ?\C-m) nil)
> +            ((and (mouse-event-p key)
> +                  (eq (event-basic-type key) 'mouse-1)
> +                  (equal (event-modifiers key) '(click)))
> +             (posn-point (event-end key)))

What if the click is in a different window?

> +            ((eq key ?\C-g) (signal 'quit nil))
> +            (t (user-error "You didn't specify an element"))))))

I wonder if we can reuse help--read-key-sequence or similar here?

> +  (unless (seq-find (lambda (fun) (when (fboundp fun) (funcall fun pos)))
> +		    describe-actions-functions)
> +    (message "No actions here")))
>  
>  \f
>  ;;; Replacements for old lib-src/ programs.  Don't seem especially useful.
> diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
> index 916d41a..f8f485a 100644
> --- a/lisp/wid-edit.el
> +++ b/lisp/wid-edit.el
> @@ -586,6 +586,70 @@ widget-map-buttons
>        (if (and widget (funcall function widget maparg))
>  	  (setq overlays nil)))))
>  
> +(defun widget-describe-actions (&optional widget-or-pos)
> +  "Describe the actions associated to the widget at point.
> +Displays a buffer with a description of the actions, as well as a link to
> +browse all the properties of the widget.
> +This command resolves the indirection of widgets running the action of its
> +parents, so the real action executed can be known.
> +
> +When called from Lisp, pass WIDGET-OR-POS as the widget to describe,
> +or a buffer position where a widget is present.  If WIDGET-OR-POS is nil,
> +the widget at point is the widget to describe."
> +  (interactive "d")
> +  (require 'wid-browse)
> +  (let ((widget (if (widgetp widget-or-pos)
> +		    widget-or-pos
> +		  (widget-at (or widget-or-pos (point)))))

widget-at accepts nil, so you can just write (widget-at widget-or-pos).

[...]

> +(defun widget-resolve-parent-action (widget)

Should this function have a name with a double hyphen '--'?

> +  "If action of WIDGET is `widget-parent-action', find out what would that be."
> +  (let ((action (widget-get widget :action))
> +	(parent (widget-get widget :parent)))
> +    (while (and (eq action 'widget-parent-action)
> +		(setq parent (widget-get parent :parent)))
> +      (setq action (widget-get parent :action)))
> +    action))

I think there's a bug here.  Assuming WIDGET's :action is
'widget-parent-action', we get the following:

0. ACTION is set to widget-parent-action
1. PARENT is set to the parent of WIDGET
2. (eq action ...) is true
3. PARENT is set to its parent

Instead of looking at WIDGET's parent, we are now looking at its
grandparent.  Shouldn't we do the following instead?

  (defun widget--resolve-parent-action (widget)
    "Resolve the real action of WIDGET up its inheritance chain.
  Follow the :parents of WIDGET until its :action is no longer
  `widget-parent-action', and return its value."
    (let ((action (widget-get widget :action)))
      (while (eq action #'widget-parent-action)
        (setq widget (widget-get widget :parent))
        (setq action (widget-get widget :action)))
      action))

Thanks,

-- 
Basil