From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: Mauro Aranda Newsgroups: gmane.emacs.bugs Subject: bug#139: describe-key vs. widget red tape Date: Tue, 8 Oct 2019 10:04:13 -0300 Message-ID: References: <87mynwgrxb.fsf@jidanni.org> <877e5ql42d.fsf@gnus.org> <8336gedyus.fsf@gnu.org> <874l0ugrl4.fsf@gnus.org> <83sgoechsf.fsf@gnu.org> <87v9t9bzng.fsf@gnus.org> <874l0s8x8s.fsf@gnus.org> <878spyemgo.fsf@tcd.ie> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="000000000000dc928a059465ce0b" Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="161612"; mail-complaints-to="usenet@blaine.gmane.org" Cc: 139@debbugs.gnu.org, Lars Ingebrigtsen , jidanni@jidanni.org To: "Basil L. Contovounesios" Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Tue Oct 08 15:06:43 2019 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.89) (envelope-from ) id 1iHpCM-000fo5-4X for geb-bug-gnu-emacs@m.gmane.org; Tue, 08 Oct 2019 15:06:42 +0200 Original-Received: from localhost ([::1]:55440 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iHpCK-0001ug-V6 for geb-bug-gnu-emacs@m.gmane.org; Tue, 08 Oct 2019 09:06:40 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:37772) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iHpAs-0000es-Ip for bug-gnu-emacs@gnu.org; Tue, 08 Oct 2019 09:05:15 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iHpAk-0000zt-7P for bug-gnu-emacs@gnu.org; Tue, 08 Oct 2019 09:05:10 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:40909) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iHpAj-0000zJ-WA for bug-gnu-emacs@gnu.org; Tue, 08 Oct 2019 09:05:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1iHpAj-0005jH-Nm for bug-gnu-emacs@gnu.org; Tue, 08 Oct 2019 09:05:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Mauro Aranda Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 08 Oct 2019 13:05:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 139 X-GNU-PR-Package: emacs Original-Received: via spool by 139-submit@debbugs.gnu.org id=B139.157053987721989 (code B ref 139); Tue, 08 Oct 2019 13:05:01 +0000 Original-Received: (at 139) by debbugs.gnu.org; 8 Oct 2019 13:04:37 +0000 Original-Received: from localhost ([127.0.0.1]:49730 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iHpAJ-0005ia-Mk for submit@debbugs.gnu.org; Tue, 08 Oct 2019 09:04:36 -0400 Original-Received: from mail-lf1-f41.google.com ([209.85.167.41]:36644) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iHpAG-0005iM-U7 for 139@debbugs.gnu.org; Tue, 08 Oct 2019 09:04:34 -0400 Original-Received: by mail-lf1-f41.google.com with SMTP id x80so11897304lff.3 for <139@debbugs.gnu.org>; Tue, 08 Oct 2019 06:04:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=14XHPTOPnsAMU5zH6VYWGSlzOyPx4O3BgLonDq5QX5Q=; b=qfLWdM8FEEDQ6VgaqOWazHMixTSyKiO9Au7wdMTvH3Vo+YdXnp6mkwQCbb3bY6Iesp U/MlPSa3uE3CgDoYt/VadZhYRFz6EspgF5Mk+Iv629szgx072aLPI7XF1T+xp7m2j2iq 40E7oeWBrHUuGiOPdqJGl93QGeBa/GAGnYAsU/uvd7pWpwQE3fL+zpeOcqxe5IPHiAKg SLibAZRb/EPZ2zfxeFQLrxhHmTL6s3Y2VhnnRDbcLHcov6O36vB4XdMruHiQMBaToktE 40SZmgxtV/xIVZhYqN+rGFGcLLpRt/Kw+wtHaM8Qs9d98oZtAJFQuKtSKxJtq9VTr1Pr xLVw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=14XHPTOPnsAMU5zH6VYWGSlzOyPx4O3BgLonDq5QX5Q=; b=ZI3ZYjSE0bbKhAnHNHF09K+uyFCZZMf+qUZc+6EdGp+x6nTjVGIOLPWR+FawNcgE30 dbe47Ng1WQqbL8iFIlZVULzjWq9dbVBL6gR1QJhso9OrzpQO7tMhFWW3PGAXUFbX90rN IUMwuDW/wjTKfrZoQMpfkyokDXYUcBk49ZieYeRYj3eriSbm7tAD3zA74A76ndcrhCs5 ryhYYGn1s3pq3+Kb9nZuO497Dc9UebZVdkSv8Ui88FfcQdC6IvgjXKWJVIoJhAf3Z6kJ ZqDWsJAGN/lAj8jT+qZUl0R/x55iFZYbPgexzWoPKaJG5eIzkNxdwyqRDre8ULOpRZ6Y LQdQ== X-Gm-Message-State: APjAAAWYqiSmXP0EEhyWB5hEwO8aF7pKely7lU+oZeAmC9z+kgtU/9SI iKHgxzx3detTOZAfyk1Hyaz/B+U7wYh+T6cuGCE= X-Google-Smtp-Source: APXvYqwmbfGhoMzA6m3X0tlB51cqj7oKmIuMg/UVbFMVEanaZMI/PfEpV3H4IxHdU8ztC4V69LRprgGSeCJ8aroy9kc= X-Received: by 2002:ac2:4253:: with SMTP id m19mr20432804lfl.34.1570539866721; Tue, 08 Oct 2019 06:04:26 -0700 (PDT) In-Reply-To: <878spyemgo.fsf@tcd.ie> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 209.51.188.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:168646 Archived-At: --000000000000dc928a059465ce0b Content-Type: text/plain; charset="UTF-8" "Basil L. Contovounesios" writes: > Mauro Aranda 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. --000000000000dc928a059465ce0b Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Mauro Aranda &= lt;maurooaranda@gmail.com>= writes:
>
>> Find in the attached patch my idea to implemen= t this.
>
> Thank you for working on this!

