RE: On obsoleting defcustoms

RE: On obsoleting defcustoms

[Drew Adams]

> > Would it make sense to change ignored options such as this into
> > defvars instead?
> 
> What problem would that solve?  Not all obsolete defcustoms have no
> effect, do they?
> 
> > 2. Finding out that an option is obsolete
> >
> > The obsolete options use a different face.  However, it's not obvious
> > that this is the meaning of that face.
> 
> IMO, we shouldn't show obsolete options at all in a Custom buffer, for
> the same reason why we remove them from the manuals.

Just because something is declared obsolete, that doesn't
(normally) mean that it's not supported or that it doesn't
still work.

Dunno whether that's true (always? sometimes? never?) for Emacs.

But if it is, then why would it make any more sense to remove
customization of an obsolete option that it would to remove
advising an obsolete function or setting an obsolete defvar?

If people are concerned about someone continuing to use
something that's obsolete, why not just have Customize give
a warning/message saying that the option is obsolete, and 
that the effect of changing its value is undefined?

That's assuming that Emacs takes the (unusual, IME) point of
view that, once declared obsolete, something should no longer
be usable.  And in that case, the right way to handle it would
be to raise an error when it's used.  We don't, thankfully, do
that.

RE: On obsoleting defcustoms

[Stefan Kangas]

[[ For context, I have just filed this bug:
   bug#44598: [PATCH] Do not show obsolete options in customize ]]

Drew Adams <drew.adams@oracle.com> writes:

> If people are concerned about someone continuing to use
> something that's obsolete, why not just have Customize give
> a warning/message saying that the option is obsolete

That's what we do now.  See `M-x customize-group RET browse-url RET' in
Emacs 27 for a bad case of what that might look like.

> That's assuming that Emacs takes the (unusual, IME) point of
> view that, once declared obsolete, something should no longer
> be usable.

It's still usable with the patch, just not advertised.

RE: On obsoleting defcustoms

[Drew Adams]

> > If people are concerned about someone continuing to use
> > something that's obsolete, why not just have Customize give
> > a warning/message saying that the option is obsolete
> 
> That's what we do now.  See `M-x customize-group RET browse-url RET'

I don't see any such warnings/messages there, until you
open the full doc string of an option.  If every option
in the group is obsolete, and so is the group itself
(which should presumably follow), then one might expect
a notification at the top (i.e. customize-group) level.

> in Emacs 27 for a bad case of what that might look like.

What's bad about it?

If all of those options still "work", a user should be
able to make use of Customize to change their values.

And if they don't work then there should be no supporting
code, and they'd be unrecognized - raise an error if
referenced in any way.  Typically, deprecated/obsolete !=
unsupported.  Does Emacs take the point of view that all
of this is unsupported?  If so, remove its code, so using
raises an error.

> > That's assuming that Emacs takes the (unusual, IME) point of
> > view that, once declared obsolete, something should no longer
> > be usable.
> 
> It's still usable with the patch, just not advertised.

Not advertising is fine.  Telling users, including in the
Customize UI, that something is obsolete is also fine.
What's not fine, IMO, is to remove it from Customize.  If
something is removed from Customize then it's not the case
that it's still usable with Customize (or Customize is
still usable for it).

RE: On obsoleting defcustoms

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

>> That's what we do now.  See `M-x customize-group RET browse-url RET'
>
> I don't see any such warnings/messages there, until you
> open the full doc string of an option.

We have the face `custom-variable-obsolete', but indeed the warning is
unfortunately only made explicit on the help screen.

>> in Emacs 27 for a bad case of what that might look like.
>
> What's bad about it?

There are more obsolete options than relevant ones.  (Including, e.g.,
the options for the now wildly irrelevant Mosaic browser.)

> If all of those options still "work", a user should be
> able to make use of Customize to change their values.
>
> And if they don't work then there should be no supporting
> code, and they'd be unrecognized - raise an error if
> referenced in any way.  Typically, deprecated/obsolete !=
> unsupported.  Does Emacs take the point of view that all
> of this is unsupported?  If so, remove its code, so using
> raises an error.

The problem is that, AFAICT, it is not really feasible to have a
one-size-fits-all for how we go about deprecating options.  In some
cases it makes sense for them to continue to have effect during the
obsoletion period, but in other cases it does not.

I for one was bitten by this trying to customize an option that turned
out to simply no longer have any effect.  Should it have just been
removed in this case?  Well, it would of course have helped me.  But
third-party code that tried to use it would get the signal "Symbol's
value as variable is void" at run-time, instead of the much gentler
byte-compiler warning that it is obsolete.

Not showing it in `M-x customize-group' seems like a good compromise.

> What's not fine, IMO, is to remove it from Customize.  If
> something is removed from Customize then it's not the case
> that it's still usable with Customize (or Customize is
> still usable for it).

The current proposal as discussed in the bug will keep both
`customize-option' and `customize-saved'.  So you can still customize
them using customize.

RE: On obsoleting defcustoms

[Drew Adams]

> > And if they don't work then there should be no supporting
> > code, and they'd be unrecognized - raise an error if
> > referenced in any way.  Typically, deprecated/obsolete !=
> > unsupported.  Does Emacs take the point of view that all
> > of this is unsupported?  If so, remove its code, so using
> > raises an error.
> 
> The problem is that, AFAICT, it is not really feasible to have a
> one-size-fits-all for how we go about deprecating options.  In some
> cases it makes sense for them to continue to have effect during the
> obsoletion period, but in other cases it does not.
> 
> I for one was bitten by this trying to customize an option
> that turned out to simply no longer have any effect.

But are you then applying your lesson from that one
option to all of these options?  Doesn't that
contradict your previous paragraph?

