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

[Alan Mackenzie <acm@muc.de>]

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