bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Josh]

I noticed two things that show that partial highlight-regexp state
lingers after calling revert-buffer, see items 9 and 12.  After buffer
reversion, I would expect this state either to be completely backed
out or to survive completely intact.  Ideally both of these behaviors
would be supported and under users' control.

 (1) emacs -Q foo.txt
 (2) insert `foo bar' into buffer and save
 (3) M-s h r foo RET RET
 (4) note that foo is highlighted with hi-yellow face
 (5) make a trivial edit to make buffer-modified-p be true
 (6) M-x revert-buffer and confirm
 (7) note that highlighting goes away as expected
 (8) M-s h r bar RET but do not respond to the prompt for the face
 (9) Expected default face: hi-yellow.  Actual: hi-pink.
(10) C-g to cancel the highlight-regexp command
(11) M-s h u but do not respond to the prompt for a regexp
(12) Expected: "No highlighting to remove".  Actual: prompts for
regexp with a default of foo.
(13) Hit RET to accept the default regexp foo to unhighlight
(14) M-s h u
(15) note that "No highlighting to remove" appears, as should have
happened after (11)

In GNU Emacs 24.3.50.1 (x86_64-apple-darwin, NS apple-appkit-1187.34)
 of 2013-03-04 on bix
Bzr revision: 111935 yamaoka@jpl.org-20130304102733-4qy111z41qwoh2as
Windowing system distributor `Apple', version 10.3.1187
Configured using:
 `configure --host=x86_64-apple-darwin --build=i686-apple-darwin
 --with-ns'

Important settings:
  value of $LC_CTYPE: en_US.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix
  default enable-multibyte-characters: t

Major mode: Text

Minor modes in effect:
  tooltip-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
f o o SPC f a <backspace> <backspace> b a r C-x C-s
M-s h r f o o <return> <return> SPC M-x r e v e r t
- b u f f <tab> <return> y e s <return> M-s h r b a
r <return> C-g M-s h u <return> M-s h u M-x e <backspace>
r e p o r t - e m a c s - b u f <tab> <return> <backspace>
<return> h i g h l i g h t - r e g e x p SPC i n c
o r r e c t l y SPC r e t a i n s SPC s t a t e SPC
a f t e r SPC b u f f e r SPC r e v e r s i o n C-a
C-k C-g M-x r e p o r t - e m a c s - b u g <retur
n>

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
foo.txt has auto save data; consider M-x recover-this-file
Saving file /Users/jlf/tmp/foo.txt...
Wrote /Users/jlf/tmp/foo.txt
You can run the command `revert-buffer' with s-u
Quit
byte-code: No highlighting to remove
Quit

Load-path shadows:
None found.

Features:
(shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils hi-lock time-date tooltip ediff-hook
vc-hooks lisp-float-type mwheel ns-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list newcomment lisp-mode register page
menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote make-network-process ns multi-tty emacs)

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Jambunathan K]

I can circulate a patch for some or all of what you report below. Are
you willing to apply the patch and give immediate feedback?  Please
confirm with Yes or No.

Josh <josh@foxtail.org> writes:

> I noticed two things that show that partial highlight-regexp state
> lingers after calling revert-buffer, see items 9 and 12.  After buffer
> reversion, I would expect this state either to be completely backed
> out or to survive completely intact.  Ideally both of these behaviors
> would be supported and under users' control.
>
>  (1) emacs -Q foo.txt
>  (2) insert `foo bar' into buffer and save
>  (3) M-s h r foo RET RET
>  (4) note that foo is highlighted with hi-yellow face
>  (5) make a trivial edit to make buffer-modified-p be true
>  (6) M-x revert-buffer and confirm
>  (7) note that highlighting goes away as expected
>  (8) M-s h r bar RET but do not respond to the prompt for the face
>  (9) Expected default face: hi-yellow.  Actual: hi-pink.
> (10) C-g to cancel the highlight-regexp command
> (11) M-s h u but do not respond to the prompt for a regexp
> (12) Expected: "No highlighting to remove".  Actual: prompts for
> regexp with a default of foo.
> (13) Hit RET to accept the default regexp foo to unhighlight
> (14) M-s h u
> (15) note that "No highlighting to remove" appears, as should have
> happened after (11)
>
> In GNU Emacs 24.3.50.1 (x86_64-apple-darwin, NS apple-appkit-1187.34)
>  of 2013-03-04 on bix
> Bzr revision: 111935 yamaoka@jpl.org-20130304102733-4qy111z41qwoh2as
> Windowing system distributor `Apple', version 10.3.1187
> Configured using:
>  `configure --host=x86_64-apple-darwin --build=i686-apple-darwin
>  --with-ns'
>
> Important settings:
>   value of $LC_CTYPE: en_US.UTF-8
>   value of $LANG: en_US.UTF-8
>   locale-coding-system: utf-8-unix
>   default enable-multibyte-characters: t
>
> Major mode: Text
>
> Minor modes in effect:
>   tooltip-mode: t
>   mouse-wheel-mode: t
>   tool-bar-mode: t
>   menu-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   blink-cursor-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   auto-compression-mode: t
>   line-number-mode: t
>   transient-mark-mode: t
>
> Recent input:
> f o o SPC f a <backspace> <backspace> b a r C-x C-s
> M-s h r f o o <return> <return> SPC M-x r e v e r t
> - b u f f <tab> <return> y e s <return> M-s h r b a
> r <return> C-g M-s h u <return> M-s h u M-x e <backspace>
> r e p o r t - e m a c s - b u f <tab> <return> <backspace>
> <return> h i g h l i g h t - r e g e x p SPC i n c
> o r r e c t l y SPC r e t a i n s SPC s t a t e SPC
> a f t e r SPC b u f f e r SPC r e v e r s i o n C-a
> C-k C-g M-x r e p o r t - e m a c s - b u g <retur
> n>
>
> Recent messages:
> For information about GNU Emacs and the GNU system, type C-h C-a.
> foo.txt has auto save data; consider M-x recover-this-file
> Saving file /Users/jlf/tmp/foo.txt...
> Wrote /Users/jlf/tmp/foo.txt
> You can run the command `revert-buffer' with s-u
> Quit
> byte-code: No highlighting to remove
> Quit
>
> Load-path shadows:
> None found.
>
> Features:
> (shadow sort gnus-util mail-extr emacsbug message format-spec rfc822 mml
> easymenu mml-sec mm-decode mm-bodies mm-encode mail-parse rfc2231
> mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
> mm-util mail-prsvr mail-utils hi-lock time-date tooltip ediff-hook
> vc-hooks lisp-float-type mwheel ns-win tool-bar dnd fontset image
> regexp-opt fringe tabulated-list newcomment lisp-mode register page
> menu-bar rfn-eshadow timer select scroll-bar mouse jit-lock font-lock
> syntax facemenu font-core frame cham georgian utf-8-lang misc-lang
> vietnamese tibetan thai tai-viet lao korean japanese hebrew greek
> romanian slovak czech european ethiopic indian cyrillic chinese
> case-table epa-hook jka-cmpr-hook help simple abbrev minibuffer loaddefs
> button faces cus-face macroexp files text-properties overlay sha1 md5
> base64 format env code-pages mule custom widget hashtable-print-readable
> backquote make-network-process ns multi-tty emacs)
>
>
>
>

-- 

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Jambunathan K]

Josh

> I can circulate a patch for some or all of what you report below.

I am unable to keep this promise.  I will let others take over this bug.

Jambunathan K.

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Josh]

Understood.  Thank you anyway.

On Mon, Mar 18, 2013 at 12:35 AM, Jambunathan K <kjambunathan@gmail.com> wrote:
> Josh
>
>> I can circulate a patch for some or all of what you report below.
>
> I am unable to keep this promise.  I will let others take over this bug.
>
> Jambunathan K.

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Stefan Monnier]

> I noticed two things that show that partial highlight-regexp state
> lingers after calling revert-buffer, see items 9 and 12.  After buffer

I've installed the patch below which should fix those problems.


        Stefan


=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2013-03-08 04:18:16 +0000
+++ lisp/hi-lock.el	2013-03-31 13:31:41 +0000
@@ -389,7 +389,9 @@
 	(define-key-after menu-bar-edit-menu [hi-lock]
 	  (cons "Regexp Highlighting" hi-lock-menu))
 	(hi-lock-find-patterns)