(And there are likely some non-obsolete options that in
some cases have no effect.)  But see what I wrote above:
if an option no longer works then we should raise an
error when it's referenced - it should be desupported.

Obsolete should mean still works and is still supported,
but is no longer being actively developed.  Desupport
means the code supporting it is gone and we raise an
error instead.

> Should it have just been
> removed in this case?  Well, it would of course have helped me.  But
> third-party code that tried to use it would get the signal "Symbol's
> value as variable is void" at run-time, instead of the much gentler
> byte-compiler warning that it is obsolete.

It's either one or the other, no?

If it doesn't work then users deserve the runtime error.
In that case, what good is a byte-compiler message
intended to warn you to move away from using it?

If it does work, and we just want you to move away from
it, then a warning makes sense.  What are we warning
about?  The fact that it might become desupported at
some point.  That's the point of a deprecation notice
and warnings.

> Not showing it in `M-x customize-group' seems like a
> good compromise.

If it no longer works, yes.  But in that case we
should raise an error, not issue a compiler warning.

If it does still work then I see no reason why we'd
remove it from `customize-group'.  Especially if
`customize-option' still works etc.  This makes no
sense to me.  (Just one opinion.)

> > What's not fine, IMO, is to remove it from Customize.  If
> > something is removed from Customize then it's not the case
> > that it's still usable with Customize (or Customize is
> > still usable for it).
> 
> The current proposal as discussed in the bug will keep both
> `customize-option' and `customize-saved'.  So you can still customize
> them using customize.

See above.  I don't see how that helps users.

RE: On obsoleting defcustoms

[Stefan Kangas]

Drew Adams <drew.adams@oracle.com> writes:

> Obsolete should mean still works and is still supported,
> but is no longer being actively developed.  Desupport
> means the code supporting it is gone and we raise an
> error instead.

Is "desupported" defined in Emacs development?  Searching online, I
was only able to find that word used in reference to Oracle Database.

I think we talk about "making obsolete" and "removing" a
variable/function/face, and their definitions are:

a) Obsolete means the byte-compiler warns about their use
b) Removed means it no longer exists

Note that we have many obsolete variables that are declared to be "no
longer used", that is, they have no effect.  You are free to argue
against the status quo, of course, but that is what we have.

> If it doesn't work then users deserve the runtime error.

I don't think raising a run-time error is wise just because we decided
that a variable should have no effect.  That would mean gratuitously
breaking code that might otherwise be working.  It is better to allow
for a grace period by raising an obsoletion warning.

> I don't see how that helps users.

The point IMO is that advertising features that we are planning to
remove does not help users.  On the contrary, we should recommend
moving away from obsolete features.

RE: On obsoleting defcustoms

[Drew Adams]

> > Obsolete should mean still works and is still supported,
> > but is no longer being actively developed.  Desupport
> > means the code supporting it is gone and we raise an
> > error instead.
> 
> Is "desupported" defined in Emacs development?  Searching online, I
> was only able to find that word used in reference to Oracle Database.

No idea, and I don't care what terms we use.

There's often (usually?) a distinction between these 3
levels of support (these are descriptions, not names):

1. Actively supported and developed.  If you file a
   bug or suggest an enhancement it will likely be
   looked at, at some point.

2. Not actively supported and developed, but still in
   the code, still working (but support for fixing bugs
   may not be there or may be reduced), and maybe even
   still documented to some extent.

3. Not supported at all.  Doesn't work.  Not in the code.
   Not documented (except old doc out on the wild web).

Each is a gray area, and different organizations have
different policies and draw lines in different places.

In principle, at least, #2 is transitional, or at least
users are told that the feature _might_ at some point
in the future change to status #3.  Things may continue
to work, but you're recommended to use the preferred
alternative/replacement (if any).

Since in #3 the feature is absent - doesn't work, you
generally get an error if you try somehow to use it.

The transition from #1 to #2 is sometimes called
deprecation, and in #2 the feature is sometimes called
obsolete or deprecated.  In #3, the feature no longer
exists, and it's sometimes said to be unsupported.
The transition from #2 to #3 is sometimes called
desupport.

There's no obligation to move from #2 to #3, BTW.  At
least in business, where there can be particularly
important customers, some features may be deprecated
(move to #2) with no intention or expectation that they
will ever really be moved to #3.

Users might not be told that, however, since the idea
is to get them to abandon the deprecated feature.
But some considerations can come into play that make
it impossible or undesirable to ever really remove a
feature altogether.

> I think we talk about "making obsolete" and "removing" a
> variable/function/face, and their definitions are:
> 
> a) Obsolete means the byte-compiler warns about their use
> b) Removed means it no longer exists

OK, that sounds to me like #2 and #3, respectively.

> Note that we have many obsolete variables that are declared to be "no
> longer used", that is, they have no effect.  You are free to argue
> against the status quo, of course, but that is what we have.

Maybe so.

We also have lots of stuff that's called obsolete and
that still works - thank goodness, IMO!  It may no
longer be used much by the distributed Emacs code, but
it may well still be used by Emacs users.

And even if an obsolete feature is no longer used, that
doesn't necessarily mean that it would have no effect
if someone did use it.

If something really has no effect then there's no reason
to warn about using it.  There may be, and often is, a
reason to retain code that just raises an error for
something that's unsupported (which isn't quite the same
thing as having no effect, but it's at least not having
the expected effect).

> > If it doesn't work then users deserve the runtime error.
> 
> I don't think raising a run-time error is wise just because we decided
> that a variable should have no effect.  That would mean gratuitously
> breaking code that might otherwise be working.  It is better to allow
> for a grace period by raising an obsoletion warning.

