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

[Mauro Aranda]

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.