-	(add-hook 'font-lock-mode-hook 'hi-lock-font-lock-hook nil t))
+        (add-hook 'font-lock-mode-hook 'hi-lock-font-lock-hook nil t)
+        ;; Remove regexps from font-lock-keywords (bug#13891).
+	(add-hook 'change-major-mode-hook (lambda () (hi-lock-mode -1)) nil t))
     ;; Turned off.
     (when (or hi-lock-interactive-patterns
 	      hi-lock-file-patterns)

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Jambunathan K]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> I noticed two things that show that partial highlight-regexp state
>> lingers after calling revert-buffer, see items 9 and 12.  After buffer
>
> I've installed the patch below which should fix those problems.

(This is not a comment on the patch you have put in, but my initial
thoughts as I investigated the bug.)

I have a feeling that your fix is "inconsistent" with having
`hi-lock-interactive-patterns' as `permanent-local'.

`hi-lock-interactive-patterns' is marked as `permanent-local'.  So the
original intention is clearly to *retain* the highlighting across the
reverts and change of major modes.  With `global-hi-lock-mode' ON, you
will notice that highlighting will be retained across reverts.

When M-s h r is used, hi-lock-mode is turned on automatically.  But
after revert, the hi-lock-mode (being a minor mode) never gets turned
back again.

I was thinking of fix along the lines of 

1. If `hi-lock-interactive-patterns' is non-nil, turn on hi-lock-mode.

2. I think `font-lock-fontify-buffer' should be added to
   `hi-lock-font-lock-hook'.  For some reason, `hi-lock-font-lock-hook'
   doesn't get called at all (even though it is added to the
   `font-lock-mode-hook'.)  

   Problem area: `font-lock-mode-hook' is not documented and I really
   don't when it is getting called.

>
>
>         Stefan
>
>
> === modified file 'lisp/hi-lock.el'
> --- lisp/hi-lock.el	2013-03-08 04:18:16 +0000
> +++ lisp/hi-lock.el	2013-03-31 13:31:41 +0000
> @@ -389,7 +389,9 @@
>  	(define-key-after menu-bar-edit-menu [hi-lock]
>  	  (cons "Regexp Highlighting" hi-lock-menu))
>  	(hi-lock-find-patterns)
> -	(add-hook 'font-lock-mode-hook 'hi-lock-font-lock-hook nil t))
> +        (add-hook 'font-lock-mode-hook 'hi-lock-font-lock-hook nil t)
> +        ;; Remove regexps from font-lock-keywords (bug#13891).
> +	(add-hook 'change-major-mode-hook (lambda () (hi-lock-mode -1)) nil t))
>      ;; Turned off.
>      (when (or hi-lock-interactive-patterns
>  	      hi-lock-file-patterns)

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Stefan Monnier]

>>> I noticed two things that show that partial highlight-regexp state
>>> lingers after calling revert-buffer, see items 9 and 12.  After buffer
>> I've installed the patch below which should fix those problems.
> (This is not a comment on the patch you have put in, but my initial
> thoughts as I investigated the bug.)
> I have a feeling that your fix is "inconsistent" with having
> `hi-lock-interactive-patterns' as `permanent-local'.

Agreed.  Even more so for hi-lock-file-patterns.

> With `global-hi-lock-mode' ON, you will notice that highlighting will
> be retained across reverts.

Not any more after my patch.

> 2. I think `font-lock-fontify-buffer' should be added to
>    `hi-lock-font-lock-hook'.

Why?

>    For some reason, `hi-lock-font-lock-hook' doesn't get called at all
>    (even though it is added to the `font-lock-mode-hook'.)

What was your test case?

>    Problem area: `font-lock-mode-hook' is not documented and I really
>    don't when it is getting called.

font-lock-mode is a normal minor-mode, so font-lock-mode-hook is called
at the end of running font-lock-mode (both to enable and to disable it).


        Stefan

bug#13891: 24.3.50; highlight-regexp incorrectly retains partial state after buffer reversion

[Jambunathan K]

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


>> `hi-lock-interactive-patterns' as `permanent-local'.
>
> Agreed.  Even more so for hi-lock-file-patterns.

>> With `global-hi-lock-mode' ON, you will notice that highlighting will
>> be retained across reverts.
>
> Not any more after my patch.

Whether this change in behaviour counts as a regression, I am not sure.


>> 2. I think `font-lock-fontify-buffer' should be added to
>>    `hi-lock-font-lock-hook'.
>
> Why?

`hi-lock-font-lock-hook' adds the font lock keywords.
`font-lock-fontify-buffer' activates the keywords. 

>>    For some reason, `hi-lock-font-lock-hook' doesn't get called at all
>>    (even though it is added to the `font-lock-mode-hook'.)
>
> What was your test case?

For the sake of discussion, I undid your patch.  

Apply this patch and re-load hi-lock.el. 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: hi-lock.el.diff --]
[-- Type: text/x-diff, Size: 623 bytes --]

=== modified file 'lisp/hi-lock.el'
--- lisp/hi-lock.el	2013-03-31 13:34:35 +0000
+++ lisp/hi-lock.el	2013-04-04 14:54:39 +0000
@@ -390,8 +390,8 @@ versions before 22 use the following in
 	  (cons "Regexp Highlighting" hi-lock-menu))
 	(hi-lock-find-patterns)
         (add-hook 'font-lock-mode-hook 'hi-lock-font-lock-hook nil t)
-        ;; Remove regexps from font-lock-keywords (bug#13891).
-	(add-hook 'change-major-mode-hook (lambda () (hi-lock-mode -1)) nil t))
+	(hi-lock-font-lock-hook)
+	(font-lock-fontify-buffer))
     ;; Turned off.
     (when (or hi-lock-interactive-patterns
 	      hi-lock-file-patterns)


[-- Attachment #3: Type: text/plain, Size: 700 bytes --]


1. M-s h r something
2. Modify buffer.
3. Revert buffer.
4. M-: hi-lock-interactive-patterns
   Verify that variable is non-nil.

5. M-x hi-lock-mode.  (For now do it by hand.)
6. See that highlighting kicks in.

My expectation was that once `hi-lock-mode' is turned on
`font-lock-mode-hook' will be *automatically* called.  

Is it the minor mode's responsibility to explicitly call it's
fontification handler?

>
>>    Problem area: `font-lock-mode-hook' is not documented and I really
>>    don't when it is getting called.
>
> font-lock-mode is a normal minor-mode, so font-lock-mode-hook is called
> at the end of running font-lock-mode (both to enable and to disable it).
>
>
>         Stefan