Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode

Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode

[Alan Mackenzie]

Hi, Emacs.

The situation here is the direct cause of bug #11152, where in CC Mode,
doc comments whose fontification is specified in a mode hook don't get
fontified properly, or at all.  It would be good to fix this bug for
Emacs 24.3.

In define-globalized-minor-mode L72-75, the newly defined -enable-
function is added to both the following hooks:
    change-major-mode-after-body-hook
    after-change-major-mode-hook
.  (These hooks are run before and after the major mode hook.)

It seems the hacker who formulated this macro was undecided whether to
run the -enable- function before or after the mode hooks, so decided
upon both as a compromise.  This isn't harmless.

In particular, running `global-font-lock-mode-enable-in-buffers' before
the objc-mode-hook causes a one-time font-lock-keywords initialisation
to happen before a critical initialisation has been performed by that
hook.

Can some means be found so that `font-lock-mode' isn't called twice?
(For that matter, `global-hi-lock-mode-enable-in-buffers' doesn't need
calling twice, either.)  Such a means might be the addition of another
parameter to define-globalized-minor-mode specifying when the -enable-
call should take place.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode

[Dmitry Gutov]

Alan Mackenzie <acm@muc.de> writes:

> The situation here is the direct cause of bug #11152, where in CC Mode,
> doc comments whose fontification is specified in a mode hook don't get
> fontified properly, or at all.  It would be good to fix this bug for
> Emacs 24.3.
>
> In define-globalized-minor-mode L72-75, the newly defined -enable-
> function is added to both the following hooks:
>     change-major-mode-after-body-hook
>     after-change-major-mode-hook
> .  (These hooks are run before and after the major mode hook.)
>
> It seems the hacker who formulated this macro was undecided whether to
> run the -enable- function before or after the mode hooks, so decided
> upon both as a compromise.  This isn't harmless.
>
> In particular, running `global-font-lock-mode-enable-in-buffers' before
> the objc-mode-hook causes a one-time font-lock-keywords initialisation
> to happen before a critical initialisation has been performed by that
> hook.

Sounds similar to http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11929

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Alan Mackenzie]

Hi, Emacs.

On Sun, Jan 13, 2013 at 07:28:54PM +0000, Alan Mackenzie wrote:
> The situation here is the direct cause of bug #11152, where in CC Mode,
> doc comments whose fontification is specified in a mode hook don't get
> fontified properly, or at all.  It would be good to fix this bug for
> Emacs 24.3.

> In define-globalized-minor-mode L72-75, the newly defined -enable-
> function is added to both the following hooks:
>     change-major-mode-after-body-hook
>     after-change-major-mode-hook
> .  (These hooks are run before and after the major mode hook.)

> It seems the hacker who formulated this macro was undecided whether to
> run the -enable- function before or after the mode hooks, so decided
> upon both as a compromise.  This isn't harmless.

> In particular, running `global-font-lock-mode-enable-in-buffers' before
> the objc-mode-hook causes a one-time font-lock-keywords initialisation
> to happen before a critical initialisation has been performed by that
> hook.

> Can some means be found so that `font-lock-mode' isn't called twice?
> (For that matter, `global-hi-lock-mode-enable-in-buffers' doesn't need
> calling twice, either.)  Such a means might be the addition of another
> parameter to define-globalized-minor-mode specifying when the -enable-
> call should take place.

I propose to amend define-globalized-minor-mode such that the generated
-enable- function gets added to precisely one of the following hooks, not
both:
    change-major-mode-after-body-hook
    after-change-major-mode-hook
, the latter one by default.  To change this default, the new
keyword-parameter :call-before-hook is introduced.  I think this
amendment should go into Emacs 24.3 (it "solves" bug #11152).

There are 6 invocations of define-globalized-minor-mode in Emacs,
1 global-visual-line-mode
2 global-hi-lock-mode
3 global-font-lock-mode
4 global-cwarn-mode
5 global-linum-mode
6 global-highlight-changes-mode
.  2 and 3 appear to work fine with the amended macro.  I haven't tested
the others.

Here is the patch to the .el file (another will be needed for NEWS):


=== modified file 'lisp/emacs-lisp/easy-mmode.el'
*** lisp/emacs-lisp/easy-mmode.el	2013-01-01 09:11:05 +0000
--- lisp/emacs-lisp/easy-mmode.el	2013-01-14 15:50:50 +0000
***************
*** 329,335 ****
    and that should try to turn MODE on if applicable for that buffer.
  KEYS is a list of CL-style keyword arguments.  As the minor mode
    defined by this function is always global, any :global keyword is
!   ignored.  Other keywords have the same meaning as in `define-minor-mode',
    which see.  In particular, :group specifies the custom group.
    The most useful keywords are those that are passed on to the
    `defcustom'.  It normally makes no sense to pass the :lighter
--- 329,338 ----
    and that should try to turn MODE on if applicable for that buffer.
  KEYS is a list of CL-style keyword arguments.  As the minor mode
    defined by this function is always global, any :global keyword is
!   ignored.  If :call-before-hook is present with a non-nil argument, TURN-ON
!   will be called before running the major mode's hook, otherwise it will be
!   called afterwards.
!  Other keywords have the same meaning as in `define-minor-mode',
    which see.  In particular, :group specifies the custom group.
    The most useful keywords are those that are passed on to the
    `defcustom'.  It normally makes no sense to pass the :lighter
***************
*** 346,351 ****
--- 349,355 ----
  	 (pretty-name (easy-mmode-pretty-mode-name mode))
  	 (pretty-global-name (easy-mmode-pretty-mode-name global-mode))
  	 (group nil)
+ 	 (call-before-hook nil)
  	 (extra-keywords nil)
  	 (MODE-buffers (intern (concat global-mode-name "-buffers")))
  	 (MODE-enable-in-buffers
***************
*** 362,367 ****
--- 366,372 ----
        (pcase keyw
  	(`:group (setq group (nconc group (list :group (pop keys)))))
  	(`:global (setq keys (cdr keys)))
+ 	(`:call-before-hook (setq call-before-hook (pop keys)))
  	(_ (push keyw extra-keywords) (push (pop keys) extra-keywords))))
  
      (unless group
***************
*** 394,402 ****
  	 ;; Setup hook to handle future mode changes and new buffers.
  	 (if ,global-mode
  	     (progn
! 	       (add-hook 'after-change-major-mode-hook
! 			 ',MODE-enable-in-buffers)
! 	       (add-hook 'change-major-mode-after-body-hook
  			 ',MODE-enable-in-buffers)
  	       (add-hook 'find-file-hook ',MODE-check-buffers)
  	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
--- 399,407 ----
  	 ;; Setup hook to handle future mode changes and new buffers.
  	 (if ,global-mode
  	     (progn
! 	       (add-hook ',(if call-before-hook
! 			       'change-major-mode-after-body-hook
! 			     'after-change-major-mode-hook)
  			 ',MODE-enable-in-buffers)
  	       (add-hook 'find-file-hook ',MODE-check-buffers)
  	       (add-hook 'change-major-mode-hook ',MODE-cmhh))



-- 
Alan Mackenzie (Nuremberg, Germany).

emacs24/auctex bug

[Camm Maguire]

Greetings!  When I try to open a .bib file in emacs24 with auctex
loaded, I hang with this backtrace.  Suggestions?

=============================================================================
Debugger entered--Lisp error: (quit)
  dbus-call-method-non-blocking(:session "org.gnome.evince.Daemon" "/org/gnome/evince/Daemon" "org.freedesktop.DBus.Introspectable" "Introspect")
  byte-code("\b\203\b.\305\202	.\306	\n\v\f\307%\207" [noninteractive bus service path dbus-interface-introspectable dbus-call-method dbus-call-method-non-blocking "Introspect"] 6)
  dbus-introspect(:session "org.gnome.evince.Daemon" "/org/gnome/evince/Daemon")
  byte-code("\304\305!.r\bq\210\306\216\307	\n\v#c\210\310ed\"+\207" [temp-buffer bus service path generate-new-buffer " *temp*" ((byte-code "\301\b!\203\n.\302\b!\210\301\207" [temp-buffer buffer-name kill-buffer] 2)) dbus-introspect xml-parse-region] 4)
  dbus-introspect-xml(:session "org.gnome.evince.Daemon" "/org/gnome/evince/Daemon")
  dbus-introspect-get-interface(:session "org.gnome.evince.Daemon" "/org/gnome/evince/Daemon" "org.gnome.evince.Daemon")
  dbus-introspect-get-method(:session "org.gnome.evince.Daemon" "/org/gnome/evince/Daemon" "org.gnome.evince.Daemon" "FindDocument")
  TeX-evince-dbus-p(:forward)
  byte-code("\b\301=\203\b.\302\207\303\304\305\306\307\310\311!\203.\0\312\202+.\313\314\315\316\317\320!\"\203&.\321\202'.\322D\323BBD\324BBBBB\207" [system-type windows-nt (("Yap" ("yap -1" (mode-io-correlate " -s %n%b") " %o")) ("dvips and start" "dvips %d -o && start \"\" %f") ("start" "start \"\" %o")) ("xdvi" ("%(o?)xdvi" (mode-io-correlate " -sourceposition \"%n %b\" -editor \"%cS\"") ((paper-a4 paper-portrait) " -paper a4") ((paper-a4 paper-landscape) " -paper a4r") ((paper-a5 paper-portrait) " -paper a5") ((paper-a5 paper-landscape) " -paper a5r") (paper-b5 " -paper b5") (paper-letter " -paper us") (paper-legal " -paper legal") (paper-executive " -paper 7.25x10.5in") " %d")) ("dvips and gv" "%(o?)dvips %d -o && gv %f") ("gv" "gv %o") ("xpdf" ("xpdf -remote %s -raise %o" (mode-io-correlate " %(outpage)"))) "Evince" TeX-evince-dbus-p :forward TeX-evince-sync-view "evince" mode-io-correlate string-match "--page-index" shell-command-to-string "evince --help" " -i %(outpage)" " -p %(outpage)" (" %o") (("Okular" ("okular --unique %o" (mode-io-correlate "#src:%n%b"))) ("xdg-open" "xdg-open %o"))] 11)
  (defvar TeX-view-program-list-builtin (byte-code "\b\301=\203\b.\302\207\303\304\305\306\307\310\311!\203.\0\312\202+.\313\314\315\316\317\320!\"\203&.\321\202'.\322D\323BBD\324BBBBB\207" [system-type windows-nt (("Yap" ("yap -1" (mode-io-correlate " -s %n%b") " %o")) ("dvips and start" "dvips %d -o && start \"\" %f") ("start" "start \"\" %o")) ("xdvi" ("%(o?)xdvi" (mode-io-correlate " -sourceposition \"%n %b\" -editor \"%cS\"") ((paper-a4 paper-portrait) " -paper a4") ((paper-a4 paper-landscape) " -paper a4r") ((paper-a5 paper-portrait) " -paper a5") ((paper-a5 paper-landscape) " -paper a5r") (paper-b5 " -paper b5") (paper-letter " -paper us") (paper-legal " -paper legal") (paper-executive " -paper 7.25x10.5in") " %d")) ("dvips and gv" "%(o?)dvips %d -o && gv %f") ("gv" "gv %o") ("xpdf" ("xpdf -remote %s -raise %o" (mode-io-correlate " %(outpage)"))) "Evince" TeX-evince-dbus-p :forward TeX-evince-sync-view "evince" mode-io-correlate string-match "--page-index" shell-command-to-string "evince --help" " -i %(outpage)" " -p %(outpage)" (" %o") (("Okular" ("okular --unique %o" (mode-io-correlate "#src:%n%b"))) ("xdg-open" "xdg-open %o"))] 11) ("/usr/share/emacs24/site-lisp/auctex/tex.elc" . 30407))
  require(tex)
  byte-code("\300\301!\210\300\302!\207" [require tex tex-style] 2)
  BibTeX-auto-store()
  run-hooks(change-major-mode-after-body-hook bibtex-mode-hook)
  apply(run-hooks (change-major-mode-after-body-hook bibtex-mode-hook))
  run-mode-hooks(bibtex-mode-hook)
  bibtex-mode()
=============================================================================
-- 
Camm Maguire			     		    camm@maguirefamily.org
==========================================================================
"The earth is but one country, and mankind its citizens."  --  Baha'u'llah

Re: emacs24/auctex bug

[Xue Fuqiao]

On Mon, 14 Jan 2013 11:52:55 -0500
Camm Maguire <camm@maguirefamily.org> wrote:

> Greetings!  When I try to open a .bib file in emacs24 with auctex
> loaded, I hang with this backtrace.

You can post it in:
auctex@gnu.org
bug-auctex@gnu.org
help-gnu-emacs@gnu.org
-- 
Best regards, Xue Fuqiao.
http://www.emacswiki.org/emacs/XueFuqiao

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Stefan Monnier]

> ! 	       (add-hook 'after-change-major-mode-hook
> ! 			 ',MODE-enable-in-buffers)
> ! 	       (add-hook 'change-major-mode-after-body-hook
>   			 ',MODE-enable-in-buffers)
>   	       (add-hook 'find-file-hook ',MODE-check-buffers)
>   	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
> --- 399,407 ----
>   	 ;; Setup hook to handle future mode changes and new buffers.
>   	 (if ,global-mode
>   	     (progn
> ! 	       (add-hook ',(if call-before-hook
> ! 			       'change-major-mode-after-body-hook
> ! 			     'after-change-major-mode-hook)

I think we first need to understand why we currently use both.
Also, the MODE-enable-in-buffers function should enable the mode only
once, normally, even if called via both hooks.

So if you see a problem, maybe it's because (eq ,MODE-major-mode
major-mode) is not true the second time around, i.e. because
`major-mode' has changed between the two hooks.  Is that the case?
If so, why?


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Alan Mackenzie]

Hi, Stefan.

On Mon, Jan 14, 2013 at 09:12:15PM -0500, Stefan Monnier wrote:
> > ! 	       (add-hook 'after-change-major-mode-hook
> > ! 			 ',MODE-enable-in-buffers)
> > ! 	       (add-hook 'change-major-mode-after-body-hook
> >   			 ',MODE-enable-in-buffers)
> >   	       (add-hook 'find-file-hook ',MODE-check-buffers)
> >   	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
> > --- 399,407 ----
> >   	 ;; Setup hook to handle future mode changes and new buffers.
> >   	 (if ,global-mode
> >   	     (progn
> > ! 	       (add-hook ',(if call-before-hook
> > ! 			       'change-major-mode-after-body-hook
> > ! 			     'after-change-major-mode-hook)

> I think we first need to understand why we currently use both.

This is difficult.  The change was in revision 106202.  Previously,
MODE-enable-in-buffers was simply add-hooked into
after-change-major-mode-hook.  The relevant ChangeLog entry runs:

+ 2011-10-27  Chong Yidong  <cyd@gnu.org>
+
+       * subr.el (change-major-mode-after-body-hook): New hook.
+       (run-mode-hooks): Run it.
+
+       * emacs-lisp/easy-mmode.el (define-globalized-minor-mode): Use
+       change-major-mode-before-body-hook.
+
+       * simple.el (fundamental-mode):
+       * emacs-lisp/derived.el (define-derived-mode): Revert 2010-04-28
+       change introducing fundamental-mode-hook.
+

The new hook change-major-mode-after-body-hook came into existence after
a naming competition in the thread starting at:

From: Christoph Scholtes <cschol2112@googlemail.com>
To: emacs-devel@gnu.org
Subject: Fundamental mode vs. special mode
Date: Sat, 22 Oct 2011 14:13:03 -0600
Message-ID: <86sjmkvl80.fsf@googlemail.com>

That thread mainly discussed the propriety of fundamental-mode-hook, and
prompted Yidong to remove it, putting change-major-mode-after-body-hook 
in its place.  Here is his email proposing this:
> I propose to remove the fundamental-mode-hook variable introduced in
> that change, and introduce `run-mode-hooks-hook'[*], which
> `run-mode-hooks' runs before all the other hooks.  Then minor modes
> can use `run-mode-hooks-hook', and the "turning off global modes"
> feature will be available to non-derived modes.
[*] renamed to `change-major-mode-after-body-hook' before
implementation.

> Also, the MODE-enable-in-buffers function should enable the mode only
> once, normally, even if called via both hooks.

I think that's true.  My problem is that font-lock-mode is being called
too soon, before the mode has been fully set up by the major mode hook.
That first call of f-l-m sets up font-lock-keywords to include entries
for fontifying "doc comments".  A buffer local variable specifies what
sort of doc comments.

It has to be admitted that the CC Mode code here isn't totally correct;
a complete solution to this bug probably involves making
font-lock-keywords buffer local when doc comments are specified.  I
don't want to make this change (which would be somewhat involved) before
Emacs 24.3.  ;-(  Postponing `font-lock-mode' until after the mode hook
seems a safer bet at the moment.

> So if you see a problem, maybe it's because (eq ,MODE-major-mode
> major-mode) is not true the second time around, i.e. because
> `major-mode' has changed between the two hooks.  Is that the case?
> If so, why?


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[João Távora]

On Tue, Jan 15, 2013 at 2:08 PM, Alan Mackenzie <acm@muc.de> wrote:

> I think that's true.  My problem is that font-lock-mode is being called
> too soon, before the mode has been fully set up by the major mode hook.

Sorry to hijack, but I have a similiar problem in autopair and yasnippet. My
problem is that I sometimes want the minor modes never to be called at all
depending on the major-mode setup.

While the major-mode hook has a chance to disable them with (xxx-mode -1),
there's no way to use the major-mode's hook to prevent a globalized minor mode
from ever being activated in the buffer.

This is a inneficient and cumbersome for these cases:

* autopair-minor-mode crashes the system when activated with sldb-minor-mode, I
  use autopair-mode-on for this and hardcode the sldb reference there. It also
  does a lot of useless setup of keybindings etc...

* yas-minor-mode jit-loads snippets for the major mode when invoked. If its
  turned off immediately after, it loaded them too soon.

before emacs 24, I used to use xxx-dont-turn-on variables set in major mode hook
and read in the xxx-mode-on function, but I agree that isn't particularly pretty
too.

Thanks,
--
João Távora

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Alan Mackenzie]

Hi, João.

On Thu, Jan 17, 2013 at 01:17:03PM +0000, João Távora wrote:
> On Tue, Jan 15, 2013 at 2:08 PM, Alan Mackenzie <acm@muc.de> wrote:

> > I think that's true.  My problem is that font-lock-mode is being called
> > too soon, before the mode has been fully set up by the major mode hook.

> Sorry to hijack, but I have a similiar problem in autopair and yasnippet. My
> problem is that I sometimes want the minor modes never to be called at all
> depending on the major-mode setup.

OK, so I'm not the only person with this problem.  :-)

> While the major-mode hook has a chance to disable them with (xxx-mode -1),
> there's no way to use the major-mode's hook to prevent a globalized minor mode
> from ever being activated in the buffer.

> This is a inneficient and cumbersome for these cases:

> * autopair-minor-mode crashes the system when activated with sldb-minor-mode, I
>   use autopair-mode-on for this and hardcode the sldb reference there. It also
>   does a lot of useless setup of keybindings etc...

> * yas-minor-mode jit-loads snippets for the major mode when invoked. If its
>   turned off immediately after, it loaded them too soon.

> before emacs 24, I used to use xxx-dont-turn-on variables set in major mode hook
> and read in the xxx-mode-on function, but I agree that isn't particularly pretty
> too.

Maybe making a buffer local copy of global-xxxx-mode would do the trick,
but that's anything but pretty.

I'm still trying to come up with a reason somebody might want to turn on
the minor mode _before_ the major mode's hook has been run.

> Thanks,
> --
> João Távora

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Stefan Monnier]

> I'm still trying to come up with a reason somebody might want to turn on
> the minor mode _before_ the major mode's hook has been run.

The issue is how to locally turn off the globalized minor mode from
a major-mode hook.  If the globalized mode is enabled after the
major-mode hook, you basically can't do it without resorting to
ugly gymnastics.

I'm not completely sure what happens (and what should happen instead) in
your scenario, but for the case described by João, where you don't just
want to disable it in some modes, but you want to prevent it from ever
being enabled in such modes, obviously the current approach
fails similarly.

A simple solution is to activate the minor mode late, and provide
a `disable-foo-mode' variable, that can be set in the major mode hook.
But it's kind of ugly because it requires another var and the user needs
to know about that magic var.

Maybe we could do it this way:

for a globalized minor mode `foo', before running the major-mode hook, a magic
`disable-foo-mode' is set to nil.  A function is added to `foo-mode-hook'
which sets this var to t when foo-mode is disabled.  And after running
the major-mode hook, the globalized minor mode code checks the magic var
to decide whether to enable the minor mode.

This way, the user doesn't need to know about the extra magic var; she
can just call (foo-mode -1) in her major-mode hook as she can now, but
the minor-mode is really only activated late.

It's also kind of ugly because of all the magic, but it's the best I can
come up with so far.


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[João Távora]

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

On Thu, Jan 17, 2013 at 6:31 PM, Stefan Monnier <monnier@iro.umontreal.ca>
wrote:
> It's also kind of ugly because of all the magic, but it's the best I can
> come up with so far.

I think the approach is quite good (was actually thinking along those
lines on my way to work this morning). It's not uglier than the existing

(defvar ,MODE-major-mode nil)
(make-variable-buffer-local ',MODE-major-mode)

I wonder if the existing MODE-major-mode and the disable-MODE you
proposed could somehow take advantage of lexical binding so they
wouldn't be visible to the user, but maybe this makes no sense...
reading that section of easy-mmode.el always makes me delirious :-)

Also couldn't ,MODE-major-mode contain both semantics and be renamed or
something?

--
João Távora

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

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch]

[Alan Mackenzie]

Hello, Stefan.

On Thu, Jan 17, 2013 at 01:31:32PM -0500, Stefan Monnier wrote:
> > I'm still trying to come up with a reason somebody might want to turn on
> > the minor mode _before_ the major mode's hook has been run.

> The issue is how to locally turn off the globalized minor mode from
> a major-mode hook.  If the globalized mode is enabled after the
> major-mode hook, you basically can't do it without resorting to
> ugly gymnastics.

Ah, OK.

> I'm not completely sure what happens (and what should happen instead) in
> your scenario, but for the case described by João, where you don't just
> want to disable it in some modes, but you want to prevent it from ever
> being enabled in such modes, obviously the current approach
> fails similarly.

> A simple solution is to activate the minor mode late, and provide
> a `disable-foo-mode' variable, that can be set in the major mode hook.
> But it's kind of ugly because it requires another var and the user needs
> to know about that magic var.

Not enabling a globally-enabled mode in particular buffers is essentially
ugly, so whatever solution we come up with will be ugly too.

> Maybe we could do it this way:

> for a globalized minor mode `foo', before running the major-mode hook, a magic
> `disable-foo-mode' is set to nil.  A function is added to `foo-mode-hook'
> which sets this var to t when foo-mode is disabled.  And after running
> the major-mode hook, the globalized minor mode code checks the magic var
> to decide whether to enable the minor mode.

!!!!!!!!!!!!!!!

> This way, the user doesn't need to know about the extra magic var; she
> can just call (foo-mode -1) in her major-mode hook as she can now, but
> the minor-mode is really only activated late.

> It's also kind of ugly because of all the magic, but it's the best I can
> come up with so far.

I can't come up with anything better at the moment, either, but I can't
help feeling that there's something more fundamental about global minor
modes that we're missing.

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Alan Mackenzie]

Hi, Stefan.

On Thu, Jan 17, 2013 at 01:31:32PM -0500, Stefan Monnier wrote:
[Note: this discussion is relevant to bug#11152.]

> The issue is how to locally turn off the globalized minor mode from
> a major-mode hook.  If the globalized mode is enabled after the
> major-mode hook, you basically can't do it without resorting to
> ugly gymnastics.

> I'm not completely sure what happens (and what should happen instead) in
> your scenario, but for the case described by João, where you don't just
> want to disable it in some modes, but you want to prevent it from ever
> being enabled in such modes, obviously the current approach
> fails similarly.

> A simple solution is to activate the minor mode late, and provide
> a `disable-foo-mode' variable, that can be set in the major mode hook.
> But it's kind of ugly because it requires another var and the user needs
> to know about that magic var.

> Maybe we could do it this way:

> for a globalized minor mode `foo', before running the major-mode hook, a magic
> `disable-foo-mode' is set to nil.  A function is added to `foo-mode-hook'
> which sets this var to t when foo-mode is disabled.  And after running
> the major-mode hook, the globalized minor mode code checks the magic var
> to decide whether to enable the minor mode.

OK, I've bitten the bullet.  Please see my patch below, which implements
this precisely.  It seems to work, in that:
(i) It fixes #11152 - the new global-font-lock-mode calls font-lock-mode
  _after_ the major mode hook.
(ii) Should the major mode hook do (font-lock-mode -1), this isn't
  overridden by global mode.

> This way, the user doesn't need to know about the extra magic var; she
> can just call (foo-mode -1) in her major-mode hook as she can now, but
> the minor-mode is really only activated late.

> It's also kind of ugly because of all the magic, but it's the best I can
> come up with so far.

Here's the patch, based on easy-mmode.el in the emacs-24 branch:



=== modified file 'lisp/emacs-lisp/easy-mmode.el'
*** lisp/emacs-lisp/easy-mmode.el	2013-01-01 09:11:05 +0000
--- lisp/emacs-lisp/easy-mmode.el	2013-01-31 10:13:51 +0000
***************
*** 340,348 ****
  enabled, then disabling and reenabling MODE should make MODE work
  correctly with the current major mode.  This is important to
  prevent problems with derived modes, that is, major modes that
! call another major mode in their body."
    (declare (doc-string 2))
    (let* ((global-mode-name (symbol-name global-mode))
  	 (pretty-name (easy-mmode-pretty-mode-name mode))
  	 (pretty-global-name (easy-mmode-pretty-mode-name global-mode))
  	 (group nil)
--- 340,353 ----
  enabled, then disabling and reenabling MODE should make MODE work
  correctly with the current major mode.  This is important to
  prevent problems with derived modes, that is, major modes that
! call another major mode in their body.
! 
! MODE is actually turned on when a major mode is initialized, just
! after running the major mode's hook.  However, MODE is not turned
! on, should the hook explicitly disable it."
    (declare (doc-string 2))
    (let* ((global-mode-name (symbol-name global-mode))
+ 	 (mode-name (symbol-name mode))
  	 (pretty-name (easy-mmode-pretty-mode-name mode))
  	 (pretty-global-name (easy-mmode-pretty-mode-name global-mode))
  	 (group nil)
***************
*** 353,358 ****
--- 358,369 ----
  	 (MODE-check-buffers
  	  (intern (concat global-mode-name "-check-buffers")))
  	 (MODE-cmhh (intern (concat global-mode-name "-cmhh")))
+ 	 (MODE-cancel-disable
+ 	  (intern (concat global-mode-name "-cancel-disable")))
+ 	 (MODE-disable-in-buffer
+ 	  (intern (concat global-mode-name "-disable-in-buffer")))
+ 	 (minor-MODE-hook (intern (concat mode-name "-hook")))
+ 	 (disable-MODE (intern (concat "disable-" mode-name)))
  	 (MODE-major-mode (intern (concat (symbol-name mode) "-major-mode")))
  	 keyw)
  
***************
*** 397,403 ****
  	       (add-hook 'after-change-major-mode-hook
  			 ',MODE-enable-in-buffers)
  	       (add-hook 'change-major-mode-after-body-hook
! 			 ',MODE-enable-in-buffers)
  	       (add-hook 'find-file-hook ',MODE-check-buffers)
  	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
  	   (remove-hook 'after-change-major-mode-hook ',MODE-enable-in-buffers)
--- 408,414 ----
  	       (add-hook 'after-change-major-mode-hook
  			 ',MODE-enable-in-buffers)
  	       (add-hook 'change-major-mode-after-body-hook
! 			 ',MODE-cancel-disable)
  	       (add-hook 'find-file-hook ',MODE-check-buffers)
  	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
  	   (remove-hook 'after-change-major-mode-hook ',MODE-enable-in-buffers)
***************
*** 415,420 ****
--- 426,435 ----
         ;; up-to-here.
         :autoload-end
  
+        ;; A function which checks whether MODE has been disabled in the major
+        ;; mode hook which has just been run.
+        (add-hook ',minor-MODE-hook ',MODE-disable-in-buffer)
+ 
         ;; List of buffers left to process.
         (defvar ,MODE-buffers nil)
  
***************
*** 423,436 ****
  	 (dolist (buf ,MODE-buffers)
  	   (when (buffer-live-p buf)
  	     (with-current-buffer buf
!                (unless (eq ,MODE-major-mode major-mode)
!                  (if ,mode
!                      (progn
!                        (,mode -1)
!                        (,turn-on)
!                        (setq ,MODE-major-mode major-mode))
!                    (,turn-on)
!                    (setq ,MODE-major-mode major-mode)))))))
         (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
  
         (defun ,MODE-check-buffers ()
--- 438,452 ----
  	 (dolist (buf ,MODE-buffers)
  	   (when (buffer-live-p buf)
  	     (with-current-buffer buf
!                (if ,disable-MODE
! 		   (if ,mode (,mode -1))
! 		 (unless (eq ,MODE-major-mode major-mode)
! 		   (if ,mode
! 		       (progn
! 			 (,mode -1)
! 			 (,turn-on))
! 		     (,turn-on))))
! 	       (setq ,MODE-major-mode major-mode)))))
         (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
  
         (defun ,MODE-check-buffers ()
***************
*** 443,449 ****
         (defun ,MODE-cmhh ()
  	 (add-to-list ',MODE-buffers (current-buffer))
  	 (add-hook 'post-command-hook ',MODE-check-buffers))
!        (put ',MODE-cmhh 'definition-name ',global-mode))))
  
  ;;;
  ;;; easy-mmode-defmap
--- 459,473 ----
         (defun ,MODE-cmhh ()
  	 (add-to-list ',MODE-buffers (current-buffer))
  	 (add-hook 'post-command-hook ',MODE-check-buffers))
!        (put ',MODE-cmhh 'definition-name ',global-mode)
!        (defvar ,disable-MODE nil)
!        (defun ,MODE-cancel-disable ()
! 	 (setq ,disable-MODE nil))
!        (put ',MODE-cancel-disable 'definition-name ',global-mode)
!        (defun ,MODE-disable-in-buffer ()
! 	 (unless ,mode
! 	   (setq ,disable-MODE t)))
!        (put ',MODE-disable-in-buffer 'definition-name ',global-mode))))
  
  ;;;
  ;;; easy-mmode-defmap


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Stefan Monnier]

> + 	 (MODE-cancel-disable
> + 	  (intern (concat global-mode-name "-cancel-disable")))
> + 	 (MODE-disable-in-buffer
> + 	  (intern (concat global-mode-name "-disable-in-buffer")))

If you defvar-local the MODE-disable-in-buffer, then you shouldn't need
MODE-cancel-disable since kill-all-local-variables will have reset
MODE-disable-in-buffer to nil already.


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Alan Mackenzie]

Hi, Stefan.

On Thu, Jan 31, 2013 at 09:38:31AM -0500, Stefan Monnier wrote:
> > + 	 (MODE-cancel-disable
> > + 	  (intern (concat global-mode-name "-cancel-disable")))
> > + 	 (MODE-disable-in-buffer
> > + 	  (intern (concat global-mode-name "-disable-in-buffer")))

> If you defvar-local the MODE-disable-in-buffer, .....

That would be disable-MODE I think you meant.  Yes, I've now made
disable-MODE buffer local.  It has to be buffer local, considering the
way that MODE-enable-in-buffers checks all buffers each time it runs.

> .... then you shouldn't need MODE-cancel-disable since
> kill-all-local-variables will have reset MODE-disable-in-buffer to nil
> already.

I'm not so sure about this.  These complicated structures of macros and
hooks and generated functions are making my head hurt.  ;-(  Are you
sure there are no ways of invoking this which won't bypass
kill-all-local-variables?  (That's a real question, not a rhetorical
one.)

Another thought.  As I've coded it up, the disable-MODE flag, once it
becomes t, stays t (upto the next major mode change).  However, running
global-MODE still enables MODE on this buffer.  Should we worry about
this?  I don't think we need to.

[Amended patch not included.]

>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Stefan Monnier]

> I'm not so sure about this.  These complicated structures of macros and
> hooks and generated functions are making my head hurt.  ;-(  Are you
> sure there are no ways of invoking this which won't bypass
> kill-all-local-variables?  (That's a real question, not a rhetorical
> one.)

Any major mode will have to call kill-all-local-variables, so it's
pretty sure.


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Alan Mackenzie]

Hi, Stefan.

On Fri, Feb 01, 2013 at 11:28:00AM -0500, Stefan Monnier wrote:
> > I'm not so sure about this.  These complicated structures of macros and
> > hooks and generated functions are making my head hurt.  ;-(  Are you
> > sure there are no ways of invoking this which won't bypass
> > kill-all-local-variables?  (That's a real question, not a rhetorical
> > one.)

> Any major mode will have to call kill-all-local-variables, so it's
> pretty sure.

OK, I see it now.  I've removed that function, replacing it with a
comment about kill-all-local-variables attending to disable-MODE.
I've also tidied up the new bit of doc-string I added, 

Is it OK to commit this change to emacs-24 now, and close bug#11152?

Here's the patch:



=== modified file 'lisp/emacs-lisp/easy-mmode.el'
*** lisp/emacs-lisp/easy-mmode.el	2013-01-01 09:11:05 +0000
--- lisp/emacs-lisp/easy-mmode.el	2013-02-01 19:13:42 +0000
***************
*** 340,348 ****
  enabled, then disabling and reenabling MODE should make MODE work
  correctly with the current major mode.  This is important to
  prevent problems with derived modes, that is, major modes that
! call another major mode in their body."
    (declare (doc-string 2))
    (let* ((global-mode-name (symbol-name global-mode))
  	 (pretty-name (easy-mmode-pretty-mode-name mode))
  	 (pretty-global-name (easy-mmode-pretty-mode-name global-mode))
  	 (group nil)
--- 340,353 ----
  enabled, then disabling and reenabling MODE should make MODE work
  correctly with the current major mode.  This is important to
  prevent problems with derived modes, that is, major modes that
! call another major mode in their body.
! 
! When a major mode is initialized, MODE is actually turned on just
! after running the major mode's hook.  However, MODE is not turned
! on if the hook has expllicitly disabled it."
    (declare (doc-string 2))
    (let* ((global-mode-name (symbol-name global-mode))
+ 	 (mode-name (symbol-name mode))
  	 (pretty-name (easy-mmode-pretty-mode-name mode))
  	 (pretty-global-name (easy-mmode-pretty-mode-name global-mode))
  	 (group nil)
***************
*** 353,358 ****
--- 358,367 ----
  	 (MODE-check-buffers
  	  (intern (concat global-mode-name "-check-buffers")))
  	 (MODE-cmhh (intern (concat global-mode-name "-cmhh")))
+ 	 (MODE-disable-in-buffer
+ 	  (intern (concat global-mode-name "-disable-in-buffer")))
+ 	 (minor-MODE-hook (intern (concat mode-name "-hook")))
+ 	 (disable-MODE (intern (concat "disable-" mode-name)))
  	 (MODE-major-mode (intern (concat (symbol-name mode) "-major-mode")))
  	 keyw)
  
***************
*** 396,403 ****
  	     (progn
  	       (add-hook 'after-change-major-mode-hook
  			 ',MODE-enable-in-buffers)
- 	       (add-hook 'change-major-mode-after-body-hook
- 			 ',MODE-enable-in-buffers)
  	       (add-hook 'find-file-hook ',MODE-check-buffers)
  	       (add-hook 'change-major-mode-hook ',MODE-cmhh))
  	   (remove-hook 'after-change-major-mode-hook ',MODE-enable-in-buffers)
--- 405,410 ----
***************
*** 415,420 ****
--- 422,431 ----
         ;; up-to-here.
         :autoload-end
  
+        ;; A function which checks whether MODE has been disabled in the major
+        ;; mode hook which has just been run.
+        (add-hook ',minor-MODE-hook ',MODE-disable-in-buffer)
+ 
         ;; List of buffers left to process.
         (defvar ,MODE-buffers nil)
  
***************
*** 423,436 ****
  	 (dolist (buf ,MODE-buffers)
  	   (when (buffer-live-p buf)
  	     (with-current-buffer buf
!                (unless (eq ,MODE-major-mode major-mode)
!                  (if ,mode
!                      (progn
!                        (,mode -1)
!                        (,turn-on)
!                        (setq ,MODE-major-mode major-mode))
!                    (,turn-on)
!                    (setq ,MODE-major-mode major-mode)))))))
         (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
  
         (defun ,MODE-check-buffers ()
--- 434,448 ----
  	 (dolist (buf ,MODE-buffers)
  	   (when (buffer-live-p buf)
  	     (with-current-buffer buf
!                (if ,disable-MODE
! 		   (if ,mode (,mode -1))
! 		 (unless (eq ,MODE-major-mode major-mode)
! 		   (if ,mode
! 		       (progn
! 			 (,mode -1)
! 			 (,turn-on))
! 		     (,turn-on))))
! 	       (setq ,MODE-major-mode major-mode)))))
         (put ',MODE-enable-in-buffers 'definition-name ',global-mode)
  
         (defun ,MODE-check-buffers ()
***************
*** 443,449 ****
         (defun ,MODE-cmhh ()
  	 (add-to-list ',MODE-buffers (current-buffer))
  	 (add-hook 'post-command-hook ',MODE-check-buffers))
!        (put ',MODE-cmhh 'definition-name ',global-mode))))
  
  ;;;
  ;;; easy-mmode-defmap
--- 455,468 ----
         (defun ,MODE-cmhh ()
  	 (add-to-list ',MODE-buffers (current-buffer))
  	 (add-hook 'post-command-hook ',MODE-check-buffers))
!        (put ',MODE-cmhh 'definition-name ',global-mode)
!        ;; disable-MODE is set in MODE-disable-in-buffer and cleared by
!        ;; kill-all-local-variables.
!        (defvar-local ,disable-MODE nil)
!        (defun ,MODE-disable-in-buffer ()
! 	 (unless ,mode
! 	   (setq ,disable-MODE t)))
!        (put ',MODE-disable-in-buffer 'definition-name ',global-mode))))
  
  ;;;
  ;;; easy-mmode-defmap



>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Achim Gratz]

Alan Mackenzie writes:
> ! on if the hook has expllicitly disabled it."

Should be "explicitly".


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf Q+, Q and microQ:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Alan Mackenzie]

Hello, Achim.

On Fri, Feb 01, 2013 at 09:09:34PM +0100, Achim Gratz wrote:
> Alan Mackenzie writes:
> > ! on if the hook has expllicitly disabled it."

> Should be "explicitly".

Yes indeed!  Now corrected.  Thanks!

> Regards,
> Achim.

-- 
Alan Mackenzie (Nuremberg, Germany).

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Stefan Monnier]

> Is it OK to commit this change to emacs-24 now, and close bug#11152?

No, I think this is good for trunk but not for emacs-24.
I think that for emacs-24 we need a less invasive change.

For that we should probably go back and try and figure out why the hook
is run both times.  As mentioned, the code tries to avoid running it
twice by checking the value of `major-mode', so it appears that in the
problem case, `major-mode' changes between the two hooks.  Can you try
and figure out if that's indeed the case and why?


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Stefan Monnier]

>> ! on if the hook has expllicitly disabled it."
> Should be "explicitly".

You mean "exppllicittly"?


        Stefan

Re: Redundant (harmful) duplication of run-hooks in define-globalized-minor-mode [patch-2]

[Alan Mackenzie]

Hi, Stefan.

On Fri, Feb 01, 2013 at 06:16:37PM -0500, Stefan Monnier wrote:
> > Is it OK to commit this change to emacs-24 now, and close bug#11152?

> No, I think this is good for trunk but not for emacs-24.
> I think that for emacs-24 we need a less invasive change.

Any less invasive change would surely be a better change, if we could
come up with one, so would also be better for the trunk.  Maybe we just
leave this bug to 24.4/25.1 to fix.

> For that we should probably go back and try and figure out why the hook
> is run both times.  As mentioned, the code tries to avoid running it
> twice by checking the value of `major-mode', so it appears that in the
> problem case, `major-mode' changes between the two hooks.  Can you try
> and figure out if that's indeed the case and why?

The MODE-major-mode check was in place long before the current issues
arose in 2010-04-25 in the emacs-devel thread "globalized minor modes -
priority over mode hook?".  I've bzr blame'd it back to before revision
49780.1.32 from 2006.

MODE-major-mode's original purpose seems to be to prevent the needless
disabling and re-enabling of the minor mode.  It doesn't appear to be
there to detect a mode changing between two run-hook calls.  I haven't
found any evidence of any such "problem case".

The first revision where MODE-enable-in-buffers gets invoked twice is
100071, this one:

    revno: 100071
    committer: Stefan Monnier <monnier@iro.umontreal.ca>
    branch nick: trunk
    timestamp: Wed 2010-04-28 11:18:37 -0400
    message:
      Make it possible to locally disable a globally enabled mode.
      * simple.el (fundamental-mode): Run fundamental-mode-hook.
      * emacs-lisp/derived.el (define-derived-mode): Use fundamental-mode
      rather than kill-all-local-variables so it runs fundamental-mode-hook.
      * emacs-lisp/easy-mmode.el (define-globalized-minor-mode):
      Use fundamental-mode-hook to run MODE-enable-in-buffers earlier, so
      that subsequent hooks get a chance to disable it.

, with this change:

    *** lisp/emacs-lisp/easy-mmode.el       2010-04-27 18:14:16 +0000
    --- lisp/emacs-lisp/easy-mmode.el       2010-04-28 15:18:37 +0000
    ***************
    *** 338,346 ****
    --- 338,348 ----
                 (progn
                   (add-hook 'after-change-major-mode-hook
                             ',MODE-enable-in-buffers)
    +              (add-hook 'fundamental-mode-hook ',MODE-enable-in-buffers)
                   (add-hook 'find-file-hook ',MODE-check-buffers)
                   (add-hook 'change-major-mode-hook ',MODE-cmhh))
               (remove-hook 'after-change-major-mode-hook ',MODE-enable-in-buffers)
    +            (remove-hook 'fundamental-mode-hook ',MODE-enable-in-buffers)
               (remove-hook 'find-file-hook ',MODE-check-buffers)
               (remove-hook 'change-major-mode-hook ',MODE-cmhh))

.  fundamental-mode-hook was later suppressed and replaced by
change-major-mode-after-body-hook.

Stefan, have you any recollection at all of why you left
MODE-enable-in-buffers also in after-change-major-mode-hook?


>         Stefan

-- 
Alan Mackenzie (Nuremberg, Germany).