* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior @ 2012-11-11 20:25 Drew Adams 2020-09-01 16:02 ` Mauro Aranda 0 siblings, 1 reply; 9+ messages in thread From: Drew Adams @ 2012-11-11 20:25 UTC (permalink / raw) To: 12864 emacs -Q M-x customize-option delete-old-versions Value Menu: Leave State: Revert This Session's Customization The State is now back to STANDARD, which is correct. In the State menu, this item is not dimmed, but should be, since there should be no backup value different from the original value (Ask): Set to Backup Value. Choose Set to Backup Value anyway. The State now shows "SET for current session only, which is incorrect (at best misleading). The State menu now shows items Set to Backup Value and Revert This Session's Customization, both of which are incorrect and misleading. (The current value is the standard value, and we reverted to it. Choose Revert This Session's Customization anyway. State now says CHANGED outside Customize, which is 100% wrong. And the State menu shows Undo Edits, Revert This Session's Customization, and Set to Backup Value, all of which are wrong (and confusing). Choose Undo Edits anyway. It has no visible effect - State and its menu stay the same. Again, confusing. Choose Set to Backup Value anyway. State now says SET for current session only, which is (still) wrong. And the same menu items are available, except Undo Edits. Choosing Set to Backup Value again has no visible effect. Choosing Revert This Session's Customization has the same incorrect effect as before (adds Undo Edits to the menu and changes State to CHANGED outside Customize. This is a confusing mess. Without emacs -Q it is even more confusing, with Reset to Saved added to the mix. One thing that is not clear in the behavior is that Set to Backup Value seems sometimes to be available without Reset to Saved (even without emacs -Q). If you have made changes to the value that have not been saved, then I would think that Reset to Saved would always be available (until you choose it or you save). In GNU Emacs 24.3.50.1 (i386-mingw-nt5.1.2600) of 2012-11-05 on MS-W7-DANI Bzr revision: 110809 lekktu@gmail.com-20121105172930-a5gn0bwi4lndchhw Windowing system distributor `Microsoft Corp.', version 5.1.2600 Configured using: `configure --with-gcc (4.7) --no-opt --enable-checking --cflags -I../../libs/libXpm-3.5.10/include -I../../libs/libXpm-3.5.10/src -I../../libs/libpng-1.2.37-lib/include -I../../libs/zlib-1.2.5 -I../../libs/giflib-4.1.4-1-lib/include -I../../libs/jpeg-6b-4-lib/include -I../../libs/tiff-3.8.2-1-lib/include -I../../libs/libxml2-2.7.8-w32-bin/include/libxml2 -I../../libs/gnutls-3.0.9-w32-bin/include -I../../libs/libiconv-1.9.2-1-lib/include' ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2012-11-11 20:25 bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior Drew Adams @ 2020-09-01 16:02 ` Mauro Aranda 2020-09-01 16:20 ` Stefan Kangas 0 siblings, 1 reply; 9+ messages in thread From: Mauro Aranda @ 2020-09-01 16:02 UTC (permalink / raw) To: 12864 [-- Attachment #1: Type: text/plain, Size: 2602 bytes --] Drew Adams <drew.adams@oracle.com> writes: > emacs -Q > > M-x customize-option delete-old-versions > > Value Menu: Leave > > State: Revert This Session's Customization > > The State is now back to STANDARD, which is correct. > > In the State menu, this item is not dimmed, but should be, since there > should be no backup value different from the original value (Ask): Set > to Backup Value. Would it make sense to guard against backing up a value, if it's the same as the new one? That way, Set to Backup Value would be disabled in this situation. > Choose Set to Backup Value anyway. The State now shows "SET for current > session only, which is incorrect (at best misleading). I think this is a bug in `custom-variable-state': the first cond clause doesn't check if value is equal to the standard-value, so it just returns 'set. It should check for it and return the correct state. > The State menu now shows items Set to Backup Value and Revert This > Session's Customization, both of which are incorrect and misleading. > (The current value is the standard value, and we reverted to it. Doing the two fixes above will fix this... > Choose Revert This Session's Customization anyway. > > State now says CHANGED outside Customize, which is 100% wrong. And the > State menu shows Undo Edits, Revert This Session's Customization, and > Set to Backup Value, all of which are wrong (and confusing). ...But this message can still appear when it shouldn't: emacs -Q M-x customize-option RET delete-old-versions Set the variable to Delete, for the current session. Set to Backup Value, to bring back the standard value of delete-old-versions. Then Revert This Session's Customization. This is a bug in `custom-variable-reset-saved': it is not resetting the 'variable-comment property of the variable when there is no saved-value, and that confuses `custom-variable-state'. > Choose Undo Edits anyway. It has no visible effect - State and its menu > stay the same. Again, confusing. > > Choose Set to Backup Value anyway. > > State now says SET for current session only, which is (still) wrong. > And the same menu items are available, except Undo Edits. Choosing Set > to Backup Value again has no visible effect. Choosing Revert This > Session's Customization has the same incorrect effect as before (adds > Undo Edits to the menu and changes State to CHANGED outside Customize. > I think this is just a consequence of the above three bugs. IOW, no new bug here. I'm not sure how to proceed here: should I send different patches for the bugs, or put the fixes altogether in one patch? [-- Attachment #2: Type: text/html, Size: 3076 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-09-01 16:02 ` Mauro Aranda @ 2020-09-01 16:20 ` Stefan Kangas 2020-09-01 20:17 ` Mauro Aranda 0 siblings, 1 reply; 9+ messages in thread From: Stefan Kangas @ 2020-09-01 16:20 UTC (permalink / raw) To: Mauro Aranda, 12864 Mauro Aranda <maurooaranda@gmail.com> writes: > I'm not sure how to proceed here: should I send different patches for > the bugs, or put the fixes altogether in one patch? Some other projects emphasize smaller and logically self-contained patches. Here, the preference definitely seems to be towards making bigger patches rather than many small ones. But I'm not aware of any strict policy in this regard; it seems to be mostly at your own discretion. Someone will correct me if I'm wrong, but my take is this: It's really up to what you feel makes more sense: if they are closely related, you might prefer sending them in one patch; otherwise, it might be better to send the changes in separate patches. (My personal preference is to avoid bundling unrelated changes. But I don't always work like that in Emacs.) ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-09-01 16:20 ` Stefan Kangas @ 2020-09-01 20:17 ` Mauro Aranda 2020-10-22 14:50 ` Mauro Aranda 0 siblings, 1 reply; 9+ messages in thread From: Mauro Aranda @ 2020-09-01 20:17 UTC (permalink / raw) To: Stefan Kangas; +Cc: 12864 [-- Attachment #1.1: Type: text/plain, Size: 606 bytes --] Stefan Kangas <stefankangas@gmail.com> writes: > Mauro Aranda <maurooaranda@gmail.com> writes: > >> I'm not sure how to proceed here: should I send different patches for >> the bugs, or put the fixes altogether in one patch? > > It's really up to what you feel makes more sense: if they are closely > related, you might prefer sending them in one patch; otherwise, it might > be better to send the changes in separate patches. > Hello Stefan, Thanks for your answer. I went with a single patch. If after seeing the patch someone feels it should be split, just let me know. Please review, and thanks. [-- Attachment #1.2: Type: text/html, Size: 827 bytes --] [-- Attachment #2: 0001-Make-State-button-interaction-less-confusing-Bug-128.patch --] [-- Type: text/x-patch, Size: 6439 bytes --] From f2e900d06005541a6084482d07bb014f9251133c Mon Sep 17 00:00:00 2001 From: Mauro Aranda <maurooaranda@gmail.com> Date: Tue, 1 Sep 2020 17:08:32 -0300 Subject: [PATCH] Make State button interaction less confusing (Bug#12864) * lisp/cus-edit.el (custom-variable-current-value): New function. (custom-variable-backup-value): Use it. (custom-variable-set, custom-variable-mark-to-reset-standard): Check that old value is different than the new one. If it is, make a backup. This way, we avoid offering the Set to Backup Value unnecesarily. (custom-variable-reset-saved): Reset the variable-comment property for the variable, to help custom-variable-state be more correct. Also check if we should backup old value. (custom-variable-state): If a variable was set to the standard value, say its state is standard rather than set, which is more correct. Getting the right variable state is important for menu options to be enabled/disabled, and for displaying the right message to the user. --- lisp/cus-edit.el | 59 ++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el index 23ceb3a857..164e865143 100644 --- a/lisp/cus-edit.el +++ b/lisp/cus-edit.el @@ -2789,7 +2789,9 @@ custom-variable-state (and (equal value (eval (car tmp))) (equal comment temp)) (error nil)) - 'set + (if (equal value (eval (car (get symbol 'standard-value)))) + 'standard + 'set) 'changed)) ((progn (setq tmp (get symbol 'theme-value)) (setq temp (get symbol 'saved-variable-comment)) @@ -2859,6 +2861,18 @@ custom-variable-state-set (defun custom-variable-standard-value (widget) (get (widget-value widget) 'standard-value)) +(defun custom-variable-current-value (widget) + "Return the current value of the variable edited by WIDGET. + +WIDGET should be a custom-variable widget." + (let* ((symbol (widget-value widget)) + (get (or (get symbol 'custom-get) 'default-value)) + (type (custom-variable-type symbol)) + (conv (widget-convert type))) + (if (default-boundp symbol) + (funcall get symbol) + (widget-get conv :value)))) + (defvar custom-variable-menu `(("Set for Current Session" custom-variable-set (lambda (widget) @@ -2956,10 +2970,12 @@ custom-variable-set (setq comment nil) ;; Make the comment invisible by hand if it's empty (custom-comment-hide comment-widget)) - (custom-variable-backup-value widget) + (setq val (widget-value child)) + (unless (equal (eval val) (custom-variable-current-value widget)) + (custom-variable-backup-value widget)) (custom-push-theme 'theme-value symbol 'user - 'set (custom-quote (widget-value child))) - (funcall set symbol (eval (setq val (widget-value child)))) + 'set (custom-quote val)) + (funcall set symbol (eval val)) (put symbol 'customized-value (list val)) (put symbol 'variable-comment comment) (put symbol 'customized-variable-comment comment)) @@ -2968,10 +2984,12 @@ custom-variable-set (setq comment nil) ;; Make the comment invisible by hand if it's empty (custom-comment-hide comment-widget)) - (custom-variable-backup-value widget) + (setq val (widget-value child)) + (unless (equal val (custom-variable-current-value widget)) + (custom-variable-backup-value widget)) (custom-push-theme 'theme-value symbol 'user - 'set (custom-quote (widget-value child))) - (funcall set symbol (setq val (widget-value child))) + 'set (custom-quote val)) + (funcall set symbol val) (put symbol 'customized-value (list (custom-quote val))) (put symbol 'variable-comment comment) (put symbol 'customized-variable-comment comment))) @@ -3040,17 +3058,23 @@ custom-variable-reset-saved (let* ((symbol (widget-value widget)) (saved-value (get symbol 'saved-value)) (comment (get symbol 'saved-variable-comment)) + (old-value (custom-variable-current-value widget)) value) - (custom-variable-backup-value widget) (if (not (or saved-value comment)) - ;; If there is no saved value, remove the setting. - (custom-push-theme 'theme-value symbol 'user 'reset) + (progn + (setq value (car (get symbol 'standard-value))) + ;; If there is no saved value, remove the setting. + (custom-push-theme 'theme-value symbol 'user 'reset) + ;; And reset this property too. + (put symbol 'variable-comment nil)) (setq value (car-safe saved-value)) (custom-push-theme 'theme-value symbol 'user 'set value) (put symbol 'variable-comment comment)) + (unless (equal (eval value) old-value) + (custom-variable-backup-value widget)) (ignore-errors (funcall (or (get symbol 'custom-set) #'set-default) symbol - (eval (or value (car (get symbol 'standard-value)))))) + (eval value))) (put symbol 'customized-value nil) (put symbol 'customized-variable-comment nil) (widget-put widget :custom-state 'unknown) @@ -3063,7 +3087,9 @@ custom-variable-mark-to-reset-standard redraw the widget immediately." (let* ((symbol (widget-value widget))) (if (get symbol 'standard-value) - (custom-variable-backup-value widget) + (unless (equal (custom-variable-current-value widget) + (eval (car (get symbol 'standard-value)))) + (custom-variable-backup-value widget)) (user-error "No standard setting known for %S" symbol)) (put symbol 'variable-comment nil) (put symbol 'customized-value nil) @@ -3100,13 +3126,8 @@ custom-variable-reset-standard (defun custom-variable-backup-value (widget) "Back up the current value for WIDGET's variable. The backup value is kept in the car of the `backup-value' property." - (let* ((symbol (widget-value widget)) - (get (or (get symbol 'custom-get) 'default-value)) - (type (custom-variable-type symbol)) - (conv (widget-convert type)) - (value (if (default-boundp symbol) - (funcall get symbol) - (widget-get conv :value)))) + (let ((symbol (widget-value widget)) + (value (custom-variable-current-value widget))) (put symbol 'backup-value (list value)))) (defun custom-variable-reset-backup (widget) -- 2.28.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-09-01 20:17 ` Mauro Aranda @ 2020-10-22 14:50 ` Mauro Aranda 2020-10-22 14:56 ` Lars Ingebrigtsen 0 siblings, 1 reply; 9+ messages in thread From: Mauro Aranda @ 2020-10-22 14:50 UTC (permalink / raw) To: Stefan Kangas; +Cc: 12864 [-- Attachment #1: Type: text/plain, Size: 298 bytes --] tags 12864 patch quit Mauro Aranda <maurooaranda@gmail.com> writes: > I went with a single patch. If after seeing the patch someone feels it > should be split, just let me know. ping. I've been using Customize with this patch, without noticing any trouble. I hope someone can review it soon. [-- Attachment #2: Type: text/html, Size: 415 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-10-22 14:50 ` Mauro Aranda @ 2020-10-22 14:56 ` Lars Ingebrigtsen 2020-10-22 15:14 ` Mauro Aranda 2020-10-22 15:51 ` Drew Adams 0 siblings, 2 replies; 9+ messages in thread From: Lars Ingebrigtsen @ 2020-10-22 14:56 UTC (permalink / raw) To: Mauro Aranda; +Cc: Stefan Kangas, 12864 Mauro Aranda <maurooaranda@gmail.com> writes: > I've been using Customize with this patch, without noticing any > trouble. I hope someone can review it soon. Skimming it, it looks fine to me, so I've pushed it to the trunk. And if there's any more outstanding patches of yours that has fallen between the cracks, please do ping the relevant bug reports. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-10-22 14:56 ` Lars Ingebrigtsen @ 2020-10-22 15:14 ` Mauro Aranda 2020-10-22 15:51 ` Drew Adams 1 sibling, 0 replies; 9+ messages in thread From: Mauro Aranda @ 2020-10-22 15:14 UTC (permalink / raw) To: Lars Ingebrigtsen; +Cc: Stefan Kangas, 12864 [-- Attachment #1: Type: text/plain, Size: 574 bytes --] Lars Ingebrigtsen <larsi@gnus.org> writes: > Mauro Aranda <maurooaranda@gmail.com> writes: > >> I've been using Customize with this patch, without noticing any >> trouble. I hope someone can review it soon. > > Skimming it, it looks fine to me, so I've pushed it to the trunk. Thanks. > And if there's any more outstanding patches of yours that has fallen > between the cracks, please do ping the relevant bug reports. I think I've run out of those, unfortunately. All I have is some tests that demonstrate the problem in Bug#21355, but I haven't worked in a fix yet. [-- Attachment #2: Type: text/html, Size: 806 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-10-22 14:56 ` Lars Ingebrigtsen 2020-10-22 15:14 ` Mauro Aranda @ 2020-10-22 15:51 ` Drew Adams 2020-10-23 12:04 ` Mauro Aranda 1 sibling, 1 reply; 9+ messages in thread From: Drew Adams @ 2020-10-22 15:51 UTC (permalink / raw) To: Lars Ingebrigtsen, Mauro Aranda; +Cc: Stefan Kangas, 12864 Thanks for working on this, Mauro. I haven't checked the fix, but I'm sure it's good. ^ permalink raw reply [flat|nested] 9+ messages in thread
* bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior 2020-10-22 15:51 ` Drew Adams @ 2020-10-23 12:04 ` Mauro Aranda 0 siblings, 0 replies; 9+ messages in thread From: Mauro Aranda @ 2020-10-23 12:04 UTC (permalink / raw) To: Drew Adams; +Cc: 12864 [-- Attachment #1: Type: text/plain, Size: 174 bytes --] Drew Adams <drew.adams@oracle.com> writes: > Thanks for working on this, Mauro. > I haven't checked the fix, but I'm sure it's good. No need to thank me, it's my pleasure. [-- Attachment #2: Type: text/html, Size: 282 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-10-23 12:04 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-11 20:25 bug#12864: 24.3.50; bad & confusing Customize `State' Menu behavior Drew Adams 2020-09-01 16:02 ` Mauro Aranda 2020-09-01 16:20 ` Stefan Kangas 2020-09-01 20:17 ` Mauro Aranda 2020-10-22 14:50 ` Mauro Aranda 2020-10-22 14:56 ` Lars Ingebrigtsen 2020-10-22 15:14 ` Mauro Aranda 2020-10-22 15:51 ` Drew Adams 2020-10-23 12:04 ` Mauro Aranda
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).