Thanks for th= e review!

>> I wrote a command, describe-actions, so the user = does not have to care
>> if the element to describe is either a bu= tton, a widget, or whatever.
>
> Isn't the name describe-ac= tions too vague/ambiguous?

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

>> And then,= the libraries that define this kind of elements should add
>> the= mselves to `describe-actions-functions'.
>
> SGTM, but what= if, in the future, we decide we want to describe more
> properties o= f these elements?=C2=A0 Instead of specifically describing
> "ac= tions", why not "describe buttons", "describe widgets&q= uot;, or "describe
> button-like elements" (i.e. both butto= ns and widgets) in general,
> leaving the precise properties describe= d open to future change?

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

>> Is there another way?
>
> = I think the way it is now is fine.=C2=A0 Alternatively, you could move the<= br>> button- and widget-specific code to help-fns.el, but I can't re= ally tell
> if either approach is superior.

OK, thanks.
>> diff --git a/lisp/button.el b/lisp/button.el
>> index 04= e77ca..66bc12f 100644
>> --- a/lisp/button.el
>> +++ b/li= sp/button.el
>> @@ -538,6 +538,64 @@ backward-button
>> = =C2=A0 =C2=A0(interactive "p\nd\nd")
>> =C2=A0 =C2=A0(fo= rward-button (- n) wrap display-message no-error))
>> =C2=A0
&g= t;> +(defun button--describe-actions-internal (type action mouse-down-ac= tion)
>
> The internal nature of the function is already convey= ed by the double
> hyphen '--' in its name, so there is no ne= ed for the '-internal' suffix,
> which is usually used in C d= efinitions.

Didn't know about the '-internal' suffix, th= anks.=C2=A0 I'll change it.

>> + =C2=A0"Describe a bu= tton's TYPE, ACTION and MOUSE-DOWN-ACTION in a *Help* buffer.
>&g= t; +This is a helper function for `button-describe-actions', in order>> to be possible
>> +to use `help-setup-xref'."<= br>>> + =C2=A0(with-help-window (help-buffer)
>> + =C2=A0 = =C2=A0(with-current-buffer (help-buffer)
>> + =C2=A0 =C2=A0 =C2=A0= (insert "This button's type is ")
>> + =C2=A0 =C2=A0= =C2=A0(princ type)
>> + =C2=A0 =C2=A0 =C2=A0(insert "\n\n&qu= ot;)
>
> Shouldn't this sentence end with a full stop?
<= br>Yes, I'll fix it.

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

Right, = looks much better.=C2=A0 Thank you.

>> + =C2=A0 =C2=A0 =C2=A0(= when (functionp action)
>> + (insert (propertize "Action"= ; 'face 'bold))
>
> According to the top-level dir-loca= ls-file in the Emacs repository,
> Elisp should be indented with inde= nt-tabs-mode disabled.

What happened was that I developed the initia= l draft out of tree.=C2=A0 I'll
be more careful next time.

&g= t;> + (insert "\nThe action of this button is ")
>> += (if (symbolp action)
>> + =C2=A0 =C2=A0(progn
>> + =C2= =A0 =C2=A0 =C2=A0(princ action)
>
> This function symbol should= probably be `quoted' and passed through
> format-message or simi= lar.

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

