bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

The user option custom-theme-directory is supposed to hold a directory
name.  But its :type is a string.

I realize that it isn't really a bug, but in order to offer better
completion, at least, I'd like to change the type to be directory.  So
I'm filing this bug report to see if people would object to such a
change.

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Eli Zaretskii]

> Date: Sun, 20 Aug 2023 07:48:09 -0300
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> The user option custom-theme-directory is supposed to hold a directory
> name.  But its :type is a string.
> 
> I realize that it isn't really a bug, but in order to offer better
> completion, at least, I'd like to change the type to be directory.  So
> I'm filing this bug report to see if people would object to such a
> change.

Does the directory have to exist?

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 20 Aug 2023 07:48:09 -0300
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> The user option custom-theme-directory is supposed to hold a directory
 >> name.  But its :type is a string.
 >>
 >> I realize that it isn't really a bug, but in order to offer better
 >> completion, at least, I'd like to change the type to be directory.  So
 >> I'm filing this bug report to see if people would object to such a
 >> change.
 >
 > Does the directory have to exist?

I don't think so.  And I don't see a reason to force the existence by
putting a non-nil :must-match property.

The function custom-theme--load-path takes care of filtering out
custom-theme-directory if it doesn't exist.

And to check I evaled:
(defcustom custom-theme-directory user-emacs-directory
   "Default user directory for storing custom theme files.
The command `customize-create-theme' writes theme files into this
directory.  By default, Emacs searches for custom themes in this
directory first---see `custom-theme-load-path'."
   :initialize #'custom-initialize-delay
   :type 'directory
   :group 'customize
   :version "22.1")

M-x customize-option RET custom-theme-directory
Changed the value to something like "~/nonexistent" and set it for the
session.
M-x customize-create-theme
Answer y at the prompt
Give the theme a dummy name.
Action the Save Theme button
The theme was created under: ~/nonexistent/dummy-theme.el

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Eli Zaretskii]

> Date: Sun, 20 Aug 2023 08:28:48 -0300
> Cc: 65401@debbugs.gnu.org
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>  > Does the directory have to exist?
> 
> I don't think so.  And I don't see a reason to force the existence by
> putting a non-nil :must-match property.
> 
> The function custom-theme--load-path takes care of filtering out
> custom-theme-directory if it doesn't exist.

If the directory doesn't have to exist, then what is the difference
between :directory and :string?

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 20 Aug 2023 08:28:48 -0300
 >> Cc: 65401@debbugs.gnu.org
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Eli Zaretskii <eliz@gnu.org> writes:
 >>
 >>  > Does the directory have to exist?
 >>
 >> I don't think so.  And I don't see a reason to force the existence by
 >> putting a non-nil :must-match property.
 >>
 >> The function custom-theme--load-path takes care of filtering out
 >> custom-theme-directory if it doesn't exist.
 >
 > If the directory doesn't have to exist, then what is the difference
 > between :directory and :string?

Completion when editing via Customize is the only one it comes to my
mind right now (useful when the user wants to edit it to some directory
it already exists).  Compare the results of:

emacs -Q
M-x customize-option RET custom-theme-directory
Enter ~/
M-TAB

and

emacs -Q
Eval the following
(defcustom custom-theme-directory user-emacs-directory
   "Default user directory for storing custom theme files.
The command `customize-create-theme' writes theme files into this
directory.  By default, Emacs searches for custom themes in this
directory first---see `custom-theme-load-path'."
   :initialize #'custom-initialize-delay
   :type 'directory
   :group 'customize
   :version "22.1")
M-x customize-option RET custom-theme-directory
Enter ~/
M-TAB

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Eli Zaretskii]

> Date: Sun, 20 Aug 2023 09:35:17 -0300
> Cc: 65401@debbugs.gnu.org
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
>  > If the directory doesn't have to exist, then what is the difference
>  > between :directory and :string?
> 
> Completion when editing via Customize is the only one it comes to my
> mind right now (useful when the user wants to edit it to some directory
> it already exists).

Then I don't see why would anyone object to the change you propose.
It has no downsides, AFAICT.

Thanks.

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

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

Eli Zaretskii <eliz@gnu.org> writes:

 >> Date: Sun, 20 Aug 2023 09:35:17 -0300
 >> Cc: 65401@debbugs.gnu.org
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >>
 >> Eli Zaretskii <eliz@gnu.org> writes:
 >>
 >>  > If the directory doesn't have to exist, then what is the difference
 >>  > between :directory and :string?
 >>
 >> Completion when editing via Customize is the only one it comes to my
 >> mind right now (useful when the user wants to edit it to some directory
 >> it already exists).
 >
 > Then I don't see why would anyone object to the change you propose.
 > It has no downsides, AFAICT.
 >
 > Thanks.

