bizarre problem with minor mode defined using define-minor-mode

bizarre problem with minor mode defined using define-minor-mode

[ken manheimer]

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

i'm encountering a serious problem with a function created using
define-minor-mode - and the problem is only occurring when operating with
the byte-compiled version of the function.  i've tried to debug the problem,
but am stumped.  i'm able to reproduce the problem in a few versions of
emacs, though, and have created a bzr branch on launchpad with a copy of the
code.

one of the final steps in my updates to allout.el is migrating allout-mode
minor mode to `define-minor-mode'.  however, some key substitutions way deep
in my code do only happen properly when i use a explicitly evaluated (via
eval-defun or eval-when-compile) version of the define-minor-mode to define
the allout-mode function.  if the minor mode was established by a
byte-compiled version of the function the same key substitutions do not show
in the mode.  (there are some other keybinding problems that occur, as
well.)

i wonder whether i'm somehow misusing define-minor-mode in a dumb way, but
am entirely missing it if that's the case.

the problem happens identically on the x toolkit and chiba-u versions of
emacs, both running on macox X snow leopard:

 GNU Emacs 23.2.91.1 (x86_64-apple-darwin10.5.0, Carbon Version 1.6.0 AppKit
1038.35) of 2010-12-20 on crisp

 GNU Emacs 23.1.90.2 (x86_64-apple-darwin10.4.0, X toolkit) of 2010-11-05 on
crisp

i can reproduce the problem by wrapping the define-minor-mode in either
`eval-and-compile' or `eval-when-compile' and doing an
`emacs-lisp-byte-compile-and-load' to show the different behaviors.  i've
created a bzr branch on launchpad.net with a prepared version (details,
below) of allout.el for others to try:


http://bazaar.launchpad.net/~ken-manheimer/emacs/alloutdev/annotate/head:/lisp/allout.el?start_revid=102483

in this version i've wrapped the define-minor-mode with eval-and-compile.
 that can be changed to test the different behaviors.

when byte-compiling and loading with the eval-when-compile, invoking the
mode yields the key substitutions, as it ought.  (of course, the step of
loading the byte-compiled code is irrelevant at that point, because the
function isn't in the byte-compiled code.  it's only defined because it was
evaluated during the byte-compile.)   with the eval-and-compile, loading the
byte-compiled allout.elc wipes out the key substitutions - they're no longer
in effect.  of course, the lack of the function in the byte-compiled code
when using eval-when-compile makes the byte-compiled code useless, so i have
no workaround for the problem, just a weird demonstration of it.

i've tried tracking down the failed key substitutions, by
wrapping define-key with some advice that noted when the key defines
happened, and they do every time the mode is activated, whether or not it's
via the byte-compiled version of the mode function (when defined
by define-minor-mode).  my function which does the key
substitutions (allout-setup-mode-map) uses fset to ensure that the mode map
is properly globally established, so it's already kind of complicated.  i'm
also wondering whether i'm just misunderstanding something about the way
define-minor-mode is supposed to work.

to see the problem, visit this version (see link, above) of allout.el and
start allout-mode.  then emacs-lisp-byte-compile-and-load the code, and
you'll find that \C-a is bound to move-beginning-of-line, as it normally is
- not replaced by allout-beginning-of-line, as it should be.

then change the `eval-and-compile' that's wrapped around
the define-minor-mode allout-mode to `eval-when-compile',
do emacs-lisp-byte-compile-and-load, and you'll find that \C-a is properly
bound to allout-beginning-of-line.  (you can also just manually evaluate the
define-minor-mode - that will also the keysubsitutions in the keymap
immediately.)

help would be much appreciated!
--
ken manheimer
http://myriadicity.net

[-- Attachment #2: Type: text/html, Size: 4263 bytes --]

Re: bizarre problem with minor mode defined using define-minor-mode

[Stefan Monnier]

> i've tried tracking down the failed key substitutions, by
> wrapping define-key with some advice that noted when the key defines
> happened, and they do every time the mode is activated, whether or not it's
> via the byte-compiled version of the mode function (when defined
> by define-minor-mode).  my function which does the key
> substitutions (allout-setup-mode-map) uses fset to ensure that the mode map
> is properly globally established, so it's already kind of complicated.  i'm
> also wondering whether i'm just misunderstanding something about the way
> define-minor-mode is supposed to work.

I think the code is too complex for its own good, so I can't really
track down the problem.  But AFAICT, the `allout-mode-map' symbol is
never used as a keymap, so the (fset 'allout-mode-map allout-mode-map)
has no effect: all the rest of the code uses the allout-mode-map
*variable* (i.e. the value stored in the `symbol-value' part of the
allout-mode-map symbol).


        Stefan


PS: While I'm here, don't use `setq' at top-level.  Either put the right
initial value into `defvar', or if you want that value to be re-set when
rereading the file, use `defconst' instead of defvar, but don't use
`setq' at top-level.

Re: bizarre problem with minor mode defined using define-minor-mode

[ken manheimer]

On Tue, Jan 11, 2011 at 4:42 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> i've tried tracking down the failed key substitutions, by
>> wrapping define-key with some advice that noted when the key defines
>> happened, and they do every time the mode is activated, whether or not it's
>> via the byte-compiled version of the mode function (when defined
>> by define-minor-mode).  my function which does the key
>> substitutions (allout-setup-mode-map) uses fset to ensure that the mode map
>> is properly globally established, so it's already kind of complicated.  i'm
>> also wondering whether i'm just misunderstanding something about the way
>> define-minor-mode is supposed to work.
>
> I think the code is too complex for its own good, so I can't really
> track down the problem.  But AFAICT, the `allout-mode-map' symbol is
> never used as a keymap, so the (fset 'allout-mode-map allout-mode-map)
> has no effect: all the rest of the code uses the allout-mode-map
> *variable* (i.e. the value stored in the `symbol-value' part of the
> allout-mode-map symbol).

i agree that the code is painfully complicated.  however, the fset
actually is effective and necessary to impose the minor-mode keymap
adjustments allout does.

in fact, removing the fset leads to the same problems with the
regular, defun'ed allout-mode that i have been seeing with the
byte-compiled define-minor-mode defined allout mode.  seeing that lead
to the cause of the problem, or at least a workaround.  the workaround
adds more complexity, though.  i'd like to describe the situation to
see if anyone can suggest a better approach.

allout needs to adjust the keymap when the mode is being established,
to take into account some user key binding customizations, of:
`allout-command-prefix', `allout-prefixed-keybindings', and
`allout-unprefixed-keybindings'.  to enable this, i made allout
register its minor-mode-map-alist entry as `(allout-mode .
allout-mode-map)'.  for this format, the keymap facility seeks the key
bindings in allout-mode-map's function value.

the problem with switching to define-minor-mode is that
define-minor-mode apparently associates the mode name with the keymap
value, itself, on minor-mode-map-alist.  thus, the dynamic adjustments
- the customized keymap prefix and key binding substitutions - weren't
taking effect, depending on the order of the redundant entries.

(i assume that ordering variations are why i would not see the problem
when i did a manual eval-defun of the define-minor-mode form, but did
see it when using the byte-compiled definition.)

since i don't control how define-minor-mode populates
minor-mode-map-alist, i have my allout-setup-mode-map routine clear
out all allout-mode entries on minor-mode-map-alist and reestablish
the `(allout-mode . allout-mode-map)' entry.  apparently
define-minor-mode does its keymap arranging early on, so calling
allout-setup-mode-map in the body corrects for the setting and all is
now ok.

except the code is still complicated.  i am open to suggestions about
simpler ways to provide for the customizable key bindings, but in the
meanwhile am checking in the current setup to the trunk.

this would be simpler if define-minor-mode used the `(x-mode .
x-mode-map)' form of entry on minor-mode-map-alist.  that would allow,
in general, for changing the keybindings without removing the
association on minor-mode-map-alist.

oh, yeah, i also migrated the setq stuff to the defvar i already had
established.  i think the reason i used a separate setq was for an
early version of the dynamic keymap settings, and i never rectified
the separation once i revised the approach.  thanks for that pointer,
and thanks much for your attention to my help request, in general!

ken

>        Stefan
>
> PS: While I'm here, don't use `setq' at top-level.  Either put the right
> initial value into `defvar', or if you want that value to be re-set when
> rereading the file, use `defconst' instead of defvar, but don't use
> `setq' at top-level.

Re: bizarre problem with minor mode defined using define-minor-mode

[Stefan Monnier]

> i agree that the code is painfully complicated.  however, the fset
> actually is effective and necessary to impose the minor-mode keymap
> adjustments allout does.

I really can't understand who it could be effective.  My reading of the
code tells me that function binding is never used.

> in fact, removing the fset leads to the same problems with the
> regular, defun'ed allout-mode that i have been seeing with the
> byte-compiled define-minor-mode defined allout mode.

That's what I would expect, since I think this fset is a no-op.

> the problem with switching to define-minor-mode is that
> define-minor-mode apparently associates the mode name with the keymap
> value, itself, on minor-mode-map-alist.

That's because that's what you told it:

  :keymap <exp>

says to use the value of <exp> as the keymap, so ":keymap
allout-mode-map" says to use the value of the variable as the keymap.
If you want to use a symbol, then you need to quote it.

This said, you can modify a keymap after the fact: as long as you don't
do a (setq allout-mode-map <newmap>), you can modify allout-mode-map on
the fly and those changes will take effect immediately without having to
use an indirection through the allout-mode-map symbol.


        Stefan

Re: bizarre problem with minor mode defined using define-minor-mode

[ken manheimer]

On Sat, Jan 15, 2011 at 11:48 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> i agree that the code is painfully complicated.  however, the fset
>> actually is effective and necessary to impose the minor-mode keymap
>> adjustments allout does.
>
> I really can't understand who it could be effective.  My reading of the
> code tells me that function binding is never used.

(elisp)Format of Keymaps: "A symbol whose function definition is a
keymap is also a keymap."

minor-mode-map-alist entries of the form `(allout-mode .
allout-mode-map)' apparently tells the emacs key resolution facility
to look in the function value of the symbol in the cdr.  that's why
the fset i do works.

>> in fact, removing the fset leads to the same problems with the
>> regular, defun'ed allout-mode that i have been seeing with the
>> byte-compiled define-minor-mode defined allout mode.
>
> That's what I would expect, since I think this fset is a no-op.

it's not a no-op!  as i said before, removing the fset changed
behavior.  as i describe above, there's an explanation.

>> the problem with switching to define-minor-mode is that
>> define-minor-mode apparently associates the mode name with the keymap
>> value, itself, on minor-mode-map-alist.
>
> That's because that's what you told it:
>
>  :keymap <exp>
>
> says to use the value of <exp> as the keymap, so ":keymap
> allout-mode-map" says to use the value of the variable as the keymap.
> If you want to use a symbol, then you need to quote it.

alas, that's apparently not so:

  (defvar example-mode-map (make-sparse-keymap))
  => example-mode-map

  (define-key example-mode-map "\C-cv" 'emacs-version)
  => emacs-version

  (assq 'example-mode minor-mode-map-alist)
  => nil

  (define-minor-mode example-mode
    "example-mode docstring."
    :keymap 'example-mode-map
    nil)
  => (keymap (3 keymap (118 . emacs-version)))

  (assq 'example-mode minor-mode-map-alist)
  => (example-mode keymap (3 keymap (118 . emacs-version)))

(it's helpful to discover that just executing the define-minor-mode
establishes the keymap in minor-mode-map-alist.  i didn't expect that.
 actually activating the mode doesn't change the situation.)

> This said, you can modify a keymap after the fact: as long as you don't
> do a (setq allout-mode-map <newmap>), you can modify allout-mode-map on
> the fly and those changes will take effect immediately without having to
> use an indirection through the allout-mode-map symbol.

are you suggesting doing a rplacd on the minor-mode-map-alist cell?  i
guess that's better than the fset approach, since for the latter, in
addition to doing the obscure fset, i also have to do surgery to
remove the keymap that define-minor-mode puts on minor-mode-map-alist
(whether i provide :keymap with a keymap or a symbol).

>        Stefan

Re: bizarre problem with minor mode defined using define-minor-mode

[Stefan Monnier]

>> I really can't understand who it could be effective.  My reading of the
>> code tells me that function binding is never used.

> (elisp)Format of Keymaps: "A symbol whose function definition is a
> keymap is also a keymap."

> minor-mode-map-alist entries of the form `(allout-mode .
> allout-mode-map)' apparently tells the emacs key resolution facility
> to look in the function value of the symbol in the cdr.  that's why
> the fset i do works.

I'm talking about my reading of the allout.el code: nowhere does your
code use the function cell of that variable, AFAICT.  I know you
*intend* to use it, and I see you set it, but I don't see where it's
used (i.e. where you use the symbol as a keymap).

>>> in fact, removing the fset leads to the same problems with the
>>> regular, defun'ed allout-mode that i have been seeing with the
>>> byte-compiled define-minor-mode defined allout mode.
>> That's what I would expect, since I think this fset is a no-op.
> it's not a no-op!  as i said before, removing the fset changed
> behavior.  as i describe above, there's an explanation.

Then, I'm lost.

>>> the problem with switching to define-minor-mode is that
>>> define-minor-mode apparently associates the mode name with the keymap
>>> value, itself, on minor-mode-map-alist.
>> 
>> That's because that's what you told it:
>> 
>>  :keymap <exp>
>> 
>> says to use the value of <exp> as the keymap, so ":keymap
>> allout-mode-map" says to use the value of the variable as the keymap.
>> If you want to use a symbol, then you need to quote it.

> alas, that's apparently not so:

Indeed, I misremembered: the :keymap argument is not an expression, it's
a funny thing, described in the docstring as:

Optional KEYMAP is the default keymap bound to the mode keymap.
  If non-nil, it should be a variable name (whose value is a keymap),
  a keymap, or a list of arguments for `easy-mmode-define-keymap'.
  If KEYMAP is a keymap or list, this also defines the variable MODE-map.

And it's not clear at all when/if that thing is evaluated (i.e. if it
can be an expression).

So if you use `allout-mode-map', it will use the value of that variable,
and if you use something else, it will turn it into a keymap, set it as
default value of allout-mode-map, and then use the value of
allout-mode-map.  Since the allout-mode-map is already defined before,
this ends up having no effect.  Here's a relevant sample of IELM
session:

ELISP> (macroexpand '(define-minor-mode allout-mode "doc" :keymap 'blabla))
  (defvar allout-mode-map
    (let ((m #1='blabla))
      (cond ((keymapp m) m) ((listp m) (easy-mmode-define-keymap m))
            (t (error "Invalid keymap %S" #1#))))
    "Keymap for `allout-mode'.")
  (with-no-warnings (add-minor-mode 'allout-mode 'nil allout-mode-map nil nil)))

I suggest to stay away from the :keymap argument.  Instead, do it this way:

(defvar allout-mode--map <blabla>)
(defalias 'allout-mode-map allout-mode--map)
(defvar allout-mode-map 'allout-mode-map)

>> This said, you can modify a keymap after the fact: as long as you don't
>> do a (setq allout-mode-map <newmap>), you can modify allout-mode-map on
>> the fly and those changes will take effect immediately without having to
>> use an indirection through the allout-mode-map symbol.
> are you suggesting doing a rplacd on the minor-mode-map-alist cell?

No, I'm really talking about modifying the keymap itself directly rather
than modifying various variables that point to it.  E.g. I'm suggesting
doing a bunch of define-key only.  But if some of your changes involve
adding/moving bindings it's more difficult, so you could instead use
(setcdr allout-mode-map (cdr newmap)), but that would be ugly.

But the above

(defvar allout-mode-map 'allout-mode-map)

should work much more cleanly.


        Stefan

Re: bizarre problem with minor mode defined using define-minor-mode

[Andreas Schwab]

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

> I'm talking about my reading of the allout.el code: nowhere does your
> code use the function cell of that variable, AFAICT.

Here:

          (setq minor-mode-map-alist
                (cons '(allout-mode . allout-mode-map)
                      minor-mode-map-alist)))

(allout is the only mode doing it that way.)

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: bizarre problem with minor mode defined using define-minor-mode

[ken manheimer]

On Sun, Jan 16, 2011 at 1:46 AM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
> [...]
>>>> in fact, removing the fset leads to the same problems with the
>>>> regular, defun'ed allout-mode that i have been seeing with the
>>>> byte-compiled define-minor-mode defined allout mode.
>>> That's what I would expect, since I think this fset is a no-op.
>> it's not a no-op!  as i said before, removing the fset changed
>> behavior.  as i describe above, there's an explanation.
>
> Then, I'm lost.

andreas schwab pointed out the place.

my mistake was assuming that define-minor-mode would notice that the
intended minor-mode-map-alist entry already existed and leave it be.

>[...]
> So if you use `allout-mode-map', it will use the value of that variable,
> and if you use something else, it will turn it into a keymap, set it as
> default value of allout-mode-map, and then use the value of
> allout-mode-map.  Since the allout-mode-map is already defined before,
> this ends up having no effect.  Here's a relevant sample of IELM
> session:

(IELM looks good - thanks for mentioning it.  likewise, macroexpand is
very helpful - i should have figured such a thing existed, and sought
it to check what define-minor-mode does.)

> I suggest to stay away from the :keymap argument.  Instead, do it this way:
>
> (defvar allout-mode--map <blabla>)
> (defalias 'allout-mode-map allout-mode--map)
> (defvar allout-mode-map 'allout-mode-map)

i'm doing what you suggest, though i'm also using the
define-minor-mode :keymap argument, since it'll work fine with this
scheme.

>>> This said, you can modify a keymap after the fact: as long as you don't
>>> do a (setq allout-mode-map <newmap>), you can modify allout-mode-map on
>>> the fly and those changes will take effect immediately without having to
>>> use an indirection through the allout-mode-map symbol.
>> are you suggesting doing a rplacd on the minor-mode-map-alist cell?
>
> No, I'm really talking about modifying the keymap itself directly rather
> than modifying various variables that point to it.  E.g. I'm suggesting
> doing a bunch of define-key only.  But if some of your changes involve
> adding/moving bindings it's more difficult, so you could instead use
> (setcdr allout-mode-map (cdr newmap)), but that would be ugly.

in fact, allout enables the user to set the command prefix and any of
the bindings via customization, so a lot of bindings can change at any
time.  (that may be the reason for the fact that allout uses the
'(allout-mode . allout-mode-map) entry form on minor-mode-map-alist
while other minor modes don't, as andreas schwab noticed.)  i agree
that using setcdr this way is ugly, and realize it's not necessary.

> But the above
>
> (defvar allout-mode-map 'allout-mode-map)
>
> should work much more cleanly.

i've adopted this approach instead of fset in my new revision, and it
is cleaner.  i've also unravelled and eliminated a bunch of
accumulated cruft, getting rid of a bunch of code and leaving what
seems to be some necessary complexity, like the defalias, in order to
deal with the way keymaps and define-minor-mode work.

upshot: allout is now migrated to define-minor-mode (as well as epg
instead of pgg, previously), and there's been a housecleaning of a lot
of residual cruft and bugs in the process.  it's not been painless,
but i admit necessary.  thanks for the help in arriving at decent
technical solutions for some of the tricky questions along the way.
let me know if you find objections with what i've done.

ken