&= gt;> + =C2=A0 =C2=A0 =C2=A0(insert ",\nwhich is ")
>>= ; + =C2=A0 =C2=A0 =C2=A0(describe-function-1 action))
>> + =C2= =A0(insert "\n")
>
> If 'action' is not a sym= bol, then the previous call to 'insert' will
> leave a traili= ng space character before this newline.

Right, I'll fix it.
<= br>>> + =C2=A0(princ action)))
>> + =C2=A0 =C2=A0 =C2=A0(wh= en (functionp mouse-down-action)
>
> There should be a newline = or two between action and mouse-down-action.

Yes, I forgot to add th= ose newlines.=C2=A0 I'll fix it.

>> + (insert (propertize = "Mouse-down-action" 'face 'bold))
>> + (insert &= quot;\nThe mouse-down-action of this button is ")
>> + (if (s= ymbolp mouse-down-action)
>> + =C2=A0 =C2=A0(progn
>> + = =C2=A0 =C2=A0 =C2=A0(princ mouse-down-action)
>> + =C2=A0 =C2=A0= =C2=A0(insert ",\nwhich is ")
>> + =C2=A0 =C2=A0 =C2= =A0(describe-function-1 mouse-down-action))
>> + =C2=A0(insert &q= uot;\n")
>> + =C2=A0(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.=C2=A0 Why not ac= cept an
> alist of useful properties as a single argument instead?=C2= =A0 Then you could
> write something like the following:
>
&= gt; =C2=A0 (defun button--describe-actions (properties)
> =C2=A0 =C2= =A0 "Describe a button's PROPERTIES (an alist) in a *Help* buffer.=
> =C2=A0 This is a helper function for `button-describe-actions'= , intended
> =C2=A0 to be used with `help-setup-xref'."
&= gt; =C2=A0 =C2=A0 (with-help-window (help-buffer)
> =C2=A0 =C2=A0 =C2= =A0 (with-current-buffer (help-buffer)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 = (insert (format-message "This button's type is `%s'."
= > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (alist-get 'type properties))= )
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 (dolist (prop '(action mouse-down= -action))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (let ((name (symbol-na= me prop))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (= val =C2=A0(alist-get prop properties)))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 (when (functionp val)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 (insert "\n\n"
> =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (propertize (capita= lize name) 'face 'bold)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "\nThe " name " of= this button is")
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 (if (symbolp val)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 (progn
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (insert (format-message " `%s',\nw= hich is " val))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 (describe-function-1 val))
> =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (insert "\n")
> = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (princ val))))))))<= br>
Totally! I'll adapt it to something like that.

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

Thanks, I'll fix those.

&= gt;> +Displays a *Help* buffer with a description of the actions.
>= ;> +
>> +When called from Lisp, pass BUTTON-OR-POS as the butto= n to describe, or a
>> +buffer position where a button is present.= =C2=A0 If BUTTON-OR-POS is nil, the
>> +button at point is the but= ton to describe."
>> + =C2=A0(interactive "d")
&= gt;> + =C2=A0(let ((button (cond ((numberp button-or-pos)
>> + = =C2=A0 =C2=A0 =C2=A0 (button-at button-or-pos))
>> + =C2=A0 = =C2=A0 =C2=A0((markerp button-or-pos)
>> + =C2=A0 =C2=A0 =C2=A0 = (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* bu= ffer.=C2=A0 However, it
looks like I can trigger the problem (or a simil= ar one, I'm not sure)
even with the way I wrote it, so I'll have= to investigate it further.

>> + =C2=A0 =C2=A0 =C2=A0((null = button-or-pos)
>> + =C2=A0 =C2=A0 =C2=A0 (button-at (point)))>> + =C2=A0 =C2=A0 =C2=A0(t
>> + =C2=A0 =C2=A0 =C2=A0 b= utton-or-pos)))
>> + action mouse-down-action type)
>> + = =C2=A0 =C2=A0(when button
>> + =C2=A0 =C2=A0 =C2=A0(setq type (but= ton-type button)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0act= ion (button-get button 'action)
>> + =C2=A0 =C2=A0mouse-down-= action (button-get button 'mouse-down-action))
>> + =C2=A0 =C2= =A0 =C2=A0(help-setup-xref
>> + (list #'button--describe-actio= ns-internal type action
>> mouse-down-action)
>> + =C2=A0= =C2=A0 =C2=A0 (called-interactively-p 'interactive))
>> + =C2= =A0 =C2=A0 =C2=A0(button--describe-actions-internal type action mouse-down-= action)
>> + =C2=A0 =C2=A0 =C2=A0t)))
>
> As suggested= previously, why not pass all interesting properties to the
> helper = function as a single argument?=C2=A0 For example:
>
> =C2=A0 (d= efun button-describe-actions (&optional button-or-pos)
> =C2=A0 = =C2=A0 "Describe the actions associated with the button at point.
&= gt; =C2=A0 Displays a *Help* buffer with a description of the actions.
&= gt;
> =C2=A0 When called from Lisp, BUTTON-OR-POS can be a buffer pos= ition
> =C2=A0 where a button (including overlay buttons) is present,= a button
> =C2=A0 object (e.g., an overlay), or nil, meaning the but= ton at point."
> =C2=A0 =C2=A0 (interactive "d")
&g= t; =C2=A0 =C2=A0 (let* ((button (cond ((integer-or-marker-p button-or-pos)<= br>> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 (button-at button-or-pos))
> =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0((null button-or-pos)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (button-at (point)))
&g= t; =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0(t
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 button-or-pos)))
> = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(props (and button
> =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0(mapcar (lambda (prop)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0(cons prop (button-get button prop)))
> =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0'(type action mouse-down-action)))))
> =C2=A0 =C2=A0= =C2=A0 (when props
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 (help-setup-xref (l= ist #'button--describe-actions props)
> =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(called-i= nteractively-p 'interactive))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 (butt= on--describe-actions props)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 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
>> =C2=A0 (while (setq= table (char-table-parent table))
>> =C2=A0 =C2=A0(insert "\= nThe parent category table is:")
>> =C2=A0 =C2=A0(describe-v= ector table 'help-describe-category-set))))))
>> +=0C
>&= gt; +;; Actions.
>> +
>> +(defvar describe-actions-functi= ons '(button-describe-actions
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0= =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 widget-describe-actions)
>> + =C2=A0"= ;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 positio= n.
>> +The argument passed might be nil, which indicates to descri= be the actions of
>> +the element at point.")
>
>= Why should these hook functions accept a nil argument?
> Why not alw= ays call them with a buffer position as argument?

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

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

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

Typo.=C2=A0 I'll fix it.

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

That'= ;s better, yes.=C2=A0 I'll change it.

>> +returns non-nil.= "
>> + =C2=A0(interactive
>> + =C2=A0 (list
>&= gt; + =C2=A0 =C2=A0(let ((key
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 (read-key
>> + =C2=A0 =C2=A0"Click an element, or hit RE= T to describe the element at point")))
>> + =C2=A0 =C2=A0 =C2= =A0(cond ((eq key ?\C-m) nil)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0((and (mouse-event-p key)
>> + =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(eq (event-basic-type key) 'mouse= -1)
>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0(equal (event-modifiers key) '(click)))
>> + =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (posn-point (event-end key)))
>
&g= t; What if the click is in a different window?
>

I missed that= , silly me.=C2=A0 I'm not sure what would be better, but I would
lik= e to describe the clicked element (if any), even if it's not in the
= selected window.

>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0((eq key ?\C-g) (signal 'quit nil))
>> + =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0(t (user-error "You didn't specify an elem= ent"))))))
>
> I wonder if we can reuse help--read-key-seq= uence or similar here?

I don't think so: =C2=A0It has a prompt t= hat would not be useful here, and
for the expected input, it seems a lit= tle overkill.=C2=A0 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.=C2=A0 If WIDG= ET-OR-POS is nil,
>> +the widget at point is the widget to describ= e."
>> + =C2=A0(interactive "d")
>> + =C2= =A0(require 'wid-browse)
>> + =C2=A0(let ((widget (if (widgetp= widget-or-pos)
>> + =C2=A0 =C2=A0widget-or-pos
>> + = =C2=A0(widget-at (or widget-or-pos (point)))))
>
> widget-at ac= cepts nil, so you can just write (widget-at widget-or-pos).

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

> [...]
= >
>> +(defun widget-resolve-parent-action (widget)
>
&= gt; Should this function have a name with a double hyphen '--'?
=
Yes, I'll do it.

>> + "If action of WIDGET is `wi= dget-parent-action', find out what
>> would that be."
= >> + =C2=A0(let ((action (widget-get widget :action))
>> + (= parent (widget-get widget :parent)))
>> + =C2=A0 =C2=A0(while (and= (eq action 'widget-parent-action)
>> + (setq parent (widget-= get parent :parent)))
>> + =C2=A0 =C2=A0 =C2=A0(setq action (widge= t-get parent :action)))
>> + =C2=A0 =C2=A0action))
>
>= I think there's a bug here.=C2=A0 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 pa= rent 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.=C2=A0 Shouldn't we do the = following instead?
>
> =C2=A0 (defun widget--resolve-parent-act= ion (widget)
> =C2=A0 =C2=A0 "Resolve the real action of WIDGET = up its inheritance chain.
> =C2=A0 Follow the :parents of WIDGET unti= l its :action is no longer
> =C2=A0 `widget-parent-action', and r= eturn its value."
> =C2=A0 =C2=A0 (let ((action (widget-get widg= et :action)))
> =C2=A0 =C2=A0 =C2=A0 (while (eq action #'widget-p= arent-action)
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 (setq widget (widget-get = widget :parent))
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 (setq action (widget-g= et widget :action)))
> =C2=A0 =C2=A0 =C2=A0 action))

Yes, I ha= d something like that in my draft, and for some reason I
changed it and = ended up with a buggy version.=C2=A0 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 v= ery helpful.=C2=A0 I hope I'll find
time in the next days to keep wo= rking on it.
--000000000000dc928a059465ce0b--