bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

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

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Philipp Stephani

M-x customize-group RET display-fill-column-indicator RET, then expand
the section for `display-fill-column-indicator-character'.  The
customization buffer then contains:

Display Fill Column Indicator group:  Group definition missing.
       State : visible group members are all at standard values.
   
[...]

Hide display-fill-column-indicator-character: nil
    State : STANDARD. (mismatch)
   Character to draw the indicator when ‘display-fill-column-indicator’ is non-nil. More

The two issues here are:

1. "Group definition missing."  It looks like customizing this group
   should load `display-fill-column-indicator-mode', which defines this
   group, or the group definition should be in cus-start.el.

2. "(mismatch)."  The default value for
   `display-fill-column-indicator-character' is nil, but its type is
   `character', so nil isn't allowed.


In GNU Emacs 27.0.91 (build 9, x86_64-pc-linux-gnu, GTK+ Version 3.24.13)
 of 2020-05-09
Repository revision: d5c184aa3e2a183144efa5f269e2c70d2681aa0a
Repository branch: emacs-27
Windowing system distributor 'The X.Org Foundation', version 11.0.12004000
System Description: Debian GNU/Linux rodete

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.

Configured using:
 'configure --enable-checking=all --enable-gtk-deprecation-warnings
 --enable-gcc-warnings=warn-only --enable-check-lisp-object-type
 --with-mailutils --without-pop 'CFLAGS=-O0 -g3' LDFLAGS=-g3'

Configured features:
XPM JPEG TIFF GIF PNG SOUND DBUS GSETTINGS GLIB NOTIFY INOTIFY
LIBSELINUX GNUTLS FREETYPE HARFBUZZ XFT ZLIB TOOLKIT_SCROLL_BARS GTK3
X11 XDBE XIM MODULES THREADS PDUMPER GMP

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc dired dired-loaddefs
format-spec rfc822 mml easymenu mml-sec epa epg epg-config gnus-util
rmail rmail-loaddefs text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils phst skeleton
derived edmacro kmacro pcase ffap thingatpt url url-proxy url-privacy
url-expand url-methods url-history url-cookie url-domsuf url-util
url-parse auth-source cl-seq eieio eieio-core cl-macs eieio-loaddefs
password-cache json map url-vars mailcap subr-x rx gnutls puny seq
byte-opt gv bytecomp byte-compile cconv dbus xml cl-loaddefs cl-lib
tooltip eldoc electric uniquify ediff-hook vc-hooks lisp-float-type
mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode elisp-mode
lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow isearch
timer select scroll-bar mouse jit-lock font-lock syntax facemenu
font-core term/tty-colors frame minibuffer cl-generic cham georgian
utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao korean
japanese eucjp-ms cp51932 hebrew greek romanian slovak czech european
ethiopic indian cyrillic chinese composite charscript charprop
case-table epa-hook jka-cmpr-hook help simple abbrev obarray
cl-preloaded nadvice loaddefs button faces cus-face macroexp files
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting
move-toolbar gtk x-toolkit x multi-tty make-network-process emacs)

Memory information:
((conses 16 63403 5547)
 (symbols 48 8443 1)
 (strings 32 22409 1678)
 (string-bytes 1 711978)
 (vectors 16 12504)
 (vector-slots 8 174118 5994)
 (floats 8 25 34)
 (intervals 56 201 0)
 (buffers 1000 12))

--
Google Germany GmbH
Erika-Mann-Straße 33
80636 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

If you received this communication by mistake, please don’t forward it to
anyone else (it may contain confidential or privileged information), please
erase all copies of it, including all attachments, and please let the sender
know it went to the wrong person.  Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Eli Zaretskii
> From: Philipp Stephani <[hidden email]>
> Date: Sat, 09 May 2020 10:30:13 +0200
>
> The two issues here are:
>
> 1. "Group definition missing."  It looks like customizing this group
>    should load `display-fill-column-indicator-mode', which defines this
>    group, or the group definition should be in cus-start.el.
The same seems to work for display-line-numbers, and I don't think I
see the crucial difference.

> 2. "(mismatch)."  The default value for
>    `display-fill-column-indicator-character' is nil, but its type is
>    `character', so nil isn't allowed.

I fixed this, thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Mauro Aranda
In reply to this post by Philipp Stephani
Eli Zaretskii <[hidden email]> writes:

>> From: Philipp Stephani <[hidden email]>
>> Date: Sat, 09 May 2020 10:30:13 +0200
>>
>> The two issues here are:
>>
>> 1. "Group definition missing."  It looks like customizing this group
>>    should load `display-fill-column-indicator-mode', which defines this
>>    group, or the group definition should be in cus-start.el.
> The same seems to work for display-line-numbers, and I don't think I
> see the crucial difference.

I did some debugging and found that the difference is that there are no
custom options or faces defined in display-fill-column-indicator.el:
fill-column-indicator is in faces.el and
display-fill-column-indicator, display-fill-column-indicator-character
and display-fill-column-indicator-column are in xdisp.c

custom-make-dependencies doesn't go through faces.el, since it's
preloaded, so it doesn't record the custom-where property into
fill-column-indicator.
And since cus-dep.el doesn't require cus-start, fill-column-indicator is
the only member of the custom-group property of
display-fill-column-indicator, so there is no way
custom-make-dependencies will record the custom-where property into any
of the options defined in xdisp.c.

So later on, when looking for a custom-where property in the members of
the display-fill-column-indicator group, we find nothing, resulting in
no custom-loads thingy for display-fill-column-indicator.

The problem goes away if we require cus-start in cus-dep, or if we
record into the custom-loads the file where to find the custom-group.
That is, add in the first mapatoms call something like this:
(when (get symbol 'custom-where)
  (push (get symbol 'custom-where) found))

Or if eventually a defface or a defcustom makes its way into
display-fill-column-indicator.el, of course.

Note that adding the above form will result in adding cus-edit to the
custom-loads property of the groups defined in cus-edit (like editing,
convenience, etc.).  I don't know if that is harmless.
Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Eli Zaretskii
> From: Mauro Aranda <[hidden email]>
> Date: Fri, 28 Aug 2020 11:48:57 -0300
> Cc: Philipp Stephani <[hidden email]>, [hidden email]
>
> Eli Zaretskii <[hidden email]> writes:
>
> >> From: Philipp Stephani <[hidden email]>
> >> Date: Sat, 09 May 2020 10:30:13 +0200
> >>
> >> The two issues here are:
> >>
> >> 1. "Group definition missing."  It looks like customizing this group
> >>    should load `display-fill-column-indicator-mode', which defines this
> >>    group, or the group definition should be in cus-start.el.
> > The same seems to work for display-line-numbers, and I don't think I
> > see the crucial difference.
>
> I did some debugging and found that the difference is that there are no
> custom options or faces defined in display-fill-column-indicator.el:
> fill-column-indicator is in faces.el and
> display-fill-column-indicator, display-fill-column-indicator-character
> and display-fill-column-indicator-column are in xdisp.c
>
> custom-make-dependencies doesn't go through faces.el, since it's
> preloaded, so it doesn't record the custom-where property into
> fill-column-indicator.
> And since cus-dep.el doesn't require cus-start, fill-column-indicator is
> the only member of the custom-group property of
> display-fill-column-indicator, so there is no way
> custom-make-dependencies will record the custom-where property into any
> of the options defined in xdisp.c.
>
> So later on, when looking for a custom-where property in the members of
> the display-fill-column-indicator group, we find nothing, resulting in
> no custom-loads thingy for display-fill-column-indicator.
>
> The problem goes away if we require cus-start in cus-dep, or if we
> record into the custom-loads the file where to find the custom-group.
> That is, add in the first mapatoms call something like this:
> (when (get symbol 'custom-where)
>   (push (get symbol 'custom-where) found))
>
> Or if eventually a defface or a defcustom makes its way into
> display-fill-column-indicator.el, of course.
>
> Note that adding the above form will result in adding cus-edit to the
> custom-loads property of the groups defined in cus-edit (like editing,
> convenience, etc.).  I don't know if that is harmless.

Stefan, any suggestions regarding which solution route is preferable?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>> I did some debugging and found that the difference is that there are no
>> custom options or faces defined in display-fill-column-indicator.el:
>> fill-column-indicator is in faces.el and
>> display-fill-column-indicator, display-fill-column-indicator-character
>> and display-fill-column-indicator-column are in xdisp.c

Which begs the question: why define the group there?

>> Or if eventually a defface or a defcustom makes its way into
>> display-fill-column-indicator.el, of course.

Actually, there is a defcustom there (in the expansion of
`define-globalized-minor-mode`).

>> Note that adding the above form will result in adding cus-edit to the
>> custom-loads property of the groups defined in cus-edit (like editing,
>> convenience, etc.).  I don't know if that is harmless.
>
> Stefan, any suggestions regarding which solution route is preferable?

I'd get rid of the file and move its content to one of the
preloaded files (simple.el?  faces.el?  fill.el? ...)


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Mauro Aranda
Stefan Monnier <[hidden email]> writes:

>>> Or if eventually a defface or a defcustom makes its way into
>>> display-fill-column-indicator.el, of course.
>
> Actually, there is a defcustom there (in the expansion of
> `define-globalized-minor-mode`).

Sorry, I wasn't clear enough.  There is no defcustom or defface whose
:group is display-fill-column-indicator.
Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>>>> Or if eventually a defface or a defcustom makes its way into
>>>> display-fill-column-indicator.el, of course.
>>
>> Actually, there is a defcustom there (in the expansion of
>> `define-globalized-minor-mode`).
>
> Sorry, I wasn't clear enough.  There is no defcustom or defface whose
> :group is display-fill-column-indicator.

`global-display-fill-column-indicator-mode` should definitely belong to
the `display-fill-column-indicator` group (because the `defcustom`
doesn't have an explicit `:group` so it should fallback to using the
last-defined group).

BTW, another quick-fix might be to move the defface definition to
display-fill-column-indicator.el.


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Mauro Aranda
Stefan Monnier <[hidden email]> writes:

>>>>> Or if eventually a defface or a defcustom makes its way into
>>>>> display-fill-column-indicator.el, of course.
>>>
>>> Actually, there is a defcustom there (in the expansion of
>>> `define-globalized-minor-mode`).
>>
>> Sorry, I wasn't clear enough.  There is no defcustom or defface whose
>> :group is display-fill-column-indicator.
>
> `global-display-fill-column-indicator-mode` should definitely belong to
> the `display-fill-column-indicator` group (because the `defcustom`
> doesn't have an explicit `:group` so it should fallback to using the
> last-defined group).

Is it, though?
emacs -Q
M-x customize-option RET global-display-fill-column-indicator-mode
And near the end of buffer I read:
Groups: Global Display Fill Column Indicator

Furthermore, the list
(global-display-fill-column-indicator-mode custom-variable) is not a
member of the custom-group property of display-fill-column-indicator.

> BTW, another quick-fix might be to move the defface definition to
> display-fill-column-indicator.el.

Right.
Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Eli Zaretskii
In reply to this post by Stefan Monnier
> From: Stefan Monnier <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  Philipp Stephani <[hidden email]>,
>   [hidden email]
> Date: Sat, 29 Aug 2020 23:58:04 -0400
>
> >>>> Or if eventually a defface or a defcustom makes its way into
> >>>> display-fill-column-indicator.el, of course.
> >>
> >> Actually, there is a defcustom there (in the expansion of
> >> `define-globalized-minor-mode`).
> >
> > Sorry, I wasn't clear enough.  There is no defcustom or defface whose
> > :group is display-fill-column-indicator.
>
> `global-display-fill-column-indicator-mode` should definitely belong to
> the `display-fill-column-indicator` group (because the `defcustom`
> doesn't have an explicit `:group` so it should fallback to using the
> last-defined group).

So you are saying this should have worked as it is?

> BTW, another quick-fix might be to move the defface definition to
> display-fill-column-indicator.el.

The idea was not to require display-fill-column-indicator.el be loaded
to activate the feature.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
In reply to this post by Mauro Aranda
>> `global-display-fill-column-indicator-mode` should definitely belong to
>> the `display-fill-column-indicator` group (because the `defcustom`
>> doesn't have an explicit `:group` so it should fallback to using the
>> last-defined group).
>
> Is it, though?
> emacs -Q
> M-x customize-option RET global-display-fill-column-indicator-mode
> And near the end of buffer I read:
> Groups: Global Display Fill Column Indicator

Oh, indeed, it's an old misfeature that was introduced by our
idiot-in-chief (tho to my defense, I think it made some sense back then
because `defcustom` did not have a useful default for `:group`).

We can fix it either by adding `:group 'display-fill-column-indicator`
to the `define-globalized-minor-mode` or by getting rid of the
misfeature, as in the patch below (this patch will likely fix a few
other similar cases).

Eli?


        Stefan


diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 24c9e79f2c..e3eb9294ed 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -157,9 +157,6 @@ define-minor-mode
   the minor mode is global):
 
 :group GROUP Custom group name to use in all generated `defcustom' forms.
- Defaults to MODE without the possible trailing \"-mode\".
- Don't use this default group name unless you have written a
- `defgroup' to define that group properly.
 :global GLOBAL If non-nil specifies that the minor mode is not meant to be
  buffer-local, so don't make the variable MODE buffer-local.
  By default, the mode is buffer-local.
@@ -262,12 +259,6 @@ define-minor-mode
     (unless initialize
       (setq initialize '(:initialize 'custom-initialize-default)))
 
-    (unless group
-      ;; We might as well provide a best-guess default group.
-      (setq group
-    `(:group ',(intern (replace-regexp-in-string
- "-mode\\'" "" mode-name)))))
-
     ;; TODO? Mark booleans as safe if booleanp?  Eg abbrev-mode.
     (unless type (setq type '(:type 'boolean)))
 




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Eli Zaretskii
> From: Stefan Monnier <[hidden email]>
> Cc: Eli Zaretskii <[hidden email]>,  Philipp Stephani <[hidden email]>,
>   [hidden email]
> Date: Sun, 30 Aug 2020 10:51:23 -0400
>
> > emacs -Q
> > M-x customize-option RET global-display-fill-column-indicator-mode
> > And near the end of buffer I read:
> > Groups: Global Display Fill Column Indicator
>
> Oh, indeed, it's an old misfeature that was introduced by our
> idiot-in-chief (tho to my defense, I think it made some sense back then
> because `defcustom` did not have a useful default for `:group`).
>
> We can fix it either by adding `:group 'display-fill-column-indicator`
> to the `define-globalized-minor-mode` or by getting rid of the
> misfeature, as in the patch below (this patch will likely fix a few
> other similar cases).
>
> Eli?

How about doing the former on emacs-27, and the latter on master?

Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>> > emacs -Q
>> > M-x customize-option RET global-display-fill-column-indicator-mode
>> > And near the end of buffer I read:
>> > Groups: Global Display Fill Column Indicator
>>
>> Oh, indeed, it's an old misfeature that was introduced by our
>> idiot-in-chief (tho to my defense, I think it made some sense back then
>> because `defcustom` did not have a useful default for `:group`).
>>
>> We can fix it either by adding `:group 'display-fill-column-indicator`
>> to the `define-globalized-minor-mode` or by getting rid of the
>> misfeature, as in the patch below (this patch will likely fix a few
>> other similar cases).
>>
>> Eli?
>
> How about doing the former on emacs-27, and the latter on master?

Oh, indeed, I did not intend to change define-minor-mode on the
emacs-27 branch.
So, we have a deal!


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>> How about doing the former on emacs-27, and the latter on master?
> Oh, indeed, I did not intend to change define-minor-mode on the
> emacs-27 branch.
> So, we have a deal!

O, so I looked into fixing it on `emacs-27` and it's not quite as
straightforward as I thought.  Here's the patch that I have currently,
because without that `cus-dep.el` patch, the
`define-globalized-minor-mode` is just completely ignored by
cus-dep anyway.

I think it's quite safe, but I'd like to make sure you agree this is
acceptable for `emacs-27`.


        Stefan


diff --git a/lisp/cus-dep.el b/lisp/cus-dep.el
index fd307a5c04..e2fd7febd2 100644
--- a/lisp/cus-dep.el
+++ b/lisp/cus-dep.el
@@ -99,7 +99,7 @@ custom-make-dependencies
                   (setq name (intern name)))
               (condition-case nil
                   (while (re-search-forward
-                          "^(def\\(custom\\|face\\|group\\)" nil t)
+                          "^(def\\(custom\\|face\\|group\\|ine\\(?:-globalized\\)?-minor-mode\\)" nil t)
                     (beginning-of-line)
                     (let ((type (match-string 1))
   (expr (read (current-buffer))))
diff --git a/lisp/display-fill-column-indicator.el b/lisp/display-fill-column-indicator.el
index 3f947bdc1c..3391aa371b 100644
--- a/lisp/display-fill-column-indicator.el
+++ b/lisp/display-fill-column-indicator.el
@@ -73,7 +73,9 @@ display-fill-column-indicator--turn-on
 
 ;;;###autoload
 (define-globalized-minor-mode global-display-fill-column-indicator-mode
-  display-fill-column-indicator-mode display-fill-column-indicator--turn-on)
+  display-fill-column-indicator-mode display-fill-column-indicator--turn-on
+  ;; See bug#41145
+  :group 'display-fill-column-indicator)
 
 (provide 'display-fill-column-indicator)
 







Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Eli Zaretskii
> From: Stefan Monnier <[hidden email]>
> Cc: [hidden email],  [hidden email],  [hidden email]
> Date: Thu, 03 Sep 2020 22:24:14 -0400
>
> >> How about doing the former on emacs-27, and the latter on master?
> > Oh, indeed, I did not intend to change define-minor-mode on the
> > emacs-27 branch.
> > So, we have a deal!
>
> O, so I looked into fixing it on `emacs-27` and it's not quite as
> straightforward as I thought.  Here's the patch that I have currently,
> because without that `cus-dep.el` patch, the
> `define-globalized-minor-mode` is just completely ignored by
> cus-dep anyway.
>
> I think it's quite safe, but I'd like to make sure you agree this is
> acceptable for `emacs-27`.

Yes, LGTM.  Thanks.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>> I think it's quite safe, but I'd like to make sure you agree this is
>> acceptable for `emacs-27`.
> Yes, LGTM.  Thanks.

OK, pushed,


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Michael Heerdegen
In reply to this post by Stefan Monnier
Stefan Monnier <[hidden email]> writes:

> -    (unless group
> -      ;; We might as well provide a best-guess default group.
> -      (setq group
> -    `(:group ',(intern (replace-regexp-in-string
> - "-mode\\'" "" mode-name)))))
> -

After this change I have to specify a group for every define-minor-mode
in my init file, otherwise I'm flooded with warnings.  It's a bit
annoying.  I don't care about groups there since I don't use customize
myself at all.  What's the suggestion to handle this use case?

Michael.



Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Stefan Monnier
>> -    (unless group
>> -      ;; We might as well provide a best-guess default group.
>> -      (setq group
>> -    `(:group ',(intern (replace-regexp-in-string
>> - "-mode\\'" "" mode-name)))))
>
> After this change I have to specify a group for every define-minor-mode
> in my init file, otherwise I'm flooded with warnings.

Hmm... so you have `define-minor-mode`s in your init file?  That sounds
rather unusual [ And "flooded" suggests you have very many.  ]

> It's a bit annoying.  I don't care about groups there since I don't
> use customize myself at all.  What's the suggestion to handle this
> use case?

Add a dummy `defgroup` at the beginning of the file?


        Stefan




Reply | Threaded
Open this post in threaded view
|

bug#41145: 27.0.91; small issues with `display-fill-column-indicator' Customization group

Michael Heerdegen
Stefan Monnier <[hidden email]> writes:

> Hmm... so you have `define-minor-mode`s in your init file?  That
> sounds rather unusual [ And "flooded" suggests you have very many.  ]

Yes, 20.  Part of my private config, for my own usage.  I'm a bit
surprised that you think that's unusual.

> Add a dummy `defgroup` at the beginning of the file?

Ok, will give it a try.

Thanks,

Michael.