It sounds like we maybe have different understandings
of "have no effect".

A variable that has no effect isn't a variable.  It's
not even an unbound variable.  But yes, if you want to
remove a variable from having its formerly expected
effect, then unbind it so users get an unbound error.

A variable that continues to have its expected effect,
or at least some semblance of that, and that you're
warned/recommended not to continue to use, is just
obsolete - it's not yet missing/removed.

> > I don't see how that helps users.
> 
> The point IMO is that advertising features that we are planning to
> remove does not help users.  On the contrary, we should recommend
> moving away from obsolete features.

Yes, a warning is appropriate in that case.  Removing
the feature is something else.

You're keeping `customize-option' for such a variable,
but you're removing it from `customize-group'.  To me
that's not a great approach, but it's not a big deal.

And I wouldn't call keeping either of those customize
possibilities "advertising".  If you remove the
possibility of using an option as an option, i.e.,
remove its usual customization, then you've removed
more than advertising.

OK, you've only removed some, not all, possibilities
of customizing - just `customize-group', I guess.
You did that to reduce noise in the group UI.  That's
understandable.  I suggested instead keeping the
obsolete options in the group but showing warnings
there for them.  Not a big deal anyway.

These things aren't hard & fast rules.  They're
judgment calls.  And opinions can differ.

Re: On obsoleting defcustoms

[Eli Zaretskii]

> Date: Thu, 12 Nov 2020 17:59:34 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: emacs-devel@gnu.org
> 
> > Not showing it in `M-x customize-group' seems like a
> > good compromise.
> 
> If it no longer works, yes.  But in that case we
> should raise an error, not issue a compiler warning.
> 
> If it does still work then I see no reason why we'd
> remove it from `customize-group'.  Especially if
> `customize-option' still works etc.  This makes no
> sense to me.  (Just one opinion.)

Your opinion is noted.  I think otherwise, and so does Stefan; others
who chimed in didn't think the idea was wrong, either.  So let's agree
to disagree, and please let's stop arguing about this aspect of the
issue.  Our decision is that we want to remove the obsolete options
from display where that makes sense, to advertise them less.

Re: On obsoleting defcustoms

[Eli Zaretskii]

> Date: Thu, 12 Nov 2020 13:37:24 -0800 (PST)
> From: Drew Adams <drew.adams@oracle.com>
> Cc: emacs-devel@gnu.org
> 
> But if it is, then why would it make any more sense to remove
> customization of an obsolete option that it would to remove
> advising an obsolete function or setting an obsolete defvar?

I didn't say we should remove customization of those, I said we should
not show them in the "customize group" buffer.

> If people are concerned about someone continuing to use
> something that's obsolete, why not just have Customize give
> a warning/message saying that the option is obsolete, and 
> that the effect of changing its value is undefined?

We already say that, since we show the doc string.

> That's assuming that Emacs takes the (unusual, IME) point of
> view that, once declared obsolete, something should no longer
> be usable.

Not "usable", "used".

On obsoleting defcustoms

[Stefan Kangas]

I see two issues with how defcustoms are currently being obsoleted.
See `M-x customize-group RET dired RET', for example.

1. Why is Emacs offering to customize something that has no effect?

See "Dired Free Space Args", whose documentation says:

   This variable is obsolete since 27.1;
   ignored, as Emacs uses ‘file-system-info’ instead

Would it make sense to change ignored options such as this into defvars
instead?  (We could keep variables like "Dired Load Hook" as a
defcustom, since it still has an effect.)

2. Finding out that an option is obsolete

The obsolete options use a different face.  However, it's not obvious
that this is the meaning of that face.

One either has to run `what-cursor-position' to see that the face is
named `custom-variable-obsolete', or expand first the option itself, and
then its documentation, and then after having seen a couple of examples
induce that the face means "obsolete".

Could we add some other marker besides just the color to show that these
options have been declared obsolete?  For example by adding the text
"(obsolete)" in a prominent location.

Best regards,
Stefan Kangas

Re: On obsoleting defcustoms

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 18 Sep 2020 06:01:14 -0700
> 
> Would it make sense to change ignored options such as this into defvars
> instead?

What problem would that solve?  Not all obsolete defcustoms have no
effect, do they?

> 2. Finding out that an option is obsolete
> 
> The obsolete options use a different face.  However, it's not obvious
> that this is the meaning of that face.

IMO, we shouldn't show obsolete options at all in a Custom buffer, for
the same reason why we remove them from the manuals.

Re: On obsoleting defcustoms

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> IMO, we shouldn't show obsolete options at all in a Custom buffer, for
> the same reason why we remove them from the manuals.

Yes, that is an even better solution.  Thank you for pointing it out.

bug#44598: [PATCH] Do not show obsolete options in customize

[Stefan Kangas]

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

Severity: wishlist

In a recent discussion on emacs-devel,[1]

Eli Zaretskii <eliz@gnu.org> wrote:

>> The obsolete options use a different face.  However, it's not obvious
>> that this is the meaning of that face.
>
> IMO, we shouldn't show obsolete options at all in a Custom buffer, for
> the same reason why we remove them from the manuals.

How about the attached patch?

[-- Attachment #2: Type: text/plain, Size: 76 bytes --]


Footnotes:
[1]  https://lists.gnu.org/r/emacs-devel/2020-09/msg01620.html


[-- Attachment #3: 0001-Do-not-show-obsolete-options-in-customize.patch --]
[-- Type: text/x-diff, Size: 7533 bytes --]

From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sat, 24 Oct 2020 19:44:20 +0200
Subject: [PATCH] Do not show obsolete options in customize

* lisp/cus-edit.el (custom-variable-obsolete): Delete face.
(custom--filter-obsolete-variables): New defun.
(custom-variable-documentation, custom-variable-value-create)
(custom-group-value-create): Do not show obsolete user options.
* test/lisp/cus-edit-tests.el: New file.
---
 etc/NEWS                    |  4 +++
 lisp/cus-edit.el            | 61 ++++++++++++-------------------------
 test/lisp/cus-edit-tests.el | 46 ++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 42 deletions(-)
 create mode 100644 test/lisp/cus-edit-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index f21c4cb02c..7d064c0c11 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -706,6 +706,10 @@ This file was a compatibility kludge which is no longer needed.
 To revert to the previous behavior,
 '(setq lisp-indent-function 'lisp-indent-function)' from 'lisp-mode-hook'.
 
+** Customize
+
+*** Customize will no longer show obsolete user options.
+
 ** Edebug
 
 +++
diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index d1077d367d..b249b7ef42 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -2505,18 +2505,6 @@ custom-comment-invisible-p
 
 ;;; The `custom-variable' Widget.
 
-(defface custom-variable-obsolete
-  '((((class color) (background dark))
-     :foreground "light blue")
-    (((min-colors 88) (class color) (background light))
-     :foreground "blue1")
-    (((class color) (background light))
-     :foreground "blue")
-    (t :slant italic))
-  "Face used for obsolete variables."
-  :version "27.1"
-  :group 'custom-faces)
-
 (defface custom-variable-tag
   '((((class color) (background dark))
      :foreground "light blue" :weight bold)
@@ -2544,7 +2532,7 @@ custom-variable-documentation
 Normally just return the docstring.  But if VARIABLE automatically
 becomes buffer local when set, append a message to that effect.
 Also append any obsolescence information."
-  (format "%s%s%s" (documentation-property variable 'variable-documentation t)
+  (format "%s%s" (documentation-property variable 'variable-documentation t)
 	  (if (and (local-variable-if-set-p variable)
 		   (or (not (local-variable-p variable))
 		       (with-temp-buffer
@@ -2552,21 +2540,7 @@ custom-variable-documentation
 	      "\n
 This variable automatically becomes buffer-local when set outside Custom.
 However, setting it through Custom sets the default value."
-	    "")
-	  ;; This duplicates some code from describe-variable.
-	  ;; TODO extract to separate utility function?
-	  (let* ((obsolete (get variable 'byte-obsolete-variable))
-		 (use (car obsolete)))
-	    (if obsolete
-		(concat "\n
-This variable is obsolete"
-			(if (nth 2 obsolete)
-			    (format " since %s" (nth 2 obsolete)))
-			(cond ((stringp use) (concat ";\n" use))
-			      (use (format-message ";\nuse `%s' instead."
-						   (car obsolete)))
-			      (t ".")))
-	      ""))))
+            "")))
 
 (define-widget 'custom-variable 'custom
   "A widget for displaying a Custom variable.
@@ -2650,8 +2624,7 @@ custom-variable-value-create
 	 (state (or (widget-get widget :custom-state)
 		    (if (memq (custom-variable-state symbol value)
 			      (widget-get widget :hidden-states))
-			'hidden)))
-	 (obsolete (get symbol 'byte-obsolete-variable)))
+			'hidden))))
 
     ;; If we don't know the state, see if we need to edit it in lisp form.
     (unless state
@@ -2684,9 +2657,7 @@ custom-variable-value-create
 	   (push (widget-create-child-and-convert
 		  widget 'item
 		  :format "%{%t%} "
-		  :sample-face (if obsolete
-				   'custom-variable-obsolete
-				 'custom-variable-tag)
+		  :sample-face 'custom-variable-tag
 		  :tag tag
 		  :parent widget)
 		 buttons))
@@ -2744,9 +2715,7 @@ custom-variable-value-create
 		    :help-echo "Change value of this option."
 		    :mouse-down-action 'custom-tag-mouse-down-action
 		    :button-face 'custom-variable-button
-		    :sample-face (if obsolete
-				     'custom-variable-obsolete
-				   'custom-variable-tag)
+		    :sample-face 'custom-variable-tag
 		    :tag tag)
 		   buttons)
 	     (push (widget-create-child-and-convert
@@ -4232,6 +4201,13 @@ custom-group-visibility-create
 	(insert "--------")))
   (widget-default-create widget))
 
+(defun custom--filter-obsolete-variables (items)
+  "Filter obsolete variables from ITEMS."
+  (seq-filter (lambda (item)
+                (not (and (eq (nth 1 item) 'custom-variable)
+                          (get (nth 0 item) 'byte-obsolete-variable))))
+              items))
+
 (defun custom-group-members (symbol groups-only)
   "Return SYMBOL's custom group members.
 If GROUPS-ONLY is non-nil, return only those members that are groups."
@@ -4437,12 +4413,13 @@ custom-group-value-create
 					     ?\s))
 	   ;; Members.
 	   (message "Creating group...")
-	   (let* ((members (custom-sort-items
-			    members
-			    ;; Never sort the top-level custom group.
-			    (unless (eq symbol 'emacs)
-			      custom-buffer-sort-alphabetically)
-			    custom-buffer-order-groups))
+           (let* ((members (custom--filter-obsolete-variables
+                            (custom-sort-items
+                             members
+                             ;; Never sort the top-level custom group.
+                             (unless (eq symbol 'emacs)
+                               custom-buffer-sort-alphabetically)
+                             custom-buffer-order-groups)))
 		  (prefixes (widget-get widget :custom-prefixes))
 		  (custom-prefix-list (custom-prefix-add symbol prefixes))
 		  (have-subtitle (and (not (eq symbol 'emacs))
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
new file mode 100644
index 0000000000..6fe1ce4a11
--- /dev/null
+++ b/test/lisp/cus-edit-tests.el
@@ -0,0 +1,46 @@
+;;; cus-edit-tests.el --- Tests for cus-edit.el  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'ert-x)
+(require 'cus-edit)
+
+(defgroup cus-edit-tests nil "test"
+  :group 'test-group)
+
+(defcustom cus-edit--testvar-obsolete nil "test"
+  :type 'boolean)
+(make-obsolete-variable 'cus-edit--testvar-obsolete nil "X.X-test")
+
+(ert-deftest cus-edit-tests-customize-group/no-obsolete ()
+  "Check that obsolete variables do not show up."
+  (save-window-excursion
+    (unwind-protect
+        (progn
+          (customize-group 'cus-edit-tests)
+          (should-not (search-forward "Cus Edit Testvar Obsolete" nil t)))
+      (when-let ((buf (get-buffer "*Customize Group: Cus Edit Tests*")))
+        (kill-buffer buf)))))
+
+(provide 'cus-edit-tests)
+;;; cus-edit-tests.el ends here
-- 
2.28.0

bug#44598: [PATCH] Do not show obsolete options in customize

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <stefan@marxist.se>
> Date: Sat, 24 Oct 2020 19:44:20 +0200
> Subject: [PATCH] Do not show obsolete options in customize
>
> * lisp/cus-edit.el (custom-variable-obsolete): Delete face.

Perhaps it should be marked obsolete first?

Thanks,

-- 
Basil

bug#44598: [PATCH] Do not show obsolete options in customize

[Stefan Kangas]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Sat, 24 Oct 2020 19:44:20 +0200
>> Subject: [PATCH] Do not show obsolete options in customize
>>
>> * lisp/cus-edit.el (custom-variable-obsolete): Delete face.
>
> Perhaps it should be marked obsolete first?

It's not clear to me how this is supposed to work.  I see some patches
in the git log that just deletes them, and some that don't.

We also have `define-obsolete-face-alias' but no `make-obsolete-face'.

bug#44598: [PATCH] Do not show obsolete options in customize

[Basil L. Contovounesios]

Stefan Kangas <stefan@marxist.se> writes:

> "Basil L. Contovounesios" <contovob@tcd.ie> writes:
>
>> Stefan Kangas <stefan@marxist.se> writes:
>>
>>> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
>>> From: Stefan Kangas <stefan@marxist.se>
>>> Date: Sat, 24 Oct 2020 19:44:20 +0200
>>> Subject: [PATCH] Do not show obsolete options in customize
>>>
>>> * lisp/cus-edit.el (custom-variable-obsolete): Delete face.
>>
>> Perhaps it should be marked obsolete first?
>
> It's not clear to me how this is supposed to work.  I see some patches
> in the git log that just deletes them, and some that don't.
>
> We also have `define-obsolete-face-alias' but no `make-obsolete-face'.

Looks like face obsoletion is not as first-class a citizen as variable
and function obsoletion.  If you'd rather not bother, that's fine with
me.

-- 
Basil

bug#44598: [PATCH] Do not show obsolete options in customize

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Thu, 12 Nov 2020 21:11:07 +0000
> Cc: 44598@debbugs.gnu.org
> 
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
> > From: Stefan Kangas <stefan@marxist.se>
> > Date: Sat, 24 Oct 2020 19:44:20 +0200
> > Subject: [PATCH] Do not show obsolete options in customize
> >
> > * lisp/cus-edit.el (custom-variable-obsolete): Delete face.
> 
> Perhaps it should be marked obsolete first?

You mean, literally an obsolete face?  Or did you mean a new variable
that would allow displaying the obsolete option as we do today?  For
the latter, it might make sense, as a "fire escape" for people who
want the old behavior.  For the former, perhaps we shouldn't remove
the face at all, see my other mail.

bug#44598: [PATCH] Do not show obsolete options in customize

[Drew Adams]

> In a recent discussion on emacs-devel,[1]

Wow.  I missed that.  I've responded there now.

> > IMO, we shouldn't show obsolete options at all in a Custom buffer,
> > for the same reason why we remove them from the manuals.
> 
> How about the attached patch?

FTR, I disagree with this approach.  I think it hurts
more than it helps.

If customizing some _particular_ option has a very
negative effect, then it could be dealt with specially.
But to just remove customization of all obsolete options
makes no sense to me.  And "for the same reason" as not
documenting something that's obsolete doesn't sound like
a sound reason to me, at all.  Doc is one thing.  Use is
another.

bug#44598: [PATCH] Do not show obsolete options in customize

[Mauro Aranda]

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

Stefan Kangas <stefan@marxist.se> writes:

> --- a/etc/NEWS
> +++ b/etc/NEWS
> @@ -706,6 +706,10 @@ This file was a compatibility kludge which is no
longer needed.
>  To revert to the previous behavior,
>  '(setq lisp-indent-function 'lisp-indent-function)' from
'lisp-mode-hook'.
>
> +** Customize
> +
> +*** Customize will no longer show obsolete user options.
> +

Only when customizing a group, it seems.  They still show up when using
customize-apropos (quite common), customize-saved (for an old setting,
it could be somewhat common), or when asking to customize them directly
(although that could be less common).  I don't know what others think,
but perhaps customize-saved should still show them: after all, it is a
current user saved setting.

> --- a/lisp/cus-edit.el
> +++ b/lisp/cus-edit.el
> @@ -2505,18 +2505,6 @@ custom-comment-invisible-p
>
>  ;;; The `custom-variable' Widget.
>
> -(defface custom-variable-obsolete
> -  '((((class color) (background dark))
> -     :foreground "light blue")
> -    (((min-colors 88) (class color) (background light))
> -     :foreground "blue1")
> -    (((class color) (background light))
> -     :foreground "blue")
> -    (t :slant italic))
> -  "Face used for obsolete variables."
> -  :version "27.1"
> -  :group 'custom-faces)
> -

Because of the above, perhaps it's too early to remove it?

> @@ -2544,7 +2532,7 @@ custom-variable-documentation
>  Normally just return the docstring.  But if VARIABLE automatically
>  becomes buffer local when set, append a message to that effect.
>  Also append any obsolescence information."
> -  (format "%s%s%s" (documentation-property variable
'variable-documentation t)
> +  (format "%s%s" (documentation-property variable
'variable-documentation t)
>    (if (and (local-variable-if-set-p variable)
>     (or (not (local-variable-p variable))
>         (with-temp-buffer
> @@ -2552,21 +2540,7 @@ custom-variable-documentation
>        "\n
>  This variable automatically becomes buffer-local when set outside Custom.
>  However, setting it through Custom sets the default value."
> -    "")
> -  ;; This duplicates some code from describe-variable.
> -  ;; TODO extract to separate utility function?
> -  (let* ((obsolete (get variable 'byte-obsolete-variable))
> - (use (car obsolete)))
> -    (if obsolete
> - (concat "\n
> -This variable is obsolete"
> - (if (nth 2 obsolete)
> -    (format " since %s" (nth 2 obsolete)))
> - (cond ((stringp use) (concat ";\n" use))
> -      (use (format-message ";\nuse `%s' instead."
> -   (car obsolete)))
> -      (t ".")))
> -      ""))))
> +            "")))

And the same goes for this: if the option is still likely to pop up in
some other Custom buffer, then this is useful information we might want
to keep showing to the user.

> +(defun custom--filter-obsolete-variables (items)
> +  "Filter obsolete variables from ITEMS."
> +  (seq-filter (lambda (item)
> +                (not (and (eq (nth 1 item) 'custom-variable)
> +                          (get (nth 0 item) 'byte-obsolete-variable))))
> +              items))
> +

Nit: perhaps seq-remove?

[-- Attachment #2: Type: text/html, Size: 3986 bytes --]

bug#44598: [PATCH] Do not show obsolete options in customize

[Stefan Kangas]

Mauro Aranda <maurooaranda@gmail.com> writes:

>> +*** Customize will no longer show obsolete user options.
>> +
>
> Only when customizing a group, it seems.  They still show up when using
> customize-apropos (quite common), customize-saved (for an old setting,
> it could be somewhat common), or when asking to customize them directly
> (although that could be less common).  I don't know what others think,
> but perhaps customize-saved should still show them: after all, it is a
> current user saved setting.

Thanks, I overlooked that.

I think `customize-option' and `customize-saved' should still show
them, indeed.

Excluding the above, is `customize-apropos' otherwise an exhaustive list
of the commands where they would still be visible?

I never use `customize-apropos', so what do you think makes sense for
that command?  Should it still show it?

> Because of the above, perhaps it's too early to remove it?

Perhaps, yes.  I could just mention in its docstring that it's obsolete
instead, since I can't find any facilities to mark a defface obsolete.
Or maybe someone will enlighten me and tell me how it's done...

> And the same goes for this: if the option is still likely to pop up in
> some other Custom buffer, then this is useful information we might want
> to keep showing to the user.

Good point.  I'll take a look at what happens with `customize-option' in
particular, where we would want to mention that information.

> Nit: perhaps seq-remove?

Sure.

bug#44598: [PATCH] Do not show obsolete options in customize

[Mauro Aranda]

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

Stefan Kangas <stefan@marxist.se> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>>> +*** Customize will no longer show obsolete user options.
>>> +
>>
>> Only when customizing a group, it seems.  They still show up when using
>> customize-apropos (quite common), customize-saved (for an old setting,
>> it could be somewhat common), or when asking to customize them directly
>> (although that could be less common).  I don't know what others think,
>> but perhaps customize-saved should still show them: after all, it is a
>> current user saved setting.
>
> Thanks, I overlooked that.
>
> I think `customize-option' and `customize-saved' should still show
> them, indeed.
>
> Excluding the above, is `customize-apropos' otherwise an exhaustive list
> of the commands where they would still be visible?

There's customize-changed-options too.

> I never use `customize-apropos', so what do you think makes sense for
> that command?  Should it still show it?

To me, it makes sense that it follows what customize-group does, in this
regard.

>> Because of the above, perhaps it's too early to remove it?
>
> Perhaps, yes.  I could just mention in its docstring that it's obsolete
> instead, since I can't find any facilities to mark a defface obsolete.
> Or maybe someone will enlighten me and tell me how it's done...

But if one Custom buffer still shows obsolete options, wouldn't the face
still be in use?

>> And the same goes for this: if the option is still likely to pop up in
>> some other Custom buffer, then this is useful information we might want
>> to keep showing to the user.
>
> Good point.  I'll take a look at what happens with `customize-option' in
> particular, where we would want to mention that information.

Why not in the current place?

[-- Attachment #2: Type: text/html, Size: 2254 bytes --]

bug#44598: [PATCH] Do not show obsolete options in customize

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Thu, 12 Nov 2020 18:44:24 -0300
> Cc: 44598@debbugs.gnu.org
> 
> I don't know what others think, but perhaps customize-saved should
> still show them: after all, it is a current user saved setting.

Yes, I think I agree.

> > -(defface custom-variable-obsolete
> > -  '((((class color) (background dark))
> > -     :foreground "light blue")
> > -    (((min-colors 88) (class color) (background light))
> > -     :foreground "blue1")
> > -    (((class color) (background light))
> > -     :foreground "blue")
> > -    (t :slant italic))
> > -  "Face used for obsolete variables."
> > -  :version "27.1"
> > -  :group 'custom-faces)
> > -
> 
> Because of the above, perhaps it's too early to remove it?

Yes.

bug#44598: [PATCH] Do not show obsolete options in customize

[Eli Zaretskii]

> Cc: eliz@gnu.org
> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 12 Nov 2020 15:56:24 -0500
> 
> In a recent discussion on emacs-devel,[1]

You mean this one:

  https://lists.gnu.org/archive/html/emacs-devel/2020-09/msg01617.html

> > IMO, we shouldn't show obsolete options at all in a Custom buffer, for
> > the same reason why we remove them from the manuals.
> 
> How about the attached patch?

Thanks, some comments:

> +** Customize
> +
> +*** Customize will no longer show obsolete user options.

This is too general, it sounds like those options will not be shown
even if the user types "M-x customize-variable" to explicitly
customize an obsolete option.  That's not what the patch does, does
it?

> -(defface custom-variable-obsolete
> -  '((((class color) (background dark))
> -     :foreground "light blue")
> -    (((min-colors 88) (class color) (background light))
> -     :foreground "blue1")
> -    (((class color) (background light))
> -     :foreground "blue")
> -    (t :slant italic))
> -  "Face used for obsolete variables."
> -  :version "27.1"
> -  :group 'custom-faces)

You are removing the face, but then how will we display obsolete
options when the user actually asks to customize them by name?

> +(defun custom--filter-obsolete-variables (items)
> +  "Filter obsolete variables from ITEMS."

This doesn't say what kind of object is ITEMS expected to be.

> +  (seq-filter (lambda (item)
> +                (not (and (eq (nth 1 item) 'custom-variable)
> +                          (get (nth 0 item) 'byte-obsolete-variable))))
> +              items))

Do we really need the power of seq.el here? wouldn't mapcar do the job
nicely, since we have a simple list here?  OTOH, if you do use seq.el,
why seq-filter and not seq-remove?

> +(ert-deftest cus-edit-tests-customize-group/no-obsolete ()
> +  "Check that obsolete variables do not show up."
> +  (save-window-excursion
> +    (unwind-protect
> +        (progn
> +          (customize-group 'cus-edit-tests)
> +          (should-not (search-forward "Cus Edit Testvar Obsolete" nil t)))
> +      (when-let ((buf (get-buffer "*Customize Group: Cus Edit Tests*")))
> +        (kill-buffer buf)))))

This test will fail when we remove the obsolete option, right?  How
can we come up with a test that doesn't suffer from such maintenance
problems?

bug#44598: [PATCH] Do not show obsolete options in customize

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks, some comments:

Thanks for commenting, as always.

Please find attached an updated patch that should fix all your comments.

> Do we really need the power of seq.el here? wouldn't mapcar do the job
> nicely, since we have a simple list here?  OTOH, if you do use seq.el,
> why seq-filter and not seq-remove?

I think mapcar won't work since the returned list is the same length as
its input.  seq-remove seems better indeed.

> This test will fail when we remove the obsolete option, right?  How
> can we come up with a test that doesn't suffer from such maintenance
> problems?

This option is only defined in this file for testing purposes, and
should never be removed.  I've clarified this in its docstring and added
a high version number.

[-- Attachment #2: 0001-Hide-obsolete-options-in-most-customize-commands.patch --]
[-- Type: text/x-diff, Size: 7067 bytes --]

From 58b144a805dd46b26db4fa266ac124af3146448e Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Sat, 24 Oct 2020 19:44:20 +0200
Subject: [PATCH] Hide obsolete options in most customize commands

* lisp/cus-edit.el (custom--filter-obsolete-variables): New defun.
* lisp/cus-edit.el (customize-changed-options)
(customize-apropos, custom-group-value-create): Hide obsolete user
options.  (Bug#44598)
* test/lisp/cus-edit-tests.el: New file.
---
 etc/NEWS                    | 10 +++++
 lisp/cus-edit.el            | 26 ++++++++----
 test/lisp/cus-edit-tests.el | 80 +++++++++++++++++++++++++++++++++++++
 3 files changed, 108 insertions(+), 8 deletions(-)
 create mode 100644 test/lisp/cus-edit-tests.el

diff --git a/etc/NEWS b/etc/NEWS
index f21c4cb02c..fa9a87d80c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -706,6 +706,16 @@ This file was a compatibility kludge which is no longer needed.
 To revert to the previous behavior,
 '(setq lisp-indent-function 'lisp-indent-function)' from 'lisp-mode-hook'.
 
+** Customize
+
+*** Most customize commands now hides obsolete user options.
+Obsolete user options are no longer shown in the listings produced by
+the commands `customize', `customize-group', `customize-apropos' and
+`customize-changed-options'.
+
+To customize obsolete user options, use `customize-option' or
+`customize-saved'.
+
 ** Edebug
 
 +++
diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index d1077d367d..b46be39e38 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -1298,7 +1298,8 @@ customize-changed-options
 	       (if (custom-facep symbol)
 		   (push (list symbol 'custom-face) found)))))))
     (if found
-	(custom-buffer-create (custom-sort-items found t 'first)
+        (custom-buffer-create (custom--filter-obsolete-variables
+                               (custom-sort-items found t 'first))
 			      "*Customize Changed Options*")
       (user-error "No user option defaults have been changed since Emacs %s"
                   since-version))))
@@ -1504,7 +1505,8 @@ customize-apropos
 						(symbol-name type))
 	     pattern))
     (custom-buffer-create
-     (custom-sort-items found t custom-buffer-order-groups)
+     (custom--filter-obsolete-variables
+      (custom-sort-items found t custom-buffer-order-groups))
      "*Customize Apropos*")))
 
 ;;;###autoload
@@ -4232,6 +4234,13 @@ custom-group-visibility-create
 	(insert "--------")))
   (widget-default-create widget))
 
+(defun custom--filter-obsolete-variables (items)
+  "Filter obsolete variables from ITEMS."
+  (seq-remove (lambda (item)
+                (and (eq (nth 1 item) 'custom-variable)
+                     (get (nth 0 item) 'byte-obsolete-variable)))
+              items))
+
 (defun custom-group-members (symbol groups-only)
   "Return SYMBOL's custom group members.
 If GROUPS-ONLY is non-nil, return only those members that are groups."
@@ -4437,12 +4446,13 @@ custom-group-value-create
 					     ?\s))
 	   ;; Members.
 	   (message "Creating group...")
-	   (let* ((members (custom-sort-items
-			    members
-			    ;; Never sort the top-level custom group.
-			    (unless (eq symbol 'emacs)
-			      custom-buffer-sort-alphabetically)
-			    custom-buffer-order-groups))
+           (let* ((members (custom--filter-obsolete-variables
+                            (custom-sort-items
+                             members
+                             ;; Never sort the top-level custom group.
+                             (unless (eq symbol 'emacs)
+                               custom-buffer-sort-alphabetically)
+                             custom-buffer-order-groups)))
 		  (prefixes (widget-get widget :custom-prefixes))
 		  (custom-prefix-list (custom-prefix-add symbol prefixes))
 		  (have-subtitle (and (not (eq symbol 'emacs))
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
new file mode 100644
index 0000000000..4d148b4f41
--- /dev/null
+++ b/test/lisp/cus-edit-tests.el
@@ -0,0 +1,80 @@
+;;; cus-edit-tests.el --- Tests for cus-edit.el  -*- lexical-binding: t -*-
+
+;; Copyright (C) 2020 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(require 'ert)
+(require 'ert-x)
+(require 'cus-edit)
+
+(defmacro with-cus-edit-test (buffer &rest body)
+  (declare (indent 1))
+  `(save-window-excursion
+     (unwind-protect
+         (progn ,@body)
+       (when-let ((buf (get-buffer ,buffer)))
+         (kill-buffer buf)))))
+
+\f
+;;;; showing/hiding obsolete options
+
+(defgroup cus-edit-tests nil "test"
+  :group 'test-group)
+
+(defcustom cus-edit-tests--obsolete-option-tag nil
+  "This should never be removed; it is obsolete for testing purposes."
+  :type 'boolean
+  :version "917.10") ; a super high version number
+(make-obsolete-variable 'cus-edit-tests--obsolete-option-tag nil "X.X-test")
+(defconst cus-edit-tests--obsolete-option-tag
+  (custom-unlispify-tag-name 'cus-edit-tests--obsolete-option-tag))
+
+(ert-deftest cus-edit-tests-customize-apropos/hide-obsolete ()
+  (with-cus-edit-test "*Customize Apropos*"
+    (customize-apropos "cus-edit-tests")
+    (should-not (search-forward cus-edit-tests--obsolete-option-tag nil t))))
+
+(ert-deftest cus-edit-tests-customize-changed-options/hide-obsolete ()
+  (with-cus-edit-test "*Customize Changed Options*"
+    (customize-changed-options "917.2") ; some future version
+    (should-not (search-forward cus-edit-tests--obsolete-option-tag nil t))))
+
+(ert-deftest cus-edit-tests-customize-group/hide-obsolete ()
+  "Check that obsolete variables do not show up."
+  (with-cus-edit-test "*Customize Group: Cus Edit Tests*"
+    (customize-group 'cus-edit-tests)
+    (should-not (search-forward cus-edit-tests--obsolete-option-tag nil t))))
+
+(ert-deftest cus-edit-tests-customize-option/show-obsolete ()
+  (with-cus-edit-test "*Customize Option: Cus Edit Tests Obsolete Option Tag*"
+    (customize-option 'cus-edit-tests--obsolete-option-tag)
+    (goto-char (point-min))
+    (should (search-forward cus-edit-tests--obsolete-option-tag nil t))))
+
+(ert-deftest cus-edit-tests-customize-saved/show-obsolete ()
+  ;; FIXME: How to test for saved options?
+  :expected-result :failed
+  (with-cus-edit-test "*Customize Saved*"
+    (customize-saved)
+    (should (search-forward cus-edit-tests--obsolete-option-tag nil t))))
+
+(provide 'cus-edit-tests)
+;;; cus-edit-tests.el ends here
-- 
2.29.2

bug#44598: [PATCH] Do not show obsolete options in customize

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 13 Nov 2020 09:10:52 -0800
> Cc: 44598@debbugs.gnu.org
> 
> +** Customize
> +
> +*** Most customize commands now hides obsolete user options.
                                   ^^^^^
"hide", plural.

Otherwise, LGTM, thanks.

bug#44598: [PATCH] Do not show obsolete options in customize

[Stefan Kangas]

tags 44598 fixed
close 44598 28.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Kangas <stefan@marxist.se>
>> Date: Fri, 13 Nov 2020 09:10:52 -0800
>> Cc: 44598@debbugs.gnu.org
>>
>> +** Customize
>> +
>> +*** Most customize commands now hides obsolete user options.
>                                    ^^^^^
> "hide", plural.

Fixed.

> Otherwise, LGTM, thanks.

Thanks, pushed to master as commit b4b1bd6e03.