bug#66635: 30.0.50; customize-icon State button doesn't work (never did)

[Mauro Aranda <maurooaranda@gmail.com>]

On 20/10/23 18:08, Stefan Kangas wrote:
 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >
 >> I attach a patch to address the more important issues for now.  That is,
 >> at least have a working State button and rudimentary state checking.
 >>
 >> Setting and Saving icon specifications through the State button should
 >> work now, by adding the custom-icon-action function.
 >
 >
 > Thanks for working on this.
 >
 > Do you propose this patch for emacs-29?  It seems quite intrusive on the
 > face of it, but OTOH `customize-icon' is new in Emacs 29, so there is no
 > risk of regressions if this stuff never worked in the first place.  Or
 > is that wrong?

I'm not aware of all things you and Eli have to take into account when
deciding whether a patch is good for emacs-29.  I know the non-intrusive
or localized, and the safe part.  This patch is certainly neither, but
it's the minimum (OK, maybe not the bare minimum) to make customizing
icons work for an user.  I think that's a good reason for the patch to
go into emacs-29, but I won't be insisting on it, specially if it is a
burden, considering 29.1.90 pretest is out.

It could be made less intrusive without the change to icons--merge-spec,
but I don't think it'd be wise to do that.

 >> From ab4fabf48665ddc142ad95a26898eb6207cd2bdc Mon Sep 17 00:00:00 2001
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >> Date: Thu, 19 Oct 2023 08:46:35 -0300
 >> Subject: [PATCH] Fix State button for customize-icon (Bug#66635)
 >>
 >> * lisp/cus-edit.el (custom-icon-action): New function.
 >> (custom-icon): Use it as the :action.  Otherwise, clicking the State
 >> button is a noop.  Remove irrelevant stuff from the docstring and
 >> comment out some copy-pasta.
 >> (custom-icon-extended-menu): New variable, the menu to show upon
 >> :action.
 >> (custom-icon-set): Really redraw the widget with the new settings.
 >> Comment out strange call to custom-variable-backup-value.
 >> (custom-icon-save): New function.
 >>
 >> * lisp/emacs-lisp/icons.el (icons--merge-spec): Fix call to plist-get
 >> and avoid infloop.
 >> ---
 >>  lisp/cus-edit.el         | 71 +++++++++++++++++++++++++++++++++-------
 >>  lisp/emacs-lisp/icons.el |  6 ++--
 >>  2 files changed, 62 insertions(+), 15 deletions(-)
 >>
 >> diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
 >> index 706e08d5657..953b8b8b80f 100644
 >> --- a/lisp/cus-edit.el
 >> +++ b/lisp/cus-edit.el
 >> @@ -5366,11 +5366,6 @@ 'custom-icon
 >>  :hidden-states should be a list of widget states for which the
 >>    widget's initial contents are to be hidden.
 >>
 >> -:custom-form should be a symbol describing how to display and
 >> -  edit the variable---either `edit' (using edit widgets),
 >> -  `lisp' (as a Lisp sexp), or `mismatch' (should not happen);
 >> -  if nil, use the return value of `custom-variable-default-form'.
 >> -
 >>  :shown-value, if non-nil, should be a list whose `car' is the
 >>    variable value to display in place of the current value.
 >>
 >> @@ -5383,11 +5378,34 @@ 'custom-icon
 >>    :custom-category 'option
 >>    :custom-state nil
 >>    :custom-form nil
 >> -  :value-create 'custom-icon-value-create
 >> +  :value-create #'custom-icon-value-create
 >>    :hidden-states '(standard)
 >> -  :custom-set 'custom-icon-set
 >> -  :custom-reset-current 'custom-redraw
 >> -  :custom-reset-saved 'custom-variable-reset-saved)
 >> +  :action #'custom-icon-action
 >> +  :custom-set #'custom-icon-set
 >> +  :custom-reset-current #'custom-redraw)
 >> +  ;; Not implemented yet.
 >> +  ;; :custom-reset-saved 'custom-icon-reset-saved)
 >> +
 >> +(defvar custom-icon-extended-menu
 >> +  (let ((map (make-sparse-keymap)))
 >> +    (define-key-after map [custom-icon-set]
 >> +      '(menu-item "Set for Current Session" custom-icon-set
 >> +                  :enable (eq (widget-get custom-actioned-widget 
:custom-state)
 >> +                              'modified)))
 >> +    (when (or custom-file init-file-user)
 >> +      (define-key-after map [custom-icon-save]
 >> +        '(menu-item "Save for Future Sessions" custom-icon-save
 >> +                    :enable (memq
 >> +                             (widget-get custom-actioned-widget 
:custom-state)
 >> +                             '(modified set changed)))))
 >> +    (define-key-after map [custom-redraw]
 >> +      '(menu-item "Undo Edits" custom-redraw
 >> +                  :enable (memq
 >> +                           (widget-get custom-actioned-widget 
:custom-state)
 >> +                           '(modified changed))))
 >> +    map)
 >> +  "A menu for `custom-icon' widgets.
 >> +Used in `custom-icon-action' to show a menu to the user.")
 >>
 >>  (defun custom-icon-value-create (widget)
 >>    "Here is where you edit the icon's specification."
 >> @@ -5517,6 +5535,24 @@ custom-icon-value-create
 >>        (custom-add-parent-links widget))
 >>      (custom-add-see-also widget)))))
 >>
 >> +(defun custom-icon-action (widget &optional event)
 >> +  "Show the menu for `custom-icon' WIDGET.
 >> +Optional EVENT is the location for the menu."
 >> +  (if (eq (widget-get widget :custom-state) 'hidden)
 >> +      (custom-toggle-hide widget)
 >> +    (unless (eq (widget-get widget :custom-state) 'modified)
 >> +      (custom-icon-state-set widget))
 >> +    (custom-redraw-magic widget)
 >> +    (let* ((completion-ignore-case t)
 >> +           (custom-actioned-widget widget)
 >> +           (answer (widget-choose (concat "Operation on "
 >> + (custom-unlispify-tag-name
 >> +                                           (widget-get widget :value)))
 >> + custom-icon-extended-menu
 >> +                                  event)))
 >> +      (when answer
 >> +        (funcall answer widget)))))
 >> +
 >>  (defun custom-toggle-hide-icon (visibility-widget &rest _ignore)
 >>    "Toggle the visibility of a `custom-icon' parent widget.
 >>  By default, this signals an error if the parent has unsaved
 >> @@ -5553,10 +5589,21 @@ custom-icon-set
 >>        (user-error "Cannot update hidden icon"))
 >>
 >>      (setq val (custom--icons-widget-value child))
 >> -    (unless (equal val (icon-complete-spec symbol))
 >> -      (custom-variable-backup-value widget))
 >> +    ;; FIXME: What was the intention here?
 >> +    ;; (unless (equal val (icon-complete-spec symbol))
 >> +    ;;   (custom-variable-backup-value widget))
 >
 > Is there any reason to not just remove this outright?
 >
 > It'd still be there in git history, in the unlikely event that we should
 > need it again.

I just couldn't tell what was the intention.  Maybe the intention was to
make a backup just like we do for variables, for later getting the
backed up value again, like custom-variable-reset-value does.  In that
case, the comment may help to remind someone (and certainly me if I ever
get the time) to implement something like that.  Since I'm not certain
what's the intention, I had no way of justifying the removal, so I opted
to comment it out.  I won't object if you prefer otherwise.