obsolete comment in tool-bar.el

obsolete comment in tool-bar.el

[Mathias Dahl]

I found this in tool-bar.el:

    ;; We want to pretend the toolbar by standard is on, as this will make
    ;; customize consider disabling the toolbar a customization, and save
    ;; that.  We could do this for real by setting :init-value above, but
    ;; that would turn on the toolbar in MS Windows where it is currently
    ;; useless, and it would overwrite disabling the tool bar from X
    ;; resources.  If anyone want to implement this in a cleaner way,
    ;; please do so.
    ;; -- Per Abrahamsen <abraham@dina.kvl.dk> 2002-02-21.
    (put 'tool-bar-mode 'standard-value '(t))

I don't know if the comment refers to the line of code just under it
or to more lines in the file or if it is old and does not refer to
anything :), but I am wondering about the sentence with "...in MS
Windows where it is currently useless". It is not useless at all now,
it is very much useful.

Shall the comment be removed and / or the code changed maybe?

/Mathias

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Mathias Dahl wrote:

   I found this in tool-bar.el:

       ;; We want to pretend the toolbar by standard is on, as this will make
       ;; customize consider disabling the toolbar a customization, and save
       ;; that.  We could do this for real by setting :init-value above, but
       ;; that would turn on the toolbar in MS Windows where it is currently
       ;; useless, and it would overwrite disabling the tool bar from X
       ;; resources.  If anyone want to implement this in a cleaner way,
       ;; please do so.
       ;; -- Per Abrahamsen <abraham@dina.kvl.dk> 2002-02-21.
       (put 'tool-bar-mode 'standard-value '(t))

   I don't know if the comment refers to the line of code just under it
   or to more lines in the file or if it is old and does not refer to
   anything :), but I am wondering about the sentence with "...in MS
   Windows where it is currently useless". It is not useless at all now,
   it is very much useful.

   Shall the comment be removed and / or the code changed maybe?

This is one of between 20 and 30 (I forgot the exact number) options
with a similar problem.  Start `emacs -Q', do `M-x customize-rogue'
and you get the list.  If you choose "Erase Customization" for them in
a Custom buffer, very unexpected things can happen.

The Custom buffer shows:

CHANGED outside Customize; operating on it here may be unreliable.

to warn you about that.

We decided that all should be fixed by putting the correct standard
value in the defcustom.  The trouble for most of them is that they are
preloaded and when the standard value is computed the variables that
are needed to compute the standard value are not yet defined.

Before fixing more, we need a decision on the best way to do fix them.
I explain the three alternatives below.

At first I solved this by using (if I remember everything correctly):

(defvar rogue-var) ;; to silence the compiler for the next line.
(unless (default-boundp rogue-var) (setq rogue-var nil))
(defcustom rogue-var ...
...
  :initialize 'custom-initialize-set
...)

This ensures that the defcustom is not evaluated when the file is
preloaded.  It then gets re-evaluated in a special way in startup.el.
This is the first alternative.

Stefan Monnier objected to what he perceived as "ugliness".

His solution, which is currently implemented for the two options we
fixed was to put in some boundp and fboundp's for all variables and
functions that are not yet defined.  This results in an essentially
arbitrary temporary value, which does not matter, because it is not
used until it gets reevaluated in startup.el.
This is the second alternative.

There some problems with Stefan's approach.  Stefan and I both
fixed one option this way.  We both made the mistake of assuming that
some variable or function "obviously" had to already be defined and
hence did not need the (f)boundp.  On certain operating systems,
people could not build CVS Emacs anymore.  Trivial to fix once
noticed, but one better hope somebody notices before the release
(although right now a release does not exactly appear to be imminent).

Things can be done completely safely by _always_ putting the value
computed by the defcustom inside a condition-case, whether one has the
impression it needs it or not, just for absolute safety.
This is the third alternative. I guess this will offend some people's
sense of esthetics, just as much as my original solution.

Both the (f)boundp and the condition-case solutions have the
disadvantage that somebody trying to customize an option correctly for
all environments by choosing "Show initial Lisp expression" in a
Custom buffer and trying to edit it, gets a version that is cluttered
with all kinds of confusing subtleties, which are not necessary in his
.emacs, because all variables and functions are defined there.

I still like my original solution best.

Even with my original solution, which simplifies the work somewhat,
scanning the Emacs source tree (_including_ the C source) for
everything that influences the standard value and then rewriting the
equivalent expression (often from C to Lisp) in the defcustom can
take, in certain cases, quite some work.  In some cases, it _might_
even require new primitives.  With two done and between twenty and
thirty to go, this does not look too good.

If one his not careful, people may add additional options to the rogue
list, making matters worse.  I had to prevent that in one case.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Mathias Dahl]

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> This is one of between 20 and 30 (I forgot the exact number) options
> with a similar problem.  Start `emacs -Q', do `M-x customize-rogue'
> and you get the list.  If you choose "Erase Customization" for them in
> a Custom buffer, very unexpected things can happen.

> [very long answer removed]

Thanks for the very long explanation of something I was not aware of
at all... :)

What about my "real" question, about the tool-bar being "useless" on
MS Windows?

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

   What about my "real" question, about the tool-bar being "useless" on
   MS Windows?

I do not use MS Windows, so I do not know.  If you use MS Windows and
you actually can use the tool-bar, that part of the remark _must_ be
obsolete I guess.  Maybe it still would turn on the tool-bar in other
situations where it is useless and certainly the remark about X
resources remains valid.  We could just erase the MS Windows part of
the remarks a temporary improvement, but the situation, as well as
the other similar ones, needs a real fix.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    (defvar rogue-var) ;; to silence the compiler for the next line.
    (unless (default-boundp rogue-var) (setq rogue-var nil))
    (defcustom rogue-var ...
    ...
      :initialize 'custom-initialize-set
    ...)

Suppose we implement a keyword in defcustom that causes it to generate
all that.  That will look nice in the source code, but at execution
time it will be equivalent to the above.

I think that would be clearly better than all three of the above
solutions.

    Even with my original solution, which simplifies the work somewhat,
    scanning the Emacs source tree (_including_ the C source) for
    everything that influences the standard value and then rewriting the
    equivalent expression (often from C to Lisp) in the defcustom can
    take, in certain cases, quite some work.

Doing this work is the only way to get the benefit, so let's do it.

RE: obsolete comment in tool-bar.el

[Drew Adams]

        (defvar rogue-var) ;; to silence the compiler for the next line.
        (unless (default-boundp rogue-var) (setq rogue-var nil))
        (defcustom rogue-var ...
        ...
          :initialize 'custom-initialize-set
        ...)

    Suppose we implement a keyword in defcustom that causes it to generate
    all that.  That will look nice in the source code, but at execution
    time it will be equivalent to the above.

    I think that would be clearly better than all three of the above
    solutions.

Sounds great to me.

Or what about just reusing keyword :initialize, perhaps redefining its
behavior to recognize this special case? After all, this is about
initializing the value. The case could be distinguished by supplying
:initialize with an argument `custom-initialize-set-runtime' (or some better
name).

Ignore that suggestion if it makes little sense - I'm no pro on defcustom.

Anyway, I like the idea of implementing Luc's "ugly" code via a defcustom
keyword. Perhaps someone will point out problems with it, but I can't think
of any offhand.

Question: How will users see/detect/understand this behavior? What will they
see in "Show initial Lisp expression"? I assume the keyword behavior would
be documented, but I'm wondering what users will see in Customize.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Drew Adams wrote:

   Or what about just reusing keyword :initialize, perhaps redefining its
   behavior to recognize this special case? After all, this is about
   initializing the value. The case could be distinguished by supplying
   :initialize with an argument `custom-initialize-set-runtime' (or some better
   name).

