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

[storm@cua.dk (Kim F. Storm)]

"Stefan Monnier" <monnier+gnu/emacs@rum.cs.yale.edu> 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.

> [ 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...

> But I guess that I can live with it.

Good :-)

> 
> BTW, should emulation-mode-map-alists take precedence over
> minor-mode-overriding-map-alist or not ?

Based on the two existing uses of minor-mode-overriding-map-alist, I
don't think it matters much, but e-m-m-a should probably take
precedence over m-m-o-m-a...  I'll change that.

> 
> 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).

> > 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.
But if we could support something like (keymap :filter fun ...),
it would satisfy the needs for cua.

> 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!  If so, the minor-mode-map-alist itself
is a hack, since it has several obvious limitations.  One of which
is remedied by the minor-mode-overriding-map-alist!

Actually, we could discard that feature (don't say that we should)
and use either the dynamic keymap selection suggested, e.g.

From diff-mode.el:

  ;; Neat trick from Dave Love to add more bindings in read-only mode:
  (add-to-list (make-local-variable 'minor-mode-overriding-map-alist)
  	       (cons 'buffer-read-only diff-mode-shared-map))

could be replaced by:

  ;; 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))))

Or maybe a simple command remapping the the buffer's local map would
do just as well, e.g.

From help-mode.el:

      ;; View mode steals RET from us.
      (set (make-local-variable 'minor-mode-overriding-map-alist)
           (list (cons 'view-mode
                       (let ((map (make-sparse-keymap)))
                         (set-keymap-parent map view-mode-map)
                         (define-key map "\r" 'help-follow)
                         map))))

could simply be

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

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

That's the two uses of minor-mode-overriding-map-alist I could find.

> 
> 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].

Also, the keymap lookup code would also have to be careful not to 
interpret the FUN part as a binding, and when we add binding to
a map, we should be careful not to split the ":filter FUN" pair.
 
> > I have already implemented these features (see attached patch),
> > which allows me to configure all the minor mode keymaps needed
> > by cua once and for all:
> 
> In the patch, you end up evaluating elisp code from a function
> that already has a comment about the fact that it needs to be extra-careful
> about memory allocation.

I know, but given the other options, I would expect this to do a lot
less memory allocations (if any!) than the alternative (of
regenerating minor-mode-map-alist after every command - and setting
the 9 destinct variables needed to control it - as viper does [more or
less]).

In my case (cua), I don't think I make any memory allocations there at all
since I only test a number of variables -- not modify anything.

>                        Also I'm not sure that all places that call this
> function are ready to have elisp code executed at that point (you might
> need some GCPROs added here and there).

As I've pointed out before, a lot of functions in keymap already lack
proper GCPROs due to the autoload enhancements to get_keymap...

And now you tell me that get_keyelt can GC as well...  I don't see
any GCPROs related to that either...

So GC-wise this only makes things 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.

But I think it would be better to use menu_item_eval_property than
safe_eval for this purpose.

> 
> 	Stefan
> 
> PS: by the way, isn't it enough to fiddle with minor-mode-map-alist
>     in after-load-hook rather than in post-command-hook ?
>     [ assuming we had such an after-load-hook. ]
> 
> 

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.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk