Re: Enhancements to "minor-mode-map-alist" functionality.

[Stefan Monnier]

> "Stefan Monnier" <monnier+gnu/address@hidden> writes:
> 
> > > However, this can be dramatically simplified by two (simple)
> > > enhancements to the minor-mode-map-alist functionality:
> > > 
> > > 1) Provide a separate emulation-mode-map-alists variable where
> > > modes like cua can insert their own mode-map-alist,
> > 
> > As you probably know, I'm not too excited about this, since it shouldn't
> > be necessary if we could enforce a bit more discipline in the way
> > entries are added to minor-mode-map-alist.
> 
> I doubt we can find a way to enforce that.

In an ideal world, all modifications of the alist would go through
either add-minor-mode or define-minor-mode, where we could enforce it.
I agree that it's not realistic.

> > [ Yes, I wish we had "watchers" so we could trap modifications of
> >   particular variables. ]
> > 
> Which would still not ensure anything -- if multiple packages are
> watching minor-mode-map-alist and rearranges it, we'd still not
> be able to _ensure_ any specific ordering...

Well, there would be a single watcher that provided the same functionality
as your emulation alist and both viper and cua would use it.

> > Also, have you contacted Michael Kifer to see if that would satisfy his
> > needs for viper (I know he also had to do some funky post-command
> > fiddling of the minor-mode-map-alist).
> As far as I can see, it is _exactly_ the same problem (viper has 9 keymaps).

That's my impression as well, but maybe he has some other idea of what
the ideal solution should look like.

> > > 2) Allow selection of the active keymaps in the alist to
> > > be based on evaluating a form in addition to a simple variable.
> > 
> > I generally like the idea of having the keymaps more dynamic,
> > but I'd rather not add another form of dynamism that isn't quite
> > good enough for everything.  To be more precise, currently you can get
> > the above "select the keymap dynamically" in a more generic way
> > (i.e. not just for minor mode maps) by using an entry of the form:
> > 
> >     '(menu-item "foo" data :filter fun)
> > 
> > It is mostly used for dynamic menus, but also works in other keymaps.
> > Problem is, it only works for submaps because it is not itself
> > a keymap and thus can't be used for a toplevel map.
> 
> I need this at the top-level maps, so it is not applicable here.

I kind of expected it :-(

> But if we could support something like (keymap :filter fun ...),
> it would satisfy the needs for cua.

I had something like that working in the past.  I don't think
I have the code around any more, tho.  IIRC it wasn't too difficult
to add.  The evaluation of the function was done in `get_keymap'
(this is not to say that it was the right place to do it, but that
it was my choice at the time, because it seemed simpler).

Another idea I have been toying with (but it never got far enough
for me to start writing any code, not even tentatively) is to use
a "null event".  Assuming this event is called `nil', you could
replace the above dynamic keymap with:

        (defvar dynmap (make-sparse-keymap))
        (define-key dynmap [nil] '(menu-item "foo" data :filter fun))

Ideally this would also allow multiple such "same-level submaps"
which would then provide multiple keymap inheritance.

> > So if the dynamism you need is only in the `command-remap' submap,
> > for example, your new hack is not needed.
> I don't think this is a hack!

I said "hack" because I think it's more ad-hoc than it should.

>   ;; Neat trick from Dave Love to add more bindings in read-only mode:
>   (add-to-list 'minor-mode-map-alist
>         `(lambda . (and diff-minor-mode buffer-read-only
>                    '(diff-minor-mode . ,diff-mode-shared-map))))

Indeed.

>       ;; View mode steals RET from us.
>       (local-set-key [remap View-scroll-line-forward] 'help-follow)

That one looks completely bogus to me.  Do you really want to prevent
the use of View-scroll-line-forward from a help-mode buffer even if
it's bound to something else than RET ?

> [Actually, I think I'll install that change in any case].

Please don't.

> > I'd rather not add a hack that's specific to minor-mode-map-alist
> > if we could do the same for all cases instead.
> 
> Sure.  But what device do you suggest then?
>   (keymap :filter fun ...) ?
> 
> I don't object to that, but it would be _less_ efficient, since
> we have to search the keymap for the :filter property (like we do
> for a menu-item, but that is much shorter)  [unless we ensure
> it stays at the head of the keymap list].

I don't care much for efficiency at this stage.

> Also, the keymap lookup code would also have to be careful not to
> interpret the FUN part as a binding,

You can force it to be a symbol.

> and when we add binding to
> a map, we should be careful not to split the ":filter FUN" pair.

Of course, it might need some care.  Doing it in get_keymap works
around the above problem, tho since the keymap that's modified
is the one returned by get_keymap.

> So GC-wise this only makes things marginally worse :-/

I'd rather try to make it better than to make it marginally worse.

> > In other words, maybe we shouldn't evaluate the form inside this
> > current_minor_maps function.
> I still think it is pretty safe to do so.

Despite the comment that's just above ?  Do you really think the
coders went to the trouble of not using xmalloc/xrealloc and of
sometimes returning an incorrect result (hoping that the cause of
the problem will cause something else to abort later on) just
for the fun of it ?

> If some packages decide to fiddle with minor-mode-map-alist when you
> activate the mode the first time (rather than on load), I don't
> think an after-load-hook will catch that.

And if some package decides to fiddle with it from an idle-hook, the
post-command-hook will fail as well.  The question is not "is it theoretically
safe" but "is it safe in practice" ?  I believe it'd be good enough in practice
because I don't see much fiddling of minor-mode-map-alist outside of
toplevel expressions.


        Stefan