I believe that I will implement it using one or two new predefined
:initialize functions.  Once I have done that, I will send diffs and
if accepted I will document them in the Elisp manual.  I will not take
care of between 20 and 30 options, some of which I know nothing about,
all by myself, however.  Some of them are hooks or listvars and for
hooks and listvars, a proper solution requires more extensive changes,
as we discussed some time ago.  So we will not be able to fix those
right away.

   Question: How will users see/detect/understand this behavior? What will they
   see in "Show initial Lisp expression"?

The exact same thing they would see if they used any other :initialize
function.  The :initialize function should not and will not have any
effect on that.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

   Doing this work is the only way to get the benefit, so let's do it.

If the solution below looks OK I will document it in the Elisp manual.
Other people will need to help with this, since I do not fancy
handling all 20-30 options, some of which I know nothing about, all
by myself.

I actually used two new predefined :initialize keywords.  I thought it
might be better not to automatically set the option to nil if not
previously defined, since it could confuse somebody who changes the
defcustom, then unbinds the option and reloads the file to try his
revised code out.  So the new :initialize functions act normally,
except on error where they set the option to nil.  That is, I
automated the `condition-case' solution, rather than my own solution.
Unlike the original described condition-case solution, the automated
version does not unnecessarily clutter the expression the user sees
when selecting "Show initial Lisp expression".

I tested it out on two actual examples and it seems to work perfectly.
The patches below actually simplify the tooltip-defcustom
implementation by eliminating code duplication.  I can install if
desired.

===File ~/custom.el-diff====================================
*** custom.el	04 Jul 2005 19:45:29 -0500	1.87
--- custom.el	08 Jul 2005 20:49:11 -0500	
***************
*** 76,81 ****
--- 76,103 ----
  		 (eval (car (get symbol 'saved-value)))
  	       (eval value)))))
  
+ (defun custom-initialize-safe-set (symbol value)
+   "Like `custom-initialize-set', but catches errors.
+ If an error occurs during evaluation of VALUE, SYMBOL is set to nil
+ and no error is signaled.  This is meant for use in pre-loaded files
+ where some variables used to compute VALUE are not yet defined.
+ You can then re-evaluate VALUE in startup.el, for instance using
+ `custom-reevaluate-setting'."
+   (condition-case nil
+       (custom-initialize-set symbol value)
+     (error (set-default symbol nil))))
+ 
+ (defun custom-initialize-safe-default (symbol value)
+   "Like `custom-initialize-default', but catches errors.
+ If an error occurs during evaluation of VALUE, SYMBOL is set to nil
+ and no error is signaled.  This is meant for use in pre-loaded files
+ where some variables used to compute VALUE are not yet defined.
+ You can then re-evaluate VALUE in startup.el, for instance using
+ `custom-reevaluate-setting'."
+   (condition-case nil
+       (custom-initialize-default symbol value)
+     (error (set-default symbol nil))))
+ 
  (defun custom-initialize-reset (symbol value)
    "Initialize SYMBOL based on VALUE.
  Set the symbol, using its `:set' function (or `set-default' if it has none).
============================================================

