* bizarre problem with minor mode defined using define-minor-mode [not found] ` <AANLkTimiwTZA4UHpEPo83cFZK9r4VWkwesGcDo4GGVGu@mail.gmail.com> @ 2010-12-26 0:31 ` ken manheimer 2011-01-11 21:42 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: ken manheimer @ 2010-12-26 0:31 UTC (permalink / raw) To: emacs-devel [-- 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 --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2010-12-26 0:31 ` bizarre problem with minor mode defined using define-minor-mode ken manheimer @ 2011-01-11 21:42 ` Stefan Monnier 2011-01-16 1:06 ` ken manheimer 0 siblings, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2011-01-11 21:42 UTC (permalink / raw) To: ken manheimer; +Cc: emacs-devel > 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-11 21:42 ` Stefan Monnier @ 2011-01-16 1:06 ` ken manheimer 2011-01-16 4:48 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: ken manheimer @ 2011-01-16 1:06 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-16 1:06 ` ken manheimer @ 2011-01-16 4:48 ` Stefan Monnier 2011-01-16 6:15 ` ken manheimer 0 siblings, 1 reply; 8+ messages in thread From: Stefan Monnier @ 2011-01-16 4:48 UTC (permalink / raw) To: ken manheimer; +Cc: emacs-devel > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-16 4:48 ` Stefan Monnier @ 2011-01-16 6:15 ` ken manheimer 2011-01-16 6:46 ` Stefan Monnier 0 siblings, 1 reply; 8+ messages in thread From: ken manheimer @ 2011-01-16 6:15 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-16 6:15 ` ken manheimer @ 2011-01-16 6:46 ` Stefan Monnier 2011-01-16 8:55 ` Andreas Schwab 2011-01-20 18:21 ` ken manheimer 0 siblings, 2 replies; 8+ messages in thread From: Stefan Monnier @ 2011-01-16 6:46 UTC (permalink / raw) To: ken manheimer; +Cc: emacs-devel >> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-16 6:46 ` Stefan Monnier @ 2011-01-16 8:55 ` Andreas Schwab 2011-01-20 18:21 ` ken manheimer 1 sibling, 0 replies; 8+ messages in thread From: Andreas Schwab @ 2011-01-16 8:55 UTC (permalink / raw) To: Stefan Monnier; +Cc: ken manheimer, emacs-devel 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." ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: bizarre problem with minor mode defined using define-minor-mode 2011-01-16 6:46 ` Stefan Monnier 2011-01-16 8:55 ` Andreas Schwab @ 2011-01-20 18:21 ` ken manheimer 1 sibling, 0 replies; 8+ messages in thread From: ken manheimer @ 2011-01-20 18:21 UTC (permalink / raw) To: Stefan Monnier; +Cc: emacs-devel 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-01-20 18:21 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- [not found] <AANLkTimx4fP1f9Cj3_3ErkWEqj7sxNgTXG_Yxz-zi0xV@mail.gmail.com> [not found] ` <AANLkTimf5w9-N=j7wLnb339xDD7PCGDf1f=H4EGoO1TJ@mail.gmail.com> [not found] ` <AANLkTimiwTZA4UHpEPo83cFZK9r4VWkwesGcDo4GGVGu@mail.gmail.com> 2010-12-26 0:31 ` bizarre problem with minor mode defined using define-minor-mode ken manheimer 2011-01-11 21:42 ` Stefan Monnier 2011-01-16 1:06 ` ken manheimer 2011-01-16 4:48 ` Stefan Monnier 2011-01-16 6:15 ` ken manheimer 2011-01-16 6:46 ` Stefan Monnier 2011-01-16 8:55 ` Andreas Schwab 2011-01-20 18:21 ` ken manheimer
Code repositories for project(s) associated with this external index https://git.savannah.gnu.org/cgit/emacs.git https://git.savannah.gnu.org/cgit/emacs/org-mode.git This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.