OK, thank you.  I attach the patch.

[-- Attachment #2: 0001-Improve-type-of-custom-theme-directory-Bug-65401.patch --]
[-- Type: text/x-patch, Size: 933 bytes --]

From c34dea06956f2a3d098badf9cea089bee0f7b61b Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 20 Aug 2023 10:39:49 -0300
Subject: [PATCH] Improve type of custom-theme-directory (Bug#65401)

* lisp/custom.el (custom-theme-directory): Make it of type directory.
---
 lisp/custom.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 083349e3591..5b244464d0b 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -1208,9 +1208,9 @@ custom-theme-directory
 directory.  By default, Emacs searches for custom themes in this
 directory first---see `custom-theme-load-path'."
   :initialize #'custom-initialize-delay
-  :type 'string
+  :type 'directory
   :group 'customize
-  :version "22.1")
+  :version "30.1")
 
 (defvar custom-theme-load-path (list 'custom-theme-directory t)
   "List of directories to search for custom theme files.
-- 
2.34.1

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Philip Kaludercic]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> Date: Sun, 20 Aug 2023 09:35:17 -0300
>>> Cc: 65401@debbugs.gnu.org
>>> From: Mauro Aranda <maurooaranda@gmail.com>
>>>
>>> Eli Zaretskii <eliz@gnu.org> writes:
>>>
>>>  > If the directory doesn't have to exist, then what is the difference
>>>  > between :directory and :string?
>>>
>>> Completion when editing via Customize is the only one it comes to my
>>> mind right now (useful when the user wants to edit it to some directory
>>> it already exists).
>>
>> Then I don't see why would anyone object to the change you propose.
>> It has no downsides, AFAICT.
>>
>> Thanks.
>
> OK, thank you.  I attach the patch.
>
> From c34dea06956f2a3d098badf9cea089bee0f7b61b Mon Sep 17 00:00:00 2001
> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Sun, 20 Aug 2023 10:39:49 -0300
> Subject: [PATCH] Improve type of custom-theme-directory (Bug#65401)
>
> * lisp/custom.el (custom-theme-directory): Make it of type directory.
> ---
>  lisp/custom.el | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/lisp/custom.el b/lisp/custom.el
> index 083349e3591..5b244464d0b 100644
> --- a/lisp/custom.el
> +++ b/lisp/custom.el
> @@ -1208,9 +1208,9 @@ custom-theme-directory
>  directory.  By default, Emacs searches for custom themes in this
>  directory first---see `custom-theme-load-path'."
>    :initialize #'custom-initialize-delay
> -  :type 'string
> +  :type 'directory
>    :group 'customize
> -  :version "22.1")
> +  :version "30.1")

Does the version tag have to be bumped, for a metadata change this like?

>  
>  (defvar custom-theme-load-path (list 'custom-theme-directory t)
>    "List of directories to search for custom theme files.

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: Eli Zaretskii <eliz@gnu.org>,  65401@debbugs.gnu.org
> Date: Mon, 21 Aug 2023 08:00:35 +0000
> 
> Mauro Aranda <maurooaranda@gmail.com> writes:
> 
> > Eli Zaretskii <eliz@gnu.org> writes:
> >
> >>> Date: Sun, 20 Aug 2023 09:35:17 -0300
> >>> Cc: 65401@debbugs.gnu.org
> >>> From: Mauro Aranda <maurooaranda@gmail.com>
> >>>
> >>> Eli Zaretskii <eliz@gnu.org> writes:
> >>>
> >>>  > If the directory doesn't have to exist, then what is the difference
> >>>  > between :directory and :string?
> >>>
> >>> Completion when editing via Customize is the only one it comes to my
> >>> mind right now (useful when the user wants to edit it to some directory
> >>> it already exists).
> >>
> >> Then I don't see why would anyone object to the change you propose.
> >> It has no downsides, AFAICT.
> >>
> >> Thanks.
> >
> > OK, thank you.  I attach the patch.
> >
> > From c34dea06956f2a3d098badf9cea089bee0f7b61b Mon Sep 17 00:00:00 2001
> > From: Mauro Aranda <maurooaranda@gmail.com>
> > Date: Sun, 20 Aug 2023 10:39:49 -0300
> > Subject: [PATCH] Improve type of custom-theme-directory (Bug#65401)
> >
> > * lisp/custom.el (custom-theme-directory): Make it of type directory.
> > ---
> >  lisp/custom.el | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/lisp/custom.el b/lisp/custom.el
> > index 083349e3591..5b244464d0b 100644
> > --- a/lisp/custom.el
> > +++ b/lisp/custom.el
> > @@ -1208,9 +1208,9 @@ custom-theme-directory
> >  directory.  By default, Emacs searches for custom themes in this
> >  directory first---see `custom-theme-load-path'."
> >    :initialize #'custom-initialize-delay
> > -  :type 'string
> > +  :type 'directory
> >    :group 'customize
> > -  :version "22.1")
> > +  :version "30.1")
> 
> Does the version tag have to be bumped, for a metadata change this like?

Probably not.  AFAIK, this is only used for showing "new and changed"
options when users upgrade, and this change will be invisible at user
level.

Stefan, am I missing something?

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > -  :type 'string
>> > +  :type 'directory
>> >    :group 'customize
>> > -  :version "22.1")
>> > +  :version "30.1")
>>
>> Does the version tag have to be bumped, for a metadata change this like?
>
> Probably not.  AFAIK, this is only used for showing "new and changed"
> options when users upgrade, and this change will be invisible at
> user level.

Agreed.


        Stefan

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

Philip Kaludercic <philipk@posteo.net> writes:

 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >
 >>
 >> OK, thank you.  I attach the patch.
 >>
 >> From c34dea06956f2a3d098badf9cea089bee0f7b61b Mon Sep 17 00:00:00 2001
 >> From: Mauro Aranda <maurooaranda@gmail.com>
 >> Date: Sun, 20 Aug 2023 10:39:49 -0300
 >> Subject: [PATCH] Improve type of custom-theme-directory (Bug#65401)
 >>
 >> * lisp/custom.el (custom-theme-directory): Make it of type directory.
 >> ---
 >>  lisp/custom.el | 4 ++--
 >>  1 file changed, 2 insertions(+), 2 deletions(-)
 >>
 >> diff --git a/lisp/custom.el b/lisp/custom.el
 >> index 083349e3591..5b244464d0b 100644
 >> --- a/lisp/custom.el
 >> +++ b/lisp/custom.el
 >> @@ -1208,9 +1208,9 @@ custom-theme-directory
 >>  directory.  By default, Emacs searches for custom themes in this
 >>  directory first---see `custom-theme-load-path'."
 >>    :initialize #'custom-initialize-delay
 >> -  :type 'string
 >> +  :type 'directory
 >>    :group 'customize
 >> -  :version "22.1")
 >> +  :version "30.1")
 >
 > Does the version tag have to be bumped, for a metadata change this like?
 >

I wasn't sure.  But now I can see why it was wrong to update it. Thank
you for noticing it.

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Mauro Aranda]

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

 >>> > -  :type 'string
 >>> > +  :type 'directory
 >>> >    :group 'customize
 >>> > -  :version "22.1")
 >>> > +  :version "30.1")
 >>>
 >>> Does the version tag have to be bumped, for a metadata change this 
like?
 >>
 >> Probably not.  AFAIK, this is only used for showing "new and changed"
 >> options when users upgrade, and this change will be invisible at
 >> user level.
 >
 > Agreed.
 >
 >
 >         Stefan

Sending an updated patch.  Thanks.

[-- Attachment #2: 0001-Improve-type-of-custom-theme-directory-Bug-65401.patch --]
[-- Type: text/x-patch, Size: 786 bytes --]

From a79d01b62d46ffe121e0b7db26803d4fecc30105 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Sun, 20 Aug 2023 10:39:49 -0300
Subject: [PATCH] Improve type of custom-theme-directory (Bug#65401)

* lisp/custom.el (custom-theme-directory): Make it of type directory.
---
 lisp/custom.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 083349e3591..5c134ff7f13 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -1208,7 +1208,7 @@ custom-theme-directory
 directory.  By default, Emacs searches for custom themes in this
 directory first---see `custom-theme-load-path'."
   :initialize #'custom-initialize-delay
-  :type 'string
+  :type 'directory
   :group 'customize
   :version "22.1")
 
-- 
2.34.1

bug#65401: 30.0.50; custom-theme-directory :type could be enhanced

[Eli Zaretskii]

> Date: Mon, 21 Aug 2023 11:46:26 -0300
> Cc: Philip Kaludercic <philipk@posteo.net>, 65401@debbugs.gnu.org
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
> 
>  >>> > -  :type 'string
>  >>> > +  :type 'directory
>  >>> >    :group 'customize
>  >>> > -  :version "22.1")
>  >>> > +  :version "30.1")
>  >>>
>  >>> Does the version tag have to be bumped, for a metadata change this 
> like?
>  >>
>  >> Probably not.  AFAIK, this is only used for showing "new and changed"
>  >> options when users upgrade, and this change will be invisible at
>  >> user level.
>  >
>  > Agreed.
>  >
>  >
>  >         Stefan
> 
> Sending an updated patch.  Thanks.

Thanks, installed on the master branch, and closing the bug.