===File ~/frame.el-diff=====================================
*** frame.el	04 Jul 2005 19:47:20 -0500	1.223
--- frame.el	08 Jul 2005 19:39:25 -0500	
***************
*** 1270,1278 ****
  displays through a window system, because then Emacs does its own
  cursor display.  On a text-only terminal, this is not implemented."
    :init-value (not (or noninteractive
! 		       (if (boundp 'no-blinking-cursor) no-blinking-cursor)
  		       (eq system-type 'ms-dos)
  		       (not (memq window-system '(x w32)))))
    :group 'cursor
    :global t
    (if blink-cursor-idle-timer (cancel-timer blink-cursor-idle-timer))
--- 1270,1279 ----
  displays through a window system, because then Emacs does its own
  cursor display.  On a text-only terminal, this is not implemented."
    :init-value (not (or noninteractive
! 		       no-blinking-cursor
  		       (eq system-type 'ms-dos)
  		       (not (memq window-system '(x w32)))))
+   :initialize 'custom-initialize-safe-default
    :group 'cursor
    :global t
    (if blink-cursor-idle-timer (cancel-timer blink-cursor-idle-timer))
============================================================

===File ~/tooltip.el-diff===================================
*** tooltip.el	04 Jul 2005 19:51:10 -0500	1.60
--- tooltip.el	08 Jul 2005 20:03:20 -0500	
***************
*** 162,171 ****
    ;; If you change the :init-value below, you also need to change the
    ;; corresponding code in startup.el.
    :init-value (not (or noninteractive
! 		       (and (boundp 'emacs-quick-startup) emacs-quick-startup)
  		       (not (and (fboundp 'display-graphic-p)
  				 (display-graphic-p)))
  		       (not (fboundp 'x-show-tip))))
    :group 'tooltip
    (unless (or (null tooltip-mode) (fboundp 'x-show-tip))
      (error "Sorry, tooltips are not yet available on this system"))
--- 162,172 ----
    ;; If you change the :init-value below, you also need to change the
    ;; corresponding code in startup.el.
    :init-value (not (or noninteractive
! 		       emacs-quick-startup
  		       (not (and (fboundp 'display-graphic-p)
  				 (display-graphic-p)))
  		       (not (fboundp 'x-show-tip))))
+   :initialize 'custom-initialize-safe-default
    :group 'tooltip
    (unless (or (null tooltip-mode) (fboundp 'x-show-tip))
      (error "Sorry, tooltips are not yet available on this system"))
============================================================

===File ~/startup.el-diff===================================
*** startup.el	04 Jul 2005 19:50:36 -0500	1.363
--- startup.el	08 Jul 2005 20:04:28 -0500	
***************
*** 752,766 ****
    ;; are not set.
    (custom-reevaluate-setting 'blink-cursor-mode)
    (custom-reevaluate-setting 'normal-erase-is-backspace)
! 
!   ;; If you change the code below, you need to also change the
!   ;; corresponding code in the tooltip-mode defcustom.  The two need
!   ;; to be equivalent under all conditions, or Custom will get confused.
!   (unless (or noninteractive
! 	      emacs-basic-display
!               (not (display-graphic-p))
!               (not (fboundp 'x-show-tip)))
!     (tooltip-mode 1))
  
    ;; Register default TTY colors for the case the terminal hasn't a
    ;; terminal init file.
--- 752,758 ----
    ;; are not set.
    (custom-reevaluate-setting 'blink-cursor-mode)
    (custom-reevaluate-setting 'normal-erase-is-backspace)
!   (custom-reevaluate-setting 'tooltip-mode)
  
    ;; Register default TTY colors for the case the terminal hasn't a
    ;; terminal init file.
============================================================

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

The following patch to tooltip.el simplifies the standard value of the
tooltip-mode defcustom further than my original version.

The remaining `(fboundp 'x-show-tip)' is really necessary, even after
startup.

===File ~/tooltip.el-diff===================================
*** tooltip.el	04 Jul 2005 19:51:10 -0500	1.60
--- tooltip.el	08 Jul 2005 21:39:53 -0500	
***************
*** 162,171 ****
    ;; If you change the :init-value below, you also need to change the
    ;; corresponding code in startup.el.
    :init-value (not (or noninteractive
! 		       (and (boundp 'emacs-quick-startup) emacs-quick-startup)
! 		       (not (and (fboundp 'display-graphic-p)
! 				 (display-graphic-p)))
  		       (not (fboundp 'x-show-tip))))
    :group 'tooltip
    (unless (or (null tooltip-mode) (fboundp 'x-show-tip))
      (error "Sorry, tooltips are not yet available on this system"))
--- 162,171 ----
    ;; If you change the :init-value below, you also need to change the
    ;; corresponding code in startup.el.
    :init-value (not (or noninteractive
! 		       emacs-quick-startup
! 		       (not (display-graphic-p))
  		       (not (fboundp 'x-show-tip))))
+   :initialize 'custom-initialize-safe-default
    :group 'tooltip
    (unless (or (null tooltip-mode) (fboundp 'x-show-tip))
      (error "Sorry, tooltips are not yet available on this system"))
============================================================

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    Question: How will users see/detect/understand this behavior? What will they
    see in "Show initial Lisp expression"?

They will see an expression that recomputes the standard value
based on whatever conditions are relevant.

My approach, like the one Luc's original one, avoids the need
to complicate this with fboundp's or condition-case, but someone
still needs to determine what all the conditions that affect the
proper standard value and write an expression that chooses
among them.

Re: obsolete comment in tool-bar.el

[Eli Zaretskii]

> Date: Fri, 8 Jul 2005 22:57:19 -0500 (CDT)
> From: Luc Teirlinck <teirllm@dms.auburn.edu>
> Cc: brakjoller@gmail.com, emacs-devel@gnu.org
> 
> The following patch to tooltip.el simplifies the standard value of the
> tooltip-mode defcustom further than my original version.

You are removing the test for display-graphic-p being fboundp, but the
ChangeLog entry for the change which introduced that test clearly says:

	* tooltip.el (tooltip-mode): `emacs-quick-startup' and
	`display-graphic-p' may not be bound yet.

I wasn't following this thread, so perhaps you suggested or installed
another change which takes care of that; if so, apologies.  If not, I
think we should understand the reason for the test and see if it's
still valid.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Eli Zaretskii wrote:

   You are removing the test for display-graphic-p being fboundp, but the
   ChangeLog entry for the change which introduced that test clearly says:

	   * tooltip.el (tooltip-mode): `emacs-quick-startup' and
	   `display-graphic-p' may not be bound yet.

   I wasn't following this thread, so perhaps you suggested or installed
   another change which takes care of that; if so, apologies.

The expression now gets evaluated inside a condition-case, and if an
error occurs, the value is set to nil, without throwing an error.  The
actual value at that stage is irrelevant, it is later reevaluated with
everything bound.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    If the solution below looks OK I will document it in the Elisp manual.

This is a clever solution.  Please install it.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

This concerns the documentation for the two new :initialize functions.
They are kind of internal and hence I believe that maybe they do not
need to be mentioned in the already very long NEWS.  On the other
hand, maybe it might be useful to mention them in the Elisp manual,
for instance because people experimenting with changing the defcustoms
in question need to be aware of their subtleties.

The patch below documents them.  The small unrelated change in the
patch appears necessary regardless.

===File ~/customize.texi-diff===============================
*** customize.texi	18 Jun 2005 08:44:38 -0500	1.45
--- customize.texi	10 Jul 2005 21:52:01 -0500	
***************
*** 270,275 ****
--- 270,290 ----
  Use the @code{:set} function to initialize the variable, if it is
  already set or has been customized; otherwise, just use
  @code{set-default}.
+ 
+ @item custom-initialize-safe-set
+ @itemx custom-initialize-safe-default
+ These functions behave like @code{custom-initialize-set}
+ (@code{custom-initialize-default}, respectively), but catch errors.
+ If an error occurs during initialization, they set the variable to
+ @code{nil} using @code{set-default}, and throw no error.
+ 
+ These two functions are only meant for options defined in pre-loaded
+ files, where some variables or functions used to compute the option's
+ value may not yet be defined.  The option normally gets updated in
+ @file{startup.el}, ignoring the previously computed value.  Because of
+ this typical usage, the value which these two functions compute
+ normally only matters when, after startup, one unsets the option's
+ value and then reevaluates the defcustom.
  @end table
  
  @item :set-after @var{variables}
***************
*** 318,325 ****
  Internally, @code{defcustom} uses the symbol property
  @code{standard-value} to record the expression for the default value,
  and @code{saved-value} to record the value saved by the user with the
! customization buffer.  The @code{saved-value} property is actually a
! list whose car is an expression which evaluates to the value.
  
  @node Customization Types
  @section Customization Types
--- 333,340 ----
  Internally, @code{defcustom} uses the symbol property
  @code{standard-value} to record the expression for the default value,
  and @code{saved-value} to record the value saved by the user with the
! customization buffer.  Both properties are actually lists whose car is
! an expression which evaluates to the value.
  
  @node Customization Types
  @section Customization Types
============================================================

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

This is good, but

    + These two functions are only meant for options defined in pre-loaded
    + files, where some variables or functions used to compute the option's
    + value may not yet be defined.  The option normally gets updated in
    + @file{startup.el}, ignoring the previously computed value.  Because of
    + this typical usage, the value which these two functions compute
    + normally only matters when, after startup, one unsets the option's
    + value and then reevaluates the defcustom.

To make that really clear, I think you should add:

  By that time, the necessary variables and functions will be defined,
  so there will not be an error.

Re: obsolete comment in tool-bar.el

[David Kastrup]

"Richard M. Stallman" <rms@gnu.org> writes:

> This is good, but
>
>     + These two functions are only meant for options defined in pre-loaded
>     + files, where some variables or functions used to compute the option's
>     + value may not yet be defined.  The option normally gets updated in
>     + @file{startup.el}, ignoring the previously computed value.  Because of
>     + this typical usage, the value which these two functions compute
>     + normally only matters when, after startup, one unsets the option's
>     + value and then reevaluates the defcustom.
>
> To make that really clear, I think you should add:
>
>   By that time, the necessary variables and functions will be defined,
>   so there will not be an error.

Stupid question: instead of having two initialization functions that
need to replace the normal initializers for preloaded stuff, wouldn't
it be easier to simply redefine the _standard_ initialization
functions in an Emacs used for dumping?

That way, one would not need to have to adapt custom definitions that
are going to be dumped.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

David Kastrup wrote:

   Stupid question: instead of having two initialization functions that
   need to replace the normal initializers for preloaded stuff, wouldn't
   it be easier to simply redefine the _standard_ initialization
   functions in an Emacs used for dumping?

That would disable error detection for all preloaded defcustoms, the
vast majority of which do not need to be re-initialized in startup.el.
My two functions do not really disable error detection at startup for
those defcustoms for which they are used, since they get reevaluated
in startup.el with error detection enabled.

Somebody who unsets an option that uses one of the two functions,
changes the definition (but not the :initialize function) and reloads
the file will have to realize that error detection is disabled, but
this is a rather exceptional situation.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    Stupid question: instead of having two initialization functions that
    need to replace the normal initializers for preloaded stuff, wouldn't
    it be easier to simply redefine the _standard_ initialization
    functions in an Emacs used for dumping?

I don't think we want to disguise unexpected errors.
However, an idea occurs to me.  We could change the two
usual functions so that

    if an error occurs
    during building Emacs
    then the variable is left unbound.

If this happens where it is expected, for the variables which would
have used the new -safe- functions, the variable will be recomputed
later in startup.el.  So the end result will be the same as if a
-safe- function had been used.

But if it happens where it is not expected, the variable will
remain unbound, and references will get an error, and we will
debug it.

I am not sure that method would work well, but maybe it would.
However, sticking with the present approach is safer.
We know it can't ignore errors for variables we did not intend
that for.

Re: obsolete comment in tool-bar.el

[YAMAMOTO Mitsuharu]

>>>>> On Sat, 9 Jul 2005 08:57:34 -0500 (CDT), Luc Teirlinck <teirllm@dms.auburn.edu> said:

> The expression now gets evaluated inside a condition-case, and if an
> error occurs, the value is set to nil, without throwing an error.
> The actual value at that stage is irrelevant, it is later
> reevaluated with everything bound.

I tried the latest CVS with Mac OS 9, which cannot dump, but it failed
to load frame.elc with "Symbol's value as variable is void:
no-blinking-cursor" on startup.  Strangely, the rest of the similar
cases (such as loading tooltip.elc) worked well.

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: obsolete comment in tool-bar.el

[YAMAMOTO Mitsuharu]

>>>>> On Tue, 12 Jul 2005 13:13:35 +0900, YAMAMOTO Mitsuharu <mituharu@math.s.chiba-u.ac.jp> said:

> I tried the latest CVS with Mac OS 9, which cannot dump, but it
> failed to load frame.elc with "Symbol's value as variable is void:
> no-blinking-cursor" on startup.  Strangely, the rest of the similar
> cases (such as loading tooltip.elc) worked well.

Correction: after "make bootstrap", Emacs on Mac OS 9 failed to load
loaddefs.el with "Symbol's value as variable is void:
emacs-quick-startup".  In loaddefs.el, we have the following line:

(defvar tooltip-mode (not (or noninteractive emacs-quick-startup (not (display-graphic-p)) (not (fboundp (quote x-show-tip))))) "\

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

I forgot that the argument to defcustom gets evaluated outside the
:initialize function.  I will implement another solution.  Also,
tooltip-mode can not use custom-reevaluate-setting, because it would
load tooltip on systems where we decided it should not be loaded.  So
I will undo my change to startup.el and put in a comment explaining
why all the stuff I replaced really is necessary.  The autoload for
tooltip-mode probably should be removed now that it is preloaded on
systems on which it makes sense.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

I reverted most of my previous changes.  Emacs should now build
correctly on all operating systems (unless it fails to do so for
unrelated reasons), but I can not check this, as I only have access to
GNU/Linux.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

>From my previous message:

   I forgot that the argument to defcustom gets evaluated outside the
   :initialize function.  I will implement another solution.

Actually I just reverted most of my changes, until we decide on what
to do with the problem.  The two new :initialize functions probably
should be removed, but since they do no harm, I kept them in
custom.el, until we make a final decision.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

YAMAMOTO Mitsuharu wrote:

   > I tried the latest CVS with Mac OS 9, which cannot dump, but it
   > failed to load frame.elc with "Symbol's value as variable is void:
   > no-blinking-cursor" on startup.  Strangely, the rest of the similar
   > cases (such as loading tooltip.elc) worked well.

   Correction: after "make bootstrap", Emacs on Mac OS 9 failed to load
   loaddefs.el with "Symbol's value as variable is void:
   emacs-quick-startup".  In loaddefs.el, we have the following line:

   (defvar tooltip-mode (not (or noninteractive emacs-quick-startup (not (display-graphic-p)) (not (fboundp (quote x-show-tip))))) "\

I believe that my original reaction to this was incorrect.  It was
based on checking defcustoms with C-M-x.  That was stupid, because
evaluating a defcustom with C-M-x behaves specially, partially
bypassing the :initialize function.

After repeating my experiments in ielm, I now believe that my two new
:initialize functions should work as intended.  The problem you
experienced with `blink-cursor-mode' seems to be due to a bug in
define-minor-mode, whereas the problem you experienced with
tooltip-mode is obviously due to autoloading, which could be removed,
since tooltip.el is preloaded in all situations where enabling tooltip
makes sense.

It would be possible to reinstate most of my original changes if the
bug in define-minor-mode would be fixed.  I will take a look at that.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Richard Stallman wrote:

   I am not sure that method would work well, but maybe it would.
   However, sticking with the present approach is safer.
   We know it can't ignore errors for variables we did not intend
   that for.

I agree.  The alternative might, at the very least, require compiler
defvars at many places.

I propose to remove the useless autoload for tooltip-mode, which fixes
one of the reported bugs, to apply the patch below to easy-mmode.el,
which fixes the other reported bug and to reinstate
custom-initialize-safe-default as the :initialize function for
tooltip-mode and blink-cursor-mode, as well as to keep recommending
the two new :initialize functions for use in similar cases.  I could
install all of that, if desired.

Unfortunately, I can not reinstate the use of
`custom-reevaluate-setting' for tooltip-mode in startup.el.  Unless we
would be willing to load tooltip.el even on systems where it makes no
sense, we just have to live with the code duplication here.  (That
would also apply to other proposed solutions.)

Here is the required patch to easy-mmode.el.  Stefan opposed a similar
patch earlier, but the objections he then stated (that loading a file
would have side effects) do not apply to custom-initialize-safe-default.

===File ~/easy-mmode-diff===================================
*** easy-mmode.el	04 Jul 2005 12:44:35 -0500	1.67
--- easy-mmode.el	12 Jul 2005 21:14:49 -0500	
***************
*** 142,147 ****
--- 142,148 ----
    (let* ((mode-name (symbol-name mode))
  	 (pretty-name (easy-mmode-pretty-mode-name mode lighter))
  	 (globalp nil)
+ 	 (initialize nil)
  	 (group nil)
  	 (extra-args nil)
  	 (extra-keywords nil)
***************
*** 159,164 ****
--- 160,166 ----
  	(:lighter (setq lighter (pop body)))
  	(:global (setq globalp (pop body)))
  	(:extra-args (setq extra-args (pop body)))
+ 	(:initialize (setq initialize (list :initialize (pop body))))
  	(:group (setq group (nconc group (list :group (pop body)))))
  	(:require (setq require (pop body)))
  	(:keymap (setq keymap (pop body)))
***************
*** 167,172 ****
--- 169,178 ----
      (setq keymap-sym (if (and keymap (symbolp keymap)) keymap
  		       (intern (concat mode-name "-map"))))
  
+     (unless initialize
+       (setq initialize
+ 	    '(:initialize 'custom-initialize-default)))
+ 
      (unless group
        ;; We might as well provide a best-guess default group.
        (setq group
***************
*** 196,202 ****
  	    `(defcustom ,mode ,init-value
  	       ,(format base-doc-string pretty-name mode mode)
  	       :set 'custom-set-minor-mode
! 	       :initialize 'custom-initialize-default
  	       ,@group
  	       :type 'boolean
  	       ,@(cond
--- 202,208 ----
  	    `(defcustom ,mode ,init-value
  	       ,(format base-doc-string pretty-name mode mode)
  	       :set 'custom-set-minor-mode
! 	       ,@initialize
  	       ,@group
  	       :type 'boolean
  	       ,@(cond
============================================================

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

I forgot to mention that the reported bug concerning
`blink-cursor-mode' was due to define-minor-mode overriding the
":initialize 'custom-initialize-safe-default" with
`custom-initialize-default'.  The patch I sent prevents
define-minor-mode from overriding an explicitly specified
`:initialize' keyword.  Stefan earlier opposed that, arguing that
`custom-initialize-default' was the _only_ acceptable :initialize
function for global minor modes and hence user overrides should be ignored.
Even assuming this would have been true previously, it certainly is no
longer true, since custom-initialize-safe-default was defined.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

       I forgot that the argument to defcustom gets evaluated outside the
       :initialize function.  I will implement another solution.

I don't think so.  Here's the body of `defcustom':

  (nconc (list 'custom-declare-variable
	       (list 'quote symbol)
	       (list 'quote value)
	       doc)
	 args))

And here's what custom-initialize-set does:

  (unless (default-boundp symbol)
    (funcall (or (get symbol 'custom-set) 'set-default)
	     symbol
	     (if (get symbol 'saved-value)
		 (eval (car (get symbol 'saved-value)))
	       (eval value)))))


I think your code was right, so please reinstall it.


What led you to think this was wrong?

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    Unfortunately, I can not reinstate the use of
    `custom-reevaluate-setting' for tooltip-mode in startup.el.  Unless we
    would be willing to load tooltip.el even on systems where it makes no
    sense, we just have to live with the code duplication here.

How do you reach this conclusion?  I do not understand.

    Here is the required patch to easy-mmode.el.  Stefan opposed a similar
    patch earlier, but the objections he then stated (that loading a file
    would have side effects) do not apply to custom-initialize-safe-default.

It seems ok to me, but the doc string should mention :initialize
and explain that the only legitimate value to use with it is
custom-initialize-safe-default, and when that should be used.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

I reinstalled my original changes, deleted the autoload for tooltip
mode and committed my changes to define-minor-mode.  The two last
steps should enable Emacs to be built on Mac OS 9, even with my
changes, although I can not test that.

       Unfortunately, I can not reinstate the use of
       `custom-reevaluate-setting' for tooltip-mode in startup.el.  Unless we
       would be willing to load tooltip.el even on systems where it makes no
       sense, we just have to live with the code duplication here.

   How do you reach this conclusion?  I do not understand.

To allow Emacs to be built on certain operating systems with my
reinstalled changes, I had to remove the autoload for tooltip-mode.
But that means that `custom-reevaluate-settings' in certain situations
(when tooltip is not preloaded, that is, when x-show-tip is not
fboundp) "reevaluates" the defcustom before the defcustom has been
executed.  In the case of tooltip-mode, that actually turns out to be
OK, because if the defcustom has not yet been executed,
`custom-reevaluate-settings' sets the variable to nil, which in the
case of tooltip mode happens to be the correct value in all situations
where tooltip is not preloaded.

So I readded the `custom-reevaluate-settings' for tooltip-mode.  It
kind of works by accident rather than by design however, and may not
work in other similar cases.

       Here is the required patch to easy-mmode.el.  Stefan opposed a similar
       patch earlier, but the objections he then stated (that loading a file
       would have side effects) do not apply to custom-initialize-safe-default.

   It seems ok to me, but the doc string should mention :initialize
   and explain that the only legitimate value to use with it is
   custom-initialize-safe-default, and when that should be used.

I committed the non-doc changes to easy-mmode.el.  Here are the
proposed doc changes which I could install if desired:

===File ~/easy-mmode-diff===================================
*** easy-mmode.el	12 Jul 2005 21:14:49 -0500	1.68
--- easy-mmode.el	13 Jul 2005 20:53:55 -0500	
***************
*** 105,112 ****
    It will be executed after any toggling but before running the hook variable
    `mode-HOOK'.
    Before the actual body code, you can write keyword arguments (alternating
!   keywords and values).  These following keyword arguments are supported (other
!   keywords will be passed to `defcustom' if 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
--- 105,111 ----
    It will be executed after any toggling but before running the hook variable
    `mode-HOOK'.
    Before the actual body code, you can write keyword arguments (alternating
!   keywords and values).  The following keyword arguments are supported:
  :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
***************
*** 122,128 ****
  For example, you could write
    (define-minor-mode foo-mode \"If enabled, foo on you!\"
      :lighter \" Foo\" :require 'foo :global t :group 'hassle :version \"27.5\"
!     ...BODY CODE...)"
    (declare (debug (&define name stringp
  			   [&optional [&not keywordp] sexp
  			    &optional [&not keywordp] sexp
--- 121,136 ----
  For example, you could write
    (define-minor-mode foo-mode \"If enabled, foo on you!\"
      :lighter \" Foo\" :require 'foo :global t :group 'hassle :version \"27.5\"
!     ...BODY CODE...)
! 
! If the minor mode is global other keywords are passed to `defcustom'.
! However, the value of the :set keyword is hard coded to
! `custom-set-minor-mode' and the value of the :type keyword is hard coded
! to `boolean'.  The value of the :initialize keyword defaults to
! `custom-initialize-default'.  This default can be overridden using
! an explicit :initialize keyword.  However, the only known situation
! in which such an override is acceptable is for some preloaded global
! minor modes which need to use `custom-initialize-safe-default'."
    (declare (debug (&define name stringp
  			   [&optional [&not keywordp] sexp
  			    &optional [&not keywordp] sexp
============================================================

Re: obsolete comment in tool-bar.el

[YAMAMOTO Mitsuharu]

>>>>> On Wed, 13 Jul 2005 21:08:55 -0500 (CDT), Luc Teirlinck <teirllm@dms.auburn.edu> said:

> I reinstalled my original changes, deleted the autoload for tooltip
> mode and committed my changes to define-minor-mode.  The two last
> steps should enable Emacs to be built on Mac OS 9, even with my
> changes, although I can not test that.

I tried the latest one on Mac OS 9, but I got the same error about
no-blinking-cursor.  I could reproduce it also on Solaris with "temacs
-l loadup".

In frame.elc, we have

  (custom-declare-variable 'blink-cursor-mode ...)
  (defalias 'blink-cursor-mode ...)
  (byte-code ...)

and the final one seems to be a culprit, which is disassembled to:

byte code:
  args: nil
0	constant  add-minor-mode
1	constant  blink-cursor-mode
2	constant  nil
3	constant  boundp
4	constant  blink-cursor-mode-map
5	call	  1
6	goto-if-nil-else-pop 1
9	constant  blink-cursor-mode-map
10	symbol-value 
11:1	call	  3
12	discard	  
13	varref	  load-file-name
14	goto-if-nil 3
17	varref	  noninteractive
18	goto-if-not-nil-else-pop 2
21	varref	  no-blinking-cursor
(snip)

				     YAMAMOTO Mitsuharu
				mituharu@math.s.chiba-u.ac.jp

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

The culprit is this code in define-minor-mode.  That code is strange,
since it does exactly what custom-initialize-default tries to avoid:

;; If the mode is global, call the function according to the default.
       ,(if globalp
           `(if (and load-file-name (not (equal ,init-value ,mode)))
		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

I have the impression that the offending code in define-minor-mode:

;; If the mode is global, call the function according to the default.
       ,(if globalp
           `(if (and load-file-name (not (equal ,init-value ,mode)))
		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))

combined with custom-initialize-default, is an ugly kludge to get around
a bug people sometimes make where they use functions or variables in a
:set function that are defined in the same file, but _after_ the defcustom.
That of course does not work and the obvious solution is to define
those functions and variables before the defcustom.

But I believe that define-minor-mode tries to hide this type of user
bug and work around it by delaying the enabling of the minor mode
function until after the file is loaded.

If this is the correct interpretation of the code, then that was a
silly thing to do, but now things may have come to rely on it.

Until we decide what to really do with this, I believe that the
following patch should at least allow Emacs CVS to be built on all
operating systems (but I can not test).  Rather than applying this
patch, I would rather get rid of the `eval-after-load' kludge
altogether, but if the patch works, it at least shows that we have
isolated the only real problem.

===File ~/easy-mmode-diff===================================
*** easy-mmode.el	14 Jul 2005 12:45:27 -0500	1.68
--- easy-mmode.el	14 Jul 2005 12:51:32 -0500	
***************
*** 264,271 ****
  
         ;; If the mode is global, call the function according to the default.
         ,(if globalp
! 	    `(if (and load-file-name (not (equal ,init-value ,mode)))
! 		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))
  \f
  ;;;
  ;;; make global minor mode
--- 264,274 ----
  
         ;; If the mode is global, call the function according to the default.
         ,(if globalp
! 	    `(and load-file-name
! 		  (eq (quote ,initialize)
! 		      (quote (:initialize 'custom-initialize-default)))
! 		  (not (equal ,init-value ,mode))
! 		  (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))
  \f
  ;;;
  ;;; make global minor mode
============================================================

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Richard Stallman wrote:

   Please install the doc string changes.

Actually, after rechecking things today, it appears that my changes to
the define-minor-mode code concerning the :initialize keyword were not
necessary.  I tested things yesterday and that testing seemed to show
that the changes were necessary, but I must have done something wrong
while testing yesterday.

This also means that the :set and :type keywords are _not_ hardwired and
can be overridden by specifying explicit :set and :type keywords.  Thus,
after rechecking, my proposed new docstring appears to be inaccurate.

Since my code changes actually resulted in no changes in behavior,
they would seem to require no doc changes at all.

There is the question of whether my changes should be reverted.  They
do not result in any change in behavior whatsoever, but they avoid
confusion for somebody who expands the macro to see what the
define-minor-mode form he wrote really does (as anybody who uses
define-minor-mode should do).  So it seems that my changes actually
were positive, even though not really necessary.  It is a little bit
inconsistent to do this for :initialize and not for :set and :type,
but since :initialize will be used more often, it might make sense.

So I believe it probably is better not to revert my changes to the
define-minor-mode code.  But since they do not produce a change in
behavior, they do not seem to call for a doc change.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

>From my previous message:

   There is the question of whether my changes should be reverted.  They
   do not result in any change in behavior whatsoever, but they avoid
   confusion for somebody who expands the macro to see what the
   define-minor-mode form he wrote really does (as anybody who uses
   define-minor-mode should do).  So it seems that my changes actually
   were positive, even though not really necessary.  It is a little bit
   inconsistent to do this for :initialize and not for :set and :type,
   but since :initialize will be used more often, it might make sense.

More precisely, without my changes, if you specify an explicit
:initialize keyword, the expansion of the define-minor-mode form
contains two :initialize keywords, first
":initialize 'custom-initialize-default" and then, at some later place
the explicit :initialize.  The last one, the explicit :initialize
wins, but since the two may be separated by other code, it might
confuse the person reading the expanded version of the macro.  With my
changes (that is with current CVS), the defcustom has only one
:initialize form, the explicitly specified one.  However, explicitly
specified :set or :type keywords still expand to defcustoms with two
:set or :type keywords.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Stefan Monnier]

> I have the impression that the offending code in define-minor-mode:
> ;; If the mode is global, call the function according to the default.
>        ,(if globalp
>            `(if (and load-file-name (not (equal ,init-value ,mode)))
> 		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))

> combined with custom-initialize-default, is an ugly kludge to get around
> a bug people sometimes make where they use functions or variables in a
> :set function that are defined in the same file, but _after_ the defcustom.

AFAIK, the motivation was also for some other cases where the user may
somehow setq the variable before loading the file.  IIRC this setq may
actually be done by Custom (at a time where it doesn't yet know that the var
have a :setter).

> That of course does not work and the obvious solution is to define
> those functions and variables before the defcustom.

But this is non-obvious, so it's ugly.  Also often those functions use the
variable, so to shut up the byte-compiler you need to pre-declare the
variable.  Miles complained loudly (in the form of comments, some of which
may still be in the current elisp code ;-) before I installed this
delayed-initialization trick.

> But I believe that define-minor-mode tries to hide this type of user
> bug and work around it by delaying the enabling of the minor mode
> function until after the file is loaded.

Which is the right thing to do.

> ! 	    `(and load-file-name
> ! 		  (eq (quote ,initialize)
> ! 		      (quote (:initialize 'custom-initialize-default)))
> ! 		  (not (equal ,init-value ,mode))
> ! 		  (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))

IIUC the (eq (quote ,initialize)
> ! 		      (quote (:initialize 'custom-initialize-default)))

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Stefan Monnier wrote:

   AFAIK, the motivation was also for some other cases where the user may
   somehow setq the variable before loading the file.  IIRC this setq may
   actually be done by Custom (at a time where it doesn't yet know that the var
   have a :setter).

The user should not set a minor mode variable directly unless setting
the variable is all that is needed to enable the mode, in which case
there is no need to call the function if the variable is already set.
The same applies to Custom.  Moreover, if it is done before loading
the file, it is done before the define-minor-mode.

   But this is non-obvious, so it's ugly.

In Elisp, if you call a function before it is defined, you get an
error.  If you reference a variable before it is defined, you get a
compiler warning, unless you use a compiler defvar.  That is true in
particular for :set functions if the defcustom is not produced by
define-minor-mode.  Why should defcustoms defined by define-minor-mode
be the lone exception?  I do not believe that having to define
functions and variables before using them is that horrible, but if it
is, then should not changes in the way files are loaded or compiled be
implemented, instead of this define-minor-mode only kludge?

   Also often those functions use the
   variable, so to shut up the byte-compiler you need to pre-declare the
   variable.

That is what compiler defvars are for.  Compiler defvars do not have
the confusing negative side effects that eval-after-load has.

   Miles complained loudly (in the form of comments, some of which
   may still be in the current elisp code ;-)

These comments make no sense.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

    More precisely, without my changes, if you specify an explicit
    :initialize keyword, the expansion of the define-minor-mode form
    contains two :initialize keywords, first
    ":initialize 'custom-initialize-default" and then, at some later place
    the explicit :initialize.

That's not clean, and it might not be clear which one would "win".
So it is better to keep your code, and pass just one :initialize.

      However, explicitly
    specified :set or :type keywords still expand to defcustoms with two
    :set or :type keywords.

It would be cleaner to do the same thing for :set and :type--to suppress
the default one when there is an explicit one.

Re: obsolete comment in tool-bar.el

[Stefan Monnier]

>    AFAIK, the motivation was also for some other cases where the user may
>    somehow setq the variable before loading the file.  IIRC this setq may
>    actually be done by Custom (at a time where it doesn't yet know that
>    the var have a :setter).

> The user should not set a minor mode variable directly unless setting
> the variable is all that is needed to enable the mode, in which case
> there is no need to call the function if the variable is already set.
> The same applies to Custom.

Can you prevent users and Custom from doing it?  If not, the "should not"
consideration is fairly meaningless.

> Moreover, if it is done before loading the file, it is done before the
> define-minor-mode.

Of course.  I'm not sure why you say it, tho.  I must be
misunderstanding something.

> In Elisp, if you call a function before it is defined, you get an
> error.  If you reference a variable before it is defined, you get a
> compiler warning, unless you use a compiler defvar.  That is true in
> particular for :set functions if the defcustom is not produced by
> define-minor-mode.  Why should defcustoms defined by define-minor-mode
> be the lone exception?  I do not believe that having to define
> functions and variables before using them is that horrible, but if it
> is, then should not changes in the way files are loaded or compiled be
> implemented, instead of this define-minor-mode only kludge?

In 99% of the cases, a minor mode's main body is only executed after the
whole file was loaded, so programmers do not expect it to be executed while
the file is being loaded.  This is empirically true.  The eval-after-load
trick turns the 99% into 100%, thus removing corner case bugs.

>    Miles complained loudly (in the form of comments, some of which
>    may still be in the current elisp code ;-)
> These comments make no sense.

If you do not understand them, I fear you may not understand the problem
well enough to judge what's the least bad solution.


        Stefan

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Stefan Monnier wrote:

   >    Miles complained loudly (in the form of comments, some of which
   >    may still be in the current elisp code ;-)
   > These comments make no sense.

   If you do not understand them, I fear you may not understand the problem
   well enough to judge what's the least bad solution.

Well I should not have said "these comments make no sense", as I may
not have found all of them.  It would also have been more accurate to
say "your solution to these comments makes no sense".  What I found
were two instances of the following comment:

;;; Note this definition must be at the end of the file, because
;;; `define-minor-mode' actually calls the mode-function if the
;;; associated variable is non-nil, which requires that all needed
;;; functions be already defined.  [This is arguably a bug in d-m-m]

You can solve the only problem pointed out in this comment in two
ways.  The ideal one is to come up with a solution that does not
require define-minor-mode to actually call the mode function.  The
second is to put the call to the define-minor-mode after all functions
used by the minor mode, for instance at the end of the file.  That may
require a compiler defvar, but that is not the end of the world.

There are several things wrong with your solution.  The call to the
minor mode becomes de facto part of the initialization of the minor
mode defcustom.  Therefore the ":initialize 'custom-initialize-default"
is misleading.  The motivation given for that :initialize function is
that loading the file should not call the mode function.  But the file
calls the minor mode function anyway (via an eval-after-load), in a
way that misleads people into believing it will not be.

The second problem is much worse.  In `(elisp)Hooks for Loading' it is
stated that well-designed Lisp programs should not use
eval-after-load.  There is a very good reason for that.  The kind of
use of eval-after-load made by define-minor-mode means that
define-minor-mode behaves differently when called interactively than
when called from a file.  This destroys the interactive nature of
Elisp.  For instance, the eval-after-load makes the interactive
testing (for instance in ielm) of any Elisp code that directly or
indirectly calls define-minor-mode unreliable.  You can only reliably
test such code by loading files.  (As I noticed.)

The two above problems seem to me to be worse than the problems caused
by a compiler defvar and a call to define-minor-mode late in the file.
If it really is impossible to get rid of the call to the mode function
(something I am not convinced of), then I believe that you should get
rid of the eval-after-load and clearly state in the define-minor-mode
docstring that it can call the mode function and state what the
consequences of that are.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

   Can you prevent users and Custom from doing it?  If not, the "should not"
   consideration is fairly meaningless.

No code can prevent users from making mistakes.  No code can reliably
correct user mistakes and I do not believe that one should try to do
that.  Just alert the user that there may be a problem, but do not
blindly try to correct.  We _can_ correct bugs in defcustoms included
with the Emacs distribution.

   > Moreover, if it is done before loading the file, it is done before the
   > define-minor-mode.

   Of course.  I'm not sure why you say it, tho.  I must be
   misunderstanding something.

Well there are two problems.  The first is : does `define-minor-mode'
really need to call the mode function?  My remark you quoted is
irrelevant to this question.

The second question is, _if_ define-minor-mode needs to call the mode
function, should the minor mode be called through eval-after-load or
should the call to define-minor-mode be postponed until all necessary
functions are defined.  This problem is what the remark you quoted
applies to.

I do not immmediately know the answer to the first question, although
I am definitely not sure that the answer is yes.  The motivation is
nowhere pointed out clearly.  If the only motivation is to autocorrect
user mistakes or bugs in Custom, then I believe that the answer is
definitely no.  If the motivation is a non-nil standard value (which
is relatively rare), you can either take care of this in the
:initialize function and document that define-minor-mode should not be
called too soon or let the programmer call the mode function near the
end of the file and explain how to do that correctly in the docstring.
There is no need to automate everything.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Here is the code:

       ;; If the mode is global, call the function according to the default.
       ,(if globalp
           `(if (and load-file-name (not (equal ,init-value ,mode)))
		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))

I believe that this code definitely should be removed.

What does this code do?  Nothing, if the current value of the minor
mode variable is equal to the standard value.  That is, it does _not_
enable the mode if the standard value is t, even though the defcustom
sets the minor mode variable to t, thereby introducing a bug, which
the programmer using `define-minor-mode' will have to hand correct.

It only does something (other than throwing unnecessary errors) if the
define-minor-mode is loaded in a file and the minor mode variable has
a value different from the default _and_ out of sync with the actual
situation, something that can only occur if there is a bug in Emacs or
a user mistake.

Now what is this code _trying_ to do?  To "call the function according
to the default", if we are to believe the comment.  That would mean
that we should call the minor mode function if ,init-value is t and
the minor mode variable was unbound before the defcustom got executed.

So, to do what the comment says, we should replace the default
:initialize for minor modes from 'custom-initialize-default to
'custom-initialize-set.  Then the only effect that loading the file
would have would be to establish the intended default if the user did
not override that default before the file got loaded.  (Note: do not
confuse custom-initialize-set with the very disruptive default
:initialize function custom-initialize-reset.)  This would avoid both
problems I pointed out with the present approach.  It would still mean
that define-minor-mode should not be called too soon, but only if the
minor mode is to be enabled by default, which is rare for a global
minor mode.

Maybe, in spite of the comment, the code is _really_ trying to do what
it actually does: try to bring the mode variable back in sync with the
actual situation if it has gotten out of sync due to some bug.  But
that is a very misguided thing to do.  If the source of the bug is
Emacs, we should correct the bug in the code, not hide it by
correcting it more or less at random in this clumsy way.  If the user
setq-ed the variable rather than calling the function or setting the
variable through Custom, then you have to give the user the
opportunity to find out that it does not work.  Correcting it at
random when a file gets loaded is not helping the user, but leading
him into total confusion: sometimes setting the variable works,
sometimes not, but then at some given moment, say when browsing Custom
for unrelated reasons, it all of a sudden starts working again.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

>From my previous message:

   So, to do what the comment says, we should replace the default
   :initialize for minor modes from 'custom-initialize-default to
   'custom-initialize-set.

Well actually, one might need to replace `custom-initialize-default'
with a new :initialize function, custom-initialize-mode or something,
that does nothing if the variable is already bound, initializes the
option to nil using set-default if the variable is bound and the
init-value is nil, but calls the :set function if the variable is
bound and init-value is non-nil.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Richard Stallman wrote:

   It would be cleaner to do the same thing for :set and :type--to suppress
   the default one when there is an explicit one.

Done.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Since the current version of `define-minor-mode', is incompatible with
the new :initialize function customize-initialize-safe-default,
_something_ needs to be done.  (It currently breaks the building of
Emacs on certain operating systems and additional uses of
customize-initialize-safe-default could, with the current
define-minor-mode, easily break it on all operating systems.)

So either we apply the patch below, or we have to introduce a
condition-case form in the code removed by that patch.

I propose to apply the patch below.  It removes completely useless
code at the end of define-minor-mode.  I explained in the message I am
replying to and in my message before that one, why that code is not
only useless, but causes several very annoying misfeatures.

Here is a summary of these misfeatures:

1) The code uses eval-after-load, which, according to
   `(elisp)Hooks for Loading', well-designed Lisp programs should not
   use.  For good reason: it destroys the interactive nature of
   Elisp.  Any code that directly or indirectly uses define-minor-mode
   can not be reliably tested interactively, say in the *scratch*
   buffer or ielm.  It can only be tested reliably by loading files,
   as I found out.  My patch would remove this misfeature.

2) Just loading the file may result in the minor mode function being
   called.  It is true that there is precedent for this: the default
   :set function `custom-initialize-reset' has the same problem.  But
   reportedly, define-minor-mode uses custom-initialize-default,
   exactly to avoid that problem.  In vain, unless we apply the patch
   below.

3) The unnatural code I propose to erase makes `define-minor-mode'
   prone to errors if anything else in Emacs changes.  We are
   currently struggling with an example of that.

4) It disguises potential bugs in Elisp code where somebody sets the
   minor mode variable instead of calling the minor mode function, by
   more or less randomly "correcting" the bug for the current session
   only.  This makes debugging Emacs more difficult.

5) By more or less at random correcting _user_ bugs of the type
   discussed in (4), namely setting the minor mode variable with setq
   outside Custom, it confuses the user, because setting the variable
   sometimes appears to work and sometimes not.  If it never worked,
   it would be much easier for the user to realize that it was not the
   correct way to customize the minor mode.  Note that a user using
   setq instead of Custom is probably trying to learn and
   define-minor-mode should not interfere with his learning process.

The code I propose to remove does not take care of a problem which the
original code it replaced did address and which the current comment
still falsely claims it addresses: loading the file does currently
_not_ enable the minor mode if the standard value is non-nil and the
minor mode variable is unbound when the defcustom is evaluated.
_Maybe_ that problem should be taken care of, although that would
inevitably also reintroduce misfeature (2) above, but not in a very
bad way.  As I explained earlier, I believe that the best way to do
that would be to make define-minor-mode use a new :initialize
function.  The patch below does _not_ do that.

===File ~/easy-mmode-diff===================================
*** easy-mmode.el	15 Jul 2005 19:36:26 -0500	1.69
--- easy-mmode.el	18 Jul 2005 19:03:51 -0500	
***************
*** 267,278 ****
         (add-minor-mode ',mode ',lighter
  		       ,(if keymap keymap-sym
  			  `(if (boundp ',keymap-sym)
! 			       (symbol-value ',keymap-sym))))
! 
!        ;; If the mode is global, call the function according to the default.
!        ,(if globalp
! 	    `(if (and load-file-name (not (equal ,init-value ,mode)))
! 		 (eval-after-load load-file-name '(,mode (if ,mode 1 -1))))))))
  \f
  ;;;
  ;;; make global minor mode
--- 267,273 ----
         (add-minor-mode ',mode ',lighter
  		       ,(if keymap keymap-sym
  			  `(if (boundp ',keymap-sym)
! 			       (symbol-value ',keymap-sym)))))))
  \f
  ;;;
  ;;; make global minor mode
============================================================

Re: obsolete comment in tool-bar.el

[Stefan Monnier]

> The code I propose to remove does not take care of a problem which the
> original code it replaced did address and which the current comment
> still falsely claims it addresses: loading the file does currently
> _not_ enable the minor mode if the standard value is non-nil and the
> minor mode variable is unbound when the defcustom is evaluated.

Please check the commit-history of this piece of code (e.g. with
vc-annotate) to see that it used to do what you say it "should" do
(i.e. also call the minor-mode function if the init-value is non-nil).

The current code follows the following idea:

Loading a file should change Emacs's state as little as possible, so the
init-value of a minor mode should *describe* (not determine) the default
state of the minor-mode (in the case where the minor-mode function is not
executed).


        Stefan

Re: obsolete comment in tool-bar.el

[Luc Teirlinck]

Stefan Monnier wrote:
   
   Please check the commit-history of this piece of code (e.g. with
   vc-annotate) to see that it used to do what you say it "should" do
   (i.e. also call the minor-mode function if the init-value is non-nil).

First of all, I assume the lack of reaction to my patch means that you
no longer object to it.  I will wait another day or so to make sure
that this is indeed the case.  I will have to do something reasonably
soon, since without that patch (or proper alternative) Emacs fails to
build on certain operating systems.  To answer the above quote, the
define-minor-mode code used to contain:

;; If the mode is global, call the function according to the default.
,(if globalp `(if ,mode (,mode 1))))))

until you started making various changes to this section of
define-minor-mode.  The original code I quoted above had the
shortcoming of performing part of the initialization outside of the
defcustom, which can be confusing.  But it _did_ enable the mode if
the defcustom sat the variable to t and that was the intended purpose
of the code, as the comment makes clear.

   The current code follows the following idea:

   Loading a file should change Emacs's state as little as possible, so the
   init-value of a minor mode should *describe* (not determine) the default
   state of the minor-mode (in the case where the minor-mode function is not
   executed).

I understood that.  However setting the mode variable to t without
enabling the mode has some very bad consequences, to the point that I
believe it could be considered an outright bug.  If the mode has a
lighter, the mode line claims that the mode is enabled, whereas it is
not, confusing the user.  Even though the mode variable is non-nil,
the mode is disabled, so `M-x foo-mode' should enable it.  Instead,
`M-x foo-mode' is actually a no-op in this situation, which is also
very confusing to the user.

With the current code (with or without the code I propose to delete),
if you want to enable a global minor mode by default, you have to
preload the library that defines the minor mode before startup.el is
loaded and then enable the mode by calling the minor mode function in
startup.el.  (Calling it in startup.el instead of in the file defining
the minor mode prevents calling it again if for some reason the file
gets reloaded.)  This way you avoid the bugs described above.  Of
course, the author of a package not included with Emacs can not
preload his file.  So the author of such a file should put
`(if foo-mode (foo-mode 1))' in his file, at a place late enough that
calling foo-mode does not produce any errors.

One solution is to just document the above.  (Many people do not know
it.)  This would have the advantage that, even after the code I
propose to remove is removed, define-minor-mode does not need to be
called near the end of the file, because it would never call the minor
mode function.

Another solution is to change the :initialize function.  There are
some reasons why this would appear to be cleaner.  However, it would
require changing the order in which define-minor-mode does things and,
_if_ a global minor mode is enabled by default, then define-minor-mode
would have to be called at a place in the file where calling the minor
mode function will not lead to an error.

Sincerely,

Luc.

Re: obsolete comment in tool-bar.el

[Richard M. Stallman]

Your arguments seem persuasive.  If nobody comes up with
a strong objection in a few days, please install your patch.
But please also add a comment explaining briefly what
the remaining problem is.

Re: obsolete comment in tool-bar.el

[Stefan Monnier]

>    Loading a file should change Emacs's state as little as possible, so the
>    init-value of a minor mode should *describe* (not determine) the default
>    state of the minor-mode (in the case where the minor-mode function is not
>    executed).

> I understood that.  However setting the mode variable to t without
> enabling the mode has some very bad consequences, to the point that I
> believe it could be considered an outright bug.  If the mode has a
> lighter, the mode line claims that the mode is enabled, whereas it is
> not, confusing the user.  Even though the mode variable is non-nil,
> the mode is disabled, so `M-x foo-mode' should enable it.  Instead,
> `M-x foo-mode' is actually a no-op in this situation, which is also
> very confusing to the user.

No, you did not understand, please re-read.  Think about the difference
between describing and determining.  If without running the minor-mode
function its state is inactive, then the init-state has to be nil.
The init-state should only ever be t if the mode is active even without
having ever run the minor-mode function.


        Stefan