bug-gnu-emacs
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Mauro Aranda
Subject: bug#139: describe-key vs. widget red tape
Date: Tue, 8 Oct 2019 10:04:13 -0300

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> Find in the attached patch my idea to implement this.
>
> Thank you for working on this!

Thanks for the review!

>> 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?

Perhaps.  I didn't feel it was obvious, so I made some effort in
describing it in the doc string.  But maybe there's a better name out
there.

>> 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?

I like this idea! I think I was a little short-sighted focusing only in
the actions, because there is already a command for describing a
widget.  
But, as with describe-actions, I can't find a good name for it.  I think
describe-buttons wouldn't make justice to the widgets that are not
buttons (e.g., editable fields), and describe widgets makes it sound
like it doesn't include other libraries...

>> 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.

OK, thanks.

>> 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.

Didn't know about the '-internal' suffix, thanks.  I'll change it.

>> +  "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?

Yes, I'll fix it.

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

Right, looks much better.  Thank you.

>> +      (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.

What happened was that I developed the initial draft out of tree.  I'll
be more careful next time.

>> + (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.

I'm not sure: other help functions, like describe-function or
describe-key don't do that.  Maybe they should too?

>> +      (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.

Right, I'll fix it.

>> +  (princ action)))
>> +      (when (functionp mouse-down-action)
>
> There should be a newline or two between action and mouse-down-action.

Yes, I forgot to add those newlines.  I'll fix it.

>> + (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))))))))

Totally! I'll adapt it to something like that.

>> +(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.

Thanks, I'll fix those.

>> +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.

I know that, but I put the two checks because I had found a problem with
the `forward' and `back' buttons of the *Help* buffer.  However, it
looks like I can trigger the problem (or a similar one, I'm not sure)
even with the way I wrote it, so I'll have to investigate it further.

>> +      ((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)))

Yes, it is much better, thank you.

>> 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))))))
>> +
>> +;; 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?

Keep in mind that I started with `widget-describe-actions', and because
I developed that as a command, I thought it was useful to accept a nil
argument defaulting to point.  I like it better.

But yes, there is no problem in passing them always a buffer position.

>> +;;;###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".

Typo.  I'll fix it.

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

That's better, yes.  I'll change it.

>> +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?
>

I missed that, silly me.  I'm not sure what would be better, but I would
like to describe the clicked element (if any), even if it's not in the
selected 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?

I don't think so:  It has a prompt that would not be useful here, and
for the expected input, it seems a little overkill.  But, I might be
short-sighted here too...

>> +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).

Yes, don't know why I missed that.  I'll fix it.

> [...]
>
>> +(defun widget-resolve-parent-action (widget)
>
> Should this function have a name with a double hyphen '--'?

Yes, I'll do it.

>> + "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))

Yes, I had something like that in my draft, and for some reason I
changed it and ended up with a buggy version.  Thanks for spotting
that, I'll revert it to my original version but keep the doc string you
suggested, because it is much better.

Thank you again for your review, it was very helpful.  I hope I'll find
time in the next days to keep working on it.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]