Package: emacs;
Reported by: Mauro Aranda <maurooaranda <at> gmail.com>
Date: Thu, 19 Oct 2023 11:20:01 UTC
Severity: normal
Tags: patch
Found in version 30.0.50
Fixed in version 29.2
Done: Stefan Kangas <stefankangas <at> gmail.com>
Bug is archived. No further changes may be made.
Message #16 received at 66635 <at> debbugs.gnu.org (full text, mbox):
From: Mauro Aranda <maurooaranda <at> gmail.com> To: Stefan Kangas <stefankangas <at> gmail.com>, 66635 <at> debbugs.gnu.org Subject: Re: bug#66635: 30.0.50; customize-icon State button doesn't work (never did) Date: Fri, 20 Oct 2023 21:21:34 -0300
On 20/10/23 18:08, Stefan Kangas wrote: > Mauro Aranda <maurooaranda <at> 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 <at> 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.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.