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

classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
Severity: wishlist

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

Eli Zaretskii <[hidden email]> 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?

attachment0 (106 bytes) Download Attachment
0001-Do-not-show-obsolete-options-in-customize.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
Stefan Kangas <[hidden email]> writes:

> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
> From: Stefan Kangas <[hidden email]>
> 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



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
"Basil L. Contovounesios" <[hidden email]> writes:

> Stefan Kangas <[hidden email]> writes:
>
>> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
>> From: Stefan Kangas <[hidden email]>
>> 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'.



Reply | Threaded
Open this post in threaded view
|

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

Drew Adams
In reply to this post by Stefan Kangas
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Mauro Aranda
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
Mauro Aranda <[hidden email]> 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.



Reply | Threaded
Open this post in threaded view
|

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

Basil L. Contovounesios
In reply to this post by Stefan Kangas
Stefan Kangas <[hidden email]> writes:

> "Basil L. Contovounesios" <[hidden email]> writes:
>
>> Stefan Kangas <[hidden email]> writes:
>>
>>> From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
>>> From: Stefan Kangas <[hidden email]>
>>> 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



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Stefan Kangas
> Cc: [hidden email]
> From: Stefan Kangas <[hidden email]>
> 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?



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Basil L. Contovounesios
> From: "Basil L. Contovounesios" <[hidden email]>
> Date: Thu, 12 Nov 2020 21:11:07 +0000
> Cc: [hidden email]
>
> Stefan Kangas <[hidden email]> writes:
>
> > From 66221f1d0f7ee4f2af0d6c65fe956cce711b48e2 Mon Sep 17 00:00:00 2001
> > From: Stefan Kangas <[hidden email]>
> > 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.



Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
In reply to this post by Mauro Aranda
> From: Mauro Aranda <[hidden email]>
> Date: Thu, 12 Nov 2020 18:44:24 -0300
> Cc: [hidden email]
>
> 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.



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
In reply to this post by Eli Zaretskii
Eli Zaretskii <[hidden email]> 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.

0001-Hide-obsolete-options-in-most-customize-commands.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Eli Zaretskii
> From: Stefan Kangas <[hidden email]>
> Date: Fri, 13 Nov 2020 09:10:52 -0800
> Cc: [hidden email]
>
> +** Customize
> +
> +*** Most customize commands now hides obsolete user options.
                                   ^^^^^
"hide", plural.

Otherwise, LGTM, thanks.



Reply | Threaded
Open this post in threaded view
|

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

Stefan Kangas
tags 44598 fixed
close 44598 28.1
thanks

Eli Zaretskii <[hidden email]> writes:

>> From: Stefan Kangas <[hidden email]>
>> Date: Fri, 13 Nov 2020 09:10:52 -0800
>> Cc: [hidden email]
>>
>> +** Customize
>> +
>> +*** Most customize commands now hides obsolete user options.
>                                    ^^^^^
> "hide", plural.

Fixed.

> Otherwise, LGTM, thanks.

Thanks, pushed to master as commit b4b1bd6e03.