Re: savehist -- save minibuffer history between Emacs invocations

[Hrvoje Niksic]

Simon Josefsson <address@hidden> writes:

>> And from the available documentation I understand define-minor-mode
>> to be pretty much equivalent to the combination of `add-minor-mode'
>> and the code I've written.
>
> define-minor-mode also create a customizable foo-mode variable,
> which load and enable the mode if it is customized in ~/.emacs.

Hmm.

(define-minor-mode foo-mode "blah")
(symbol-plist 'foo-mode)
  -> (variable-documentation "Non-nil if Foo mode is enabled.
Use the command `foo-mode' to change this variable.")

Are you sure it gives the variable any special properties recognized
by custom?  I should at least have a `custom-type' property.

>> If you know how to correctly achieve that with `defcustom', by all
>> means let me know.
>
> How about this patch?
[...]
> -;;     (savehist-mode 1)
> +;;     (require 'savehist)

This is bad style -- libraries are not supposed to change the state of
Emacs by merely being loaded.  From the "Coding Conventions" node of
the Elisp manual:

   * When a package provides a modification of ordinary Emacs behavior,
     it is good to include a command to enable and disable the feature,
     Provide a command named `WHATEVER-mode' which turns the feature on
     or off, and make it autoload (*note Autoload::).  Design the
     package so that simply loading it has no visible effect--that
     should not enable the feature.(2)  Users will request the feature
     by invoking the command.

     (2) Consider that the package may be loaded arbitrarily by Custom
     for instance.

> -(defvar savehist-mode nil
> -  "Mode for automatic saving of minibuffer history.
> -Set this by calling `savehist-mode'.")
> +(defcustom savehist-mode nil
> +  "Mode for automatic saving of minibuffer history."
> +  :type 'boolean
> +  :require 'savehist
> +  :group 'savehist)

That requires loading savehist to install savehist-mode, which is
wrong.  I was thinking of something along the lines of
 :set (lambda (val) (savehist-mode (if val 1 0))), but I couldn't
really make it work.