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

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

[Mauro Aranda]

emacs -Q
M-x customize-icon RET button
Edit something.  For example, under the Text Only choice, make the
string look like: "-button-" instead of "button".
Observe that Custom says: "EDITED, ..."
Go to the State button and hit RET.  Nothing happens.
Click it.  Nothing happens.

Expectation is that a menu pops up to set the icon settings. And
without -Q, to save the settings.


In GNU Emacs 30.0.50 (build 7, x86_64-pc-linux-gnu, GTK+ Version
  3.24.33, cairo version 1.16.0) of 2023-10-19 built on tbb-desktop
Repository revision: 0fd7f785e76c9f2eea1baa40aed6ee327f68a993
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12201001
System Description: Ubuntu 22.04.3 LTS

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
   value of $LC_MONETARY: es_AR.UTF-8
   value of $LC_NUMERIC: es_AR.UTF-8
   value of $LC_TIME: es_AR.UTF-8
   value of $LANG: en_US.UTF-8
   value of $XMODIFIERS: @im=ibus
   locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
   tooltip-mode: t
   global-eldoc-mode: t
   eldoc-mode: t
   show-paren-mode: t
   electric-indent-mode: t
   mouse-wheel-mode: t
   tool-bar-mode: t
   menu-bar-mode: t
   file-name-shadow-mode: t
   global-font-lock-mode: t
   font-lock-mode: t
   blink-cursor-mode: t
   minibuffer-regexp-mode: t
   line-number-mode: t
   indent-tabs-mode: t
   transient-mark-mode: t
   auto-composition-mode: t
   auto-encryption-mode: t
   auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel
term/x-win x-win term/common-win x-dnd touch-screen tool-bar dnd fontset
image regexp-opt fringe tabulated-list replace newcomment text-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo gtk
x-toolkit xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 37643 10434) (symbols 48 5213 0) (strings 32 13209 1857)
  (string-bytes 1 374142) (vectors 16 10299)
  (vector-slots 8 156471 14182) (floats 8 21 23) (intervals 56 223 0)
  (buffers 992 10))

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

[Mauro Aranda]

[-- Attachment #1: Type: text/plain, Size: 628 bytes --]

tags 66635 patch
quit


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.

And while trying to fix the state messages in Custom, I found that the
arguments for plist-get were transposed in icons--merge-spec, so I
suspect that function never worked either.

There are a lot of things to improve here (I discovered the issues while
looking at Bug#57113), but I hope this patch is good enough for this bug
report.

[-- Attachment #2: 0001-Fix-State-button-for-customize-icon-Bug-66635.patch --]
[-- Type: text/x-patch, Size: 6120 bytes --]

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))
     (custom-push-theme 'theme-icon symbol 'user 'set val)
-    (custom-redraw-magic widget)))
+    (custom-redraw widget)))
+
+(defun custom-icon-save (widget)
+  "Save value of icon edited by widget WIDGET."
+  (custom-set-icons (cons (widget-value widget)
+                          (list
+                           (custom--icons-widget-value
+                            (car (widget-get widget :children))))))
+  (custom-save-all)
+  (custom-icon-state-set widget)
+  (custom-redraw-magic widget))
 
 ;;;###autoload
 (defun customize-icon (icon)
diff --git a/lisp/emacs-lisp/icons.el b/lisp/emacs-lisp/icons.el
index cb08c1a6b81..9a6d26243c7 100644
--- a/lisp/emacs-lisp/icons.el
+++ b/lisp/emacs-lisp/icons.el
@@ -181,9 +181,9 @@ icons--merge-spec
         (let ((parent-keywords (icon-spec-keywords elem))
               (current-keywords (icon-spec-keywords current)))
           (while parent-keywords
-            (unless (plist-get (car parent-keywords) current-keywords)
-              (nconc current (take 2 parent-keywords))
-              (setq parent-keywords (cddr parent-keywords))))))))
+            (unless (plist-get current-keywords (car parent-keywords))
+              (nconc current (take 2 parent-keywords)))
+            (setq parent-keywords (cddr parent-keywords)))))))
   merged)
 
 (cl-defmethod icons--create ((_type (eql 'image)) icon keywords)
-- 
2.34.1

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

[Stefan Kangas]

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?

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

>      (custom-push-theme 'theme-icon symbol 'user 'set val)
> -    (custom-redraw-magic widget)))
> +    (custom-redraw widget)))
> +
> +(defun custom-icon-save (widget)
> +  "Save value of icon edited by widget WIDGET."
> +  (custom-set-icons (cons (widget-value widget)
> +                          (list
> +                           (custom--icons-widget-value
> +                            (car (widget-get widget :children))))))
> +  (custom-save-all)
> +  (custom-icon-state-set widget)
> +  (custom-redraw-magic widget))
>
>  ;;;###autoload
>  (defun customize-icon (icon)
> diff --git a/lisp/emacs-lisp/icons.el b/lisp/emacs-lisp/icons.el
> index cb08c1a6b81..9a6d26243c7 100644
> --- a/lisp/emacs-lisp/icons.el
> +++ b/lisp/emacs-lisp/icons.el
> @@ -181,9 +181,9 @@ icons--merge-spec
>          (let ((parent-keywords (icon-spec-keywords elem))
>                (current-keywords (icon-spec-keywords current)))
>            (while parent-keywords
> -            (unless (plist-get (car parent-keywords) current-keywords)
> -              (nconc current (take 2 parent-keywords))
> -              (setq parent-keywords (cddr parent-keywords))))))))
> +            (unless (plist-get current-keywords (car parent-keywords))
> +              (nconc current (take 2 parent-keywords)))
> +            (setq parent-keywords (cddr parent-keywords)))))))
>    merged)
>
>  (cl-defmethod icons--create ((_type (eql 'image)) icon keywords)
> --
> 2.34.1

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.

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

[Eli Zaretskii]

> Date: Fri, 20 Oct 2023 21:21:34 -0300
> From: 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.

icons.el is a new package in Emacs 29, so maybe such changes in it to
fix a serious problem are okay even at this stage.

How well is the fix tested?  Did you test it with several icons
introduced in Emacs 29 in various modes?  If not, would you please
test those and see the changes work well enough there to be
appropriate for a bug-fix release?

Thanks.

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

[Mauro Aranda]

On 21/10/23 04:35, Eli Zaretskii wrote:
 >> Date: Fri, 20 Oct 2023 21:21:34 -0300
 >> From: 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.
 >
 > icons.el is a new package in Emacs 29, so maybe such changes in it to
 > fix a serious problem are okay even at this stage.

Great, thanks.

 > How well is the fix tested?  Did you test it with several icons
 > introduced in Emacs 29 in various modes?  If not, would you please
 > test those and see the changes work well enough there to be
 > appropriate for a bug-fix release?

I just tested with the button icon, the only one suggested after
running emacs -Q.  I tested making modifications in the buffer, and that
the two options (three if not started with -Q) work and do what's
intended.  Namely:
- Undo Editions took me back to the STANDARD state.
- Setting for the session modified the specs for the session and
correctly displayed the SET state.
- Saving the setting saved a spec that in a new session (without emacs
-Q) causes:
M-x describe-icon RET button
to show the button customized.  The value I changed for that was from
LARGE BLUE CIRCLE to LARGE RED SQUARE

It's important to note that saving the setting shows the SET state
(rather than the SAVED state), but that's in a FIXME right before
custom-icon-state.

I'll do some more testing with the icons defined in outline.el,
tab-bar.el and warnings.el.  Additionally I'll try to test if
Customizing an icon interacts good enough with a Theme customizations.
I'll do that and report back.

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

[Mauro Aranda]

On 21/10/23 07:18, Mauro Aranda wrote:
 > On 21/10/23 04:35, Eli Zaretskii wrote:

 >  > How well is the fix tested?  Did you test it with several icons
 >  > introduced in Emacs 29 in various modes?  If not, would you please
 >  > test those and see the changes work well enough there to be
 >  > appropriate for a bug-fix release?
 >
 > I just tested with the button icon, the only one suggested after
 > running emacs -Q.  I tested making modifications in the buffer, and that
 > the two options (three if not started with -Q) work and do what's
 > intended.  Namely:
 > - Undo Editions took me back to the STANDARD state.
 > - Setting for the session modified the specs for the session and
 > correctly displayed the SET state.
 > - Saving the setting saved a spec that in a new session (without emacs
 > -Q) causes:
 > M-x describe-icon RET button
 > to show the button customized.  The value I changed for that was from
 > LARGE BLUE CIRCLE to LARGE RED SQUARE
 >
 > It's important to note that saving the setting shows the SET state
 > (rather than the SAVED state), but that's in a FIXME right before
 > custom-icon-state.
 >
 > I'll do some more testing with the icons defined in outline.el,
 > tab-bar.el and warnings.el.  Additionally I'll try to test if
 > Customizing an icon interacts good enough with a Theme customizations.
 > I'll do that and report back.

I checked outline.el icons and everything seems to work OK.  Tested in
some buffer with outline-mode, including the NEWS file.  Savings get
loaded correctly.  Similarly with warning-suppress icon.

With tab-bar.el I tried other images, and works fine too.  It didn't get
updated immediately, but after some time and user interaction, it showed
the new image.  I don't use tab-bar-mode so maybe there's something to
do to make it update.

Then I created a foo theme to customize icons and pon starting emacs and
customizing the icons the state was THEMED, and after customizing them
my customizations were respected.  So that works fine too.

One thing I found, but not related to my patch (and see below for
others) is that I had to require 'cus-edit to make
custom-theme-set-icons available.  That might not be optimal.

To sum it up, after testing more thoroughly, I haven't yet found any
trouble with my patch, which improves considerably icons customization.
More work needs to be done (expanding state checking, reverting
customizations, improving help in the custom buffer, adding support in
customize-create-theme, showing the image that would get used, etc), but
those should be separate bug reports, I think.

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

[Stefan Kangas]

Mauro Aranda <maurooaranda@gmail.com> writes:

> To sum it up, after testing more thoroughly, I haven't yet found any
> trouble with my patch, which improves considerably icons customization.
> More work needs to be done (expanding state checking, reverting
> customizations, improving help in the custom buffer, adding support in
> customize-create-theme, showing the image that would get used, etc), but
> those should be separate bug reports, I think.

Thanks for working on this.

Eli, do you see any reason for not installing this on emacs-29?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Sat, 21 Oct 2023 04:17:51 -0700
> Cc: 66635@debbugs.gnu.org
> 
> Mauro Aranda <maurooaranda@gmail.com> writes:
> 
> > To sum it up, after testing more thoroughly, I haven't yet found any
> > trouble with my patch, which improves considerably icons customization.
> > More work needs to be done (expanding state checking, reverting
> > customizations, improving help in the custom buffer, adding support in
> > customize-create-theme, showing the image that would get used, etc), but
> > those should be separate bug reports, I think.
> 
> Thanks for working on this.
> 
> Eli, do you see any reason for not installing this on emacs-29?

I think this is OK for emacs-29, thanks.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> I think this is OK for emacs-29, thanks.

Thanks.

Mauro, could you send the latest version of the patch?  I believe the
one you sent before has a typo.

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

[Mauro Aranda]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

On 21/10/23 08:51, Stefan Kangas wrote:
 > Eli Zaretskii <eliz@gnu.org> writes:
 >
 >> I think this is OK for emacs-29, thanks.
 >
 > Thanks.
 >
 > Mauro, could you send the latest version of the patch?  I believe the
 > one you sent before has a typo.

Hmm, I didn't find the typo.  But it's hard for me to catch typos of my
own.  Sending the patch again anyway, but let me know if you spot it.

[-- Attachment #2: 0001-Fix-State-button-for-customize-icon-Bug-66635.patch --]
[-- Type: text/x-patch, Size: 6120 bytes --]

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))
     (custom-push-theme 'theme-icon symbol 'user 'set val)
-    (custom-redraw-magic widget)))
+    (custom-redraw widget)))
+
+(defun custom-icon-save (widget)
+  "Save value of icon edited by widget WIDGET."
+  (custom-set-icons (cons (widget-value widget)
+                          (list
+                           (custom--icons-widget-value
+                            (car (widget-get widget :children))))))
+  (custom-save-all)
+  (custom-icon-state-set widget)
+  (custom-redraw-magic widget))
 
 ;;;###autoload
 (defun customize-icon (icon)
diff --git a/lisp/emacs-lisp/icons.el b/lisp/emacs-lisp/icons.el
index cb08c1a6b81..9a6d26243c7 100644
--- a/lisp/emacs-lisp/icons.el
+++ b/lisp/emacs-lisp/icons.el
@@ -181,9 +181,9 @@ icons--merge-spec
         (let ((parent-keywords (icon-spec-keywords elem))
               (current-keywords (icon-spec-keywords current)))
           (while parent-keywords
-            (unless (plist-get (car parent-keywords) current-keywords)
-              (nconc current (take 2 parent-keywords))
-              (setq parent-keywords (cddr parent-keywords))))))))
+            (unless (plist-get current-keywords (car parent-keywords))
+              (nconc current (take 2 parent-keywords)))
+            (setq parent-keywords (cddr parent-keywords)))))))
   merged)
 
 (cl-defmethod icons--create ((_type (eql 'image)) icon keywords)
-- 
2.34.1

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

[Stefan Kangas]

Mauro Aranda <maurooaranda@gmail.com> writes:

> +  ;; Not implemented yet.
> +  ;; :custom-reset-saved 'custom-icon-reset-saved)
     ^^                                             ^

The closing paren shouldn't be commented out.

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

[Mauro Aranda]

On 22/10/23 09:09, Stefan Kangas wrote:
> Mauro Aranda <maurooaranda@gmail.com> writes:
> 
>> +  ;; Not implemented yet.
>> +  ;; :custom-reset-saved 'custom-icon-reset-saved)
>       ^^                                             ^
> 
> The closing paren shouldn't be commented out.
> 

It's on the previous line.

+  :custom-reset-current #'custom-redraw)
^^                                      ^
+  ;; Not implemented yet.
+  ;; :custom-reset-saved 'custom-icon-reset-saved)
+

BTW, I'm working on implementing the missing functions
(there are others, apart from :custom-reset-saved).

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

[Stefan Kangas]

Version: 29.2

Mauro Aranda <maurooaranda@gmail.com> writes:

> It's on the previous line.

Oops, I missed that.

Pushed to emacs-29, and I'm consequently closing this bug.

[1: 5f60913208f]: 2023-10-22 18:39:52 +0200
  Fix State button for customize-icon (Bug#66635)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=5f60913208f3fb2df9a2d3bda1655e01075bf446

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

[Eli Zaretskii]

> Date: Sat, 21 Oct 2023 07:18:31 -0300
> Cc: stefankangas@gmail.com, 66635@debbugs.gnu.org
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> On 21/10/23 04:35, Eli Zaretskii wrote:
>  > How well is the fix tested?  Did you test it with several icons
>  > introduced in Emacs 29 in various modes?  If not, would you please
>  > test those and see the changes work well enough there to be
>  > appropriate for a bug-fix release?
> 
> I just tested with the button icon, the only one suggested after
> running emacs -Q.  I tested making modifications in the buffer, and that
> the two options (three if not started with -Q) work and do what's
> intended.  Namely:
> - Undo Editions took me back to the STANDARD state.
> - Setting for the session modified the specs for the session and
> correctly displayed the SET state.
> - Saving the setting saved a spec that in a new session (without emacs
> -Q) causes:
> M-x describe-icon RET button
> to show the button customized.  The value I changed for that was from
> LARGE BLUE CIRCLE to LARGE RED SQUARE
> 
> It's important to note that saving the setting shows the SET state
> (rather than the SAVED state), but that's in a FIXME right before
> custom-icon-state.
> 
> I'll do some more testing with the icons defined in outline.el,
> tab-bar.el and warnings.el.  Additionally I'll try to test if
> Customizing an icon interacts good enough with a Theme customizations.
> I'll do that and report back.

Thanks.