Re: obsolete comment in tool-bar.el

[Luc Teirlinck <teirllm@dms.auburn.edu>]

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.