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