From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED.blaine.gmane.org!not-for-mail From: "Basil L. Contovounesios" Newsgroups: gmane.emacs.bugs Subject: bug#139: describe-key vs. widget red tape Date: Sun, 06 Oct 2019 01:39:35 +0100 Message-ID: <878spyemgo.fsf@tcd.ie> 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> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: blaine.gmane.org; posting-host="blaine.gmane.org:195.159.176.226"; logging-data="99362"; mail-complaints-to="usenet@blaine.gmane.org" User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (gnu/linux) Cc: 139@debbugs.gnu.org, Lars Ingebrigtsen , jidanni@jidanni.org To: Mauro Aranda Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sun Oct 06 02:40:16 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 1iGuat-000Pj4-L7 for geb-bug-gnu-emacs@m.gmane.org; Sun, 06 Oct 2019 02:40:15 +0200 Original-Received: from localhost ([::1]:59570 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iGuar-0000kb-ST for geb-bug-gnu-emacs@m.gmane.org; Sat, 05 Oct 2019 20:40:13 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:60948) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iGuai-0000kC-Nw for bug-gnu-emacs@gnu.org; Sat, 05 Oct 2019 20:40:06 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iGuag-0005Ty-RW for bug-gnu-emacs@gnu.org; Sat, 05 Oct 2019 20:40:04 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:36534) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iGuag-0005TU-DX for bug-gnu-emacs@gnu.org; Sat, 05 Oct 2019 20:40:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1iGuag-000839-4F for bug-gnu-emacs@gnu.org; Sat, 05 Oct 2019 20:40:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: "Basil L. Contovounesios" Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sun, 06 Oct 2019 00:40:02 +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.157032238630892 (code B ref 139); Sun, 06 Oct 2019 00:40:02 +0000 Original-Received: (at 139) by debbugs.gnu.org; 6 Oct 2019 00:39:46 +0000 Original-Received: from localhost ([127.0.0.1]:45353 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iGuaQ-00082B-40 for submit@debbugs.gnu.org; Sat, 05 Oct 2019 20:39:46 -0400 Original-Received: from mail-wr1-f41.google.com ([209.85.221.41]:44654) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1iGuaO-00081v-11 for 139@debbugs.gnu.org; Sat, 05 Oct 2019 20:39:45 -0400 Original-Received: by mail-wr1-f41.google.com with SMTP id z9so11110345wrl.11 for <139@debbugs.gnu.org>; Sat, 05 Oct 2019 17:39:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=tcd-ie.20150623.gappssmtp.com; s=20150623; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=DxK7qLNh+sZT/odDRBXq+37TPSaZrF4zHaEKuyojYWk=; b=0vVUsQJ1WCia0vpivvzyIQ7MkX0dHBOisaWhydAnpEYwJD1idQFt7r+/UGnv5n/FgG Zk3YKcmg5fpRXhu3Ooa3qGM3rBBSQNMRW4aR6Zmn4ahS/VX6Qk9Wguiqlvifs/q2veID ieJDEH+ADoN95EupZxwYZEiiKxvoiGBIFs5PnnpnAm1FyfLCEW2oZFpXYy8xAJazFjxi KgJCua8N8gPZx2nnH/hBg9IOwmS6Uvy7oYr8BdocdUCj5d4gr7ro8wf0nNPxsvmoJykH ebM1BEB9IEWL+iEl5f/LaQGTKxm4KFzvpGft2o3fQXakeJPw5All9EOtyox3WQ6gPbqW ss3Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=DxK7qLNh+sZT/odDRBXq+37TPSaZrF4zHaEKuyojYWk=; b=Xlzn8RPmSO1cFRUHO4P1DI854gi+tB9zALrzCVLptY+FlM5VOEj22kWPqTFi02rQI+ +Kecu2E4MA+uIRv5Gk1TyooZyS+SUfohrMJOMn7RTMytEDycwJXpCqu41c+LGn8moPBC GOQSf6oYR4Lutk2MgWxn1gS96QpaumpIeK0RJ2dcDHSvKt2Yp50OjwuH+CMWaEdK1QWy Z934r36PBmtrub6zcTnnF96q8XbP6WJPFFCE0fSQ4XxztTUh6aklRW2UL9LAsrMwj+Te UcJ5fkQjupt49zd40ktXN2vghcQGyAL9/k1c7fI+zN1ZiKX8EzpM33n7lurE5s60qq8E TYXg== X-Gm-Message-State: APjAAAV7gc7nTB8Z7wI2iPiKzSfbrLKfVsjWmIi+Sn6LnDhjMc7niRsB lS/uHN1ZTVrlPPhryE/97bAHPw== X-Google-Smtp-Source: APXvYqz0lieDDuE8W0U3/Z5sjBMZMgy6DPHwXGL/NmtQSQcNhAniKGghlU/HQRSIZqBEvXLufupyrQ== X-Received: by 2002:adf:fd4d:: with SMTP id h13mr16846763wrs.66.1570322377826; Sat, 05 Oct 2019 17:39:37 -0700 (PDT) Original-Received: from localhost ([2a02:8084:20e2:c380:92bd:1bfd:38fc:fae2]) by smtp.gmail.com with ESMTPSA id g17sm3414461wrq.58.2019.10.05.17.39.36 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 05 Oct 2019 17:39:37 -0700 (PDT) In-Reply-To: (Mauro Aranda's message of "Sat, 5 Oct 2019 11:06:24 -0300") 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:168404 Archived-At: Mauro Aranda 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)))))) > + > +;; 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"))) > > > ;;; 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