[R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

I don't know how to use this feature, but looking at the code suggests
that this bug still exists.  Can anyone verify that?

It should be pretty easy to prevent this code from changing the
modified flag (and perhaps bind buffer-undo-list to t).  I am not sure
where the change should be made, though.  Would someone familiar with
this package please take a look and DTRT?

------- Start of forwarded message -------
X-Spam-Status: No, score=0.0 required=5.0 tests=UNPARSEABLE_RELAY 
	autolearn=failed version=3.1.0
Date: Mon, 7 May 2007 17:30:31 +0200 (CEST)
From: Reindert-Jan Ekker <R.Ekker@ai.rug.nl>
To: bug-gnu-emacs@gnu.org
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed
Subject: highlight-changes-rotate-faces sets buffer modified flag

L.S.

Running the function highlight-changes-rotate-faces sets the buffer's 
modified flag. It seems to me that this should not happen, since only some 
faces are changed. As a workaround, I use the following advice:

;; advice for highlight-changes-rotate-faces
;; so that it does not change the modified flag for the buffer
(defadvice highlight-changes-rotate-faces (around around-rotate-faces)
   (let ((was-modified (buffer-modified-p)))
 	ad-do-it
 	(unless was-modified
 	  (set-buffer-modified-p nil))))
(ad-activate 'highlight-changes-rotate-faces)

The version string of my emacs:
"GNU Emacs 21.3.1 (i386-mingw-nt5.1.2600) of 2004-03-10 on NYAUMO"

With kind regards,

Reindert-Jan Ekker


_______________________________________________
bug-gnu-emacs mailing list
bug-gnu-emacs@gnu.org
http://lists.gnu.org/mailman/listinfo/bug-gnu-emacs
------- End of forwarded message -------

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

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

> I don't know how to use this feature, but looking at the code suggests
> that this bug still exists.  Can anyone verify that?
> 
> It should be pretty easy to prevent this code from changing the
> modified flag (and perhaps bind buffer-undo-list to t).

The attached patch works for me in trivial test cases.

[-- Attachment #2: hilit-chg.patch --]
[-- Type: text/plain, Size: 1616 bytes --]

*** hilit-chg.el	Tue Jan 23 06:40:02 2007
--- hilit-chg.el	Tue May  8 23:38:32 2007
***************
*** 790,806 ****
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (if (eq highlight-changes-mode 'active)
!       (let ((after-change-functions nil))
! 	;; ensure hilit-chg-list is made and up to date
! 	(hilit-chg-make-list)
! 	;; remove our existing overlays
! 	(hilit-chg-hide-changes)
! 	;; for each change text property, increment it
! 	(hilit-chg-map-changes 'hilit-chg-bump-change)
! 	;; and display them all if active
! 	(if (eq highlight-changes-mode 'active)
! 	    (hilit-chg-display-changes))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)

--- 790,811 ----
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (when (eq highlight-changes-mode 'active)
!     (let ((modified (buffer-modified-p))
! 	  (buffer-undo-list t)
! 	  (after-change-functions nil))
!       (unwind-protect
! 	  (progn
! 	    ;; ensure hilit-chg-list is made and up to date
! 	    (hilit-chg-make-list)
! 	    ;; remove our existing overlays
! 	    (hilit-chg-hide-changes)
! 	    ;; for each change text property, increment it
! 	    (hilit-chg-map-changes 'hilit-chg-bump-change)
! 	    ;; and display them all if active
! 	    (if (eq highlight-changes-mode 'active)
! 		(hilit-chg-display-changes)))
! 	(unless modified (set-buffer-modified-p nil)))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)


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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

 > I don't know how to use this feature, but looking at the code suggests
 > that this bug still exists.  Can anyone verify that?
 >
 > It should be pretty easy to prevent this code from changing the
 > modified flag (and perhaps bind buffer-undo-list to t).  I am not sure
 > where the change should be made, though.  Would someone familiar with
 > this package please take a look and DTRT?

At second sight I would _not_ change the code at all.

(1) With the current code the first undo after saving a buffer would
undo the change done by `highlight-changes-rotate-faces' and incorrectly
make the buffer appear modified.  Changing the code by resetting the
buffer-modified flag and _not_ let-binding `buffer-undo-list' wouldn't
ameliorate anything here.

(2) Suppose I reset the buffer-modified flag and bind `buffer-undo-list'
to t as in my patch.  In this case I would not be able to undo the
change caused by `highlight-changes-rotate-faces' and end up with quite
confusing colors after an arbitrary sequence of undos and saves.

`highlight-changes-rotate-faces' is supposed to run by a hook within
code that resets the buffer-modified flag.  We could say so in the
doc-string of that function to avoid confusion for the case that the
function is called interactively (the OP's problem, IIUC).

The problem described in (1) could be probably resolved by setting undo
boundaries around the last "real" buffer change(s) and the subsequent
color rotation but I doubt that it's worth the trouble.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Stefan Monnier]

> !       (let ((after-change-functions nil))

That's bad for karma.  Use "(let ((inhibit-modification-hooks t))"
instead, thank you,


        Stefan

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

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

 >>!       (let ((after-change-functions nil))
 >
 >
 > That's bad for karma.  Use "(let ((inhibit-modification-hooks t))"
 > instead, thank you,

Indeed.  Meanwhile I think that we should avoid altering the modified
state but retain undo information for face rotations.  This may show up
as a minor annoyance during undos but can't harm otherwise.  Eventually,
we should also add an `after-revert-hook' to remove hilit-chg's overlays
(unless `revert-buffer' gets re-implemented as TObeDOne).

[-- Attachment #2: hilit-chg.patch --]
[-- Type: text/plain, Size: 1788 bytes --]

*** hilit-chg.el	Tue Jan 23 06:40:02 2007
--- hilit-chg.el	Wed May  9 19:33:32 2007
***************
*** 790,806 ****
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (if (eq highlight-changes-mode 'active)
!       (let ((after-change-functions nil))
! 	;; ensure hilit-chg-list is made and up to date
! 	(hilit-chg-make-list)
! 	;; remove our existing overlays
! 	(hilit-chg-hide-changes)
! 	;; for each change text property, increment it
! 	(hilit-chg-map-changes 'hilit-chg-bump-change)
! 	;; and display them all if active
! 	(if (eq highlight-changes-mode 'active)
! 	    (hilit-chg-display-changes))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)

--- 790,813 ----
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (when (eq highlight-changes-mode 'active)
!     ;; Avoid marking the buffer as modified.  Note: We do not suppress modifying
!     ;; `buffer-undo-list' here.  Hence, undoing a face rotation may show up as a
!     ;; buffer modification.
!     (let ((modified (buffer-modified-p))
! 	  (inhibit-modification-hooks t))
!       (unwind-protect
! 	  (progn
! 	    ;; ensure hilit-chg-list is made and up to date
! 	    (hilit-chg-make-list)
! 	    ;; remove our existing overlays
! 	    (hilit-chg-hide-changes)
! 	    ;; for each change text property, increment it
! 	    (hilit-chg-map-changes 'hilit-chg-bump-change)
! 	    ;; and display them all if active
! 	    (if (eq highlight-changes-mode 'active)
! 		(hilit-chg-display-changes)))
! 	(unless modified (set-buffer-modified-p nil)))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)


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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Stefan Monnier]

> ! 	(unless modified (set-buffer-modified-p nil)))))

Great, another occasion for nitpicking:
a minor optimization is to use restore-buffer-modified-p here,


        Stefan

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

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

 > a minor optimization is to use restore-buffer-modified-p here,

... karmatologistically yours

[-- Attachment #2: hilit-chg.patch --]
[-- Type: text/plain, Size: 1792 bytes --]

*** hilit-chg.el	Tue Jan 23 06:40:02 2007
--- hilit-chg.el	Wed May  9 19:33:32 2007
***************
*** 790,806 ****
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (if (eq highlight-changes-mode 'active)
!       (let ((after-change-functions nil))
! 	;; ensure hilit-chg-list is made and up to date
! 	(hilit-chg-make-list)
! 	;; remove our existing overlays
! 	(hilit-chg-hide-changes)
! 	;; for each change text property, increment it
! 	(hilit-chg-map-changes 'hilit-chg-bump-change)
! 	;; and display them all if active
! 	(if (eq highlight-changes-mode 'active)
! 	    (hilit-chg-display-changes))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)

--- 790,813 ----
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (when (eq highlight-changes-mode 'active)
!     ;; Avoid marking the buffer as modified.  Note: We do not suppress modifying
!     ;; `buffer-undo-list' here.  Hence, undoing a face rotation may show up as a
!     ;; buffer modification.
!     (let ((modified (buffer-modified-p))
! 	  (inhibit-modification-hooks t))
!       (unwind-protect
! 	  (progn
! 	    ;; ensure hilit-chg-list is made and up to date
! 	    (hilit-chg-make-list)
! 	    ;; remove our existing overlays
! 	    (hilit-chg-hide-changes)
! 	    ;; for each change text property, increment it
! 	    (hilit-chg-map-changes 'hilit-chg-bump-change)
! 	    ;; and display them all if active
! 	    (if (eq highlight-changes-mode 'active)
! 		(hilit-chg-display-changes)))
! 	(unless modified (restore-buffer-modified-p nil)))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)


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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

    (2) Suppose I reset the buffer-modified flag and bind `buffer-undo-list'
    to t as in my patch.  In this case I would not be able to undo the
    change caused by `highlight-changes-rotate-faces' and end up with quite
    confusing colors after an arbitrary sequence of undos and saves.

Should this mode use overlays?

Should it use something the font-lock mechanism so that it can update
when the buffer is edited?

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

    ! 	  (after-change-functions nil))

That does just part of the job.  You need to bind
inhibit-modification-hooks to t.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

>     ! 	  (after-change-functions nil))
> 
> That does just part of the job.  You need to bind
> inhibit-modification-hooks to t.

Stefan already noticed that.  I simply didn't pay much attention
to existing code.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

 >     (2) Suppose I reset the buffer-modified flag and bind `buffer-undo-list'
 >     to t as in my patch.  In this case I would not be able to undo the
 >     change caused by `highlight-changes-rotate-faces' and end up with quite
 >     confusing colors after an arbitrary sequence of undos and saves.
 >
 > Should this mode use overlays?

It does use a combination of text-properties and overlays.  On my
machine, the combined use of an after-change hook and overlays already
seems to cause a noticeable slowdown during editing.

 > Should it use something the font-lock mechanism so that it can update
 > when the buffer is edited?

Hmmm ... it does so, in principle, but for the reliance on the undo
mechanism.  I doubt that we can do much better.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

     > Should it use something the font-lock mechanism so that it can update
     > when the buffer is edited?

    Hmmm ... it does so, in principle, but for the reliance on the undo
    mechanism.

I don't understand that -- can you explain?

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

 >      > Should it use something the font-lock mechanism so that it can update
 >      > when the buffer is edited?
 >
 >     Hmmm ... it does so, in principle, but for the reliance on the undo
 >     mechanism.
 >
 > I don't understand that -- can you explain?

`hilit-chg-set-face-on-change' operates pretty much like the font-lock
after-change mechanism:

If the change _is not_ caused by an undo it assigns the text-property
hilit-chg to the changed text and calls `hilit-chg-fixup' which installs
the overlays necessary for highlighting the changed text in accordance
with the just assigned text-property.

If the change _is_ caused by an undo, `hilit-chg-set-face-on-change'
falls back on the undo mechanism to restore the hilit-chg
text-properties and calls `hilit-chg-fixup' as in the non-undo case.

font-lock (usually) calculates the text properties anew after every
buffer change for the changed region regardless of whether they are
caused by an undo or not.  The idea of using undo for restoring text
(and subsequently overlay) properties strikes me as both clever and
elegant.  It makes it tricky though to keep the buffer-modified state
congruent as I explained in another mail: "face rotations" must show up
in buffer-undo-list but should not mark the buffer as modified.  This is
no problem as long as I'm able to hide rotations from the user, for
example, by running them in a hook which resets the modified flag
anyway.  It is a problem when the hook or function does not set the
modified-flag.  Hence, undoing a rotation may set the modified flag
although no user initiated buffer-change occurred.  If there were a
buffer-undo-list entry type which wouldn't set the modified flag ...

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

    If the change _is not_ caused by an undo it assigns the text-property
    hilit-chg to the changed text and calls `hilit-chg-fixup' which installs
    the overlays necessary for highlighting the changed text in accordance
    with the just assigned text-property.

    If the change _is_ caused by an undo, `hilit-chg-set-face-on-change'
    falls back on the undo mechanism to restore the hilit-chg
    text-properties and calls `hilit-chg-fixup' as in the non-undo case.

That sounds good.  However, given that, it seems that explicit changes
of the highlighting ought not to mark the buffer as modified.

When these commands are used on an unmodified buffer, perhaps they
should make undo records, but not mark the buffer as modified, and set
up the undo records so that undoing them re-marks the buffer as
unmodified.  That is a strange thing to do, but perhaps it is the least
strange alternative.

      If there were a
    buffer-undo-list entry type which wouldn't set the modified flag ...

You can make an undo entry that will clear it.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

 > That sounds good.  However, given that, it seems that explicit changes
 > of the highlighting ought not to mark the buffer as modified.

... the OP's problem.

 > When these commands are used on an unmodified buffer, perhaps they
 > should make undo records, but not mark the buffer as modified, and set
 > up the undo records so that undoing them re-marks the buffer as
 > unmodified.  That is a strange thing to do, but perhaps it is the least
 > strange alternative.
 >
 >       If there were a
 >     buffer-undo-list entry type which wouldn't set the modified flag ...
 >
 > You can make an undo entry that will clear it.

Maybe we could use a macro like

(defmacro with-buffer-undo-unmodified (&rest body)
   "Eval BODY, preserving the current buffer's modified state.
In addition create entries to `buffer-undo-list' in order to not alter
the buffer's modified state during undos and redos."
   (let ((modified (make-symbol "modified")))
     `(let ((,modified
	    (or (buffer-modified-p)
		(progn
		  (setq buffer-undo-list
			(cons '(apply set-buffer-modified-p nil)
			      buffer-undo-list))
		  nil))))
        (unwind-protect
	   (progn ,@body)
	 (unless ,modified
	   (setq buffer-undo-list
		 (cons '(apply set-buffer-modified-p nil)
		       buffer-undo-list))
	   (restore-buffer-modified-p nil))))))

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

    Maybe we could use a macro like

    (defmacro with-buffer-undo-unmodified (&rest body)
       "Eval BODY, preserving the current buffer's modified state.
    In addition create entries to `buffer-undo-list' in order to not alter
    the buffer's modified state during undos and redos."

First let's see what is best in this specific case.
Afterward we could see if any part of it is worth generalizing.

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[martin rudalics]

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

> First let's see what is best in this specific case.
> Afterward we could see if any part of it is worth generalizing.

The attached patch should handle the specific case.

[-- Attachment #2: hilit-chg.patch --]
[-- Type: text/plain, Size: 2289 bytes --]

*** hilit-chg.el	Tue May 15 20:20:12 2007
--- hilit-chg.el	Tue May 15 20:22:12 2007
***************
*** 790,806 ****
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (if (eq highlight-changes-mode 'active)
!       (let ((after-change-functions nil))
! 	;; ensure hilit-chg-list is made and up to date
! 	(hilit-chg-make-list)
! 	;; remove our existing overlays
! 	(hilit-chg-hide-changes)
! 	;; for each change text property, increment it
! 	(hilit-chg-map-changes 'hilit-chg-bump-change)
! 	;; and display them all if active
! 	(if (eq highlight-changes-mode 'active)
! 	    (hilit-chg-display-changes))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)

--- 790,826 ----
    (interactive)
    ;; If not in active mode do nothing but don't complain because this
    ;; may be bound to a hook.
!   (when (eq highlight-changes-mode 'active)
!     (let ((modified (buffer-modified-p))
! 	  (inhibit-modification-hooks t))
!       ;; The `modified' related code tries to combine two goals: (1) Record the
!       ;; rotation in `buffer-undo-list' and (2) avoid setting the modified flag
!       ;; of the current buffer due to the rotation.  We do this by inserting (in
!       ;; `buffer-undo-list') entries restoring buffer-modified-p to nil before
!       ;; and after the entry for the rotation.
!       (unless modified
! 	;; Install the "before" entry.
! 	(setq buffer-undo-list
! 	      (cons '(apply restore-buffer-modified-p nil)
! 		    buffer-undo-list)))
!       (unwind-protect
! 	  (progn
! 	    ;; ensure hilit-chg-list is made and up to date
! 	    (hilit-chg-make-list)
! 	    ;; remove our existing overlays
! 	    (hilit-chg-hide-changes)
! 	    ;; for each change text property, increment it
! 	    (hilit-chg-map-changes 'hilit-chg-bump-change)
! 	    ;; and display them all if active
! 	    (if (eq highlight-changes-mode 'active)
! 		(hilit-chg-display-changes)))
! 	(unless modified
! 	  ;; Install the "after" entry.
! 	  (setq buffer-undo-list
! 		(cons '(apply restore-buffer-modified-p nil)
! 		      buffer-undo-list))
! 
! 	  (restore-buffer-modified-p nil)))))
    ;; This always returns nil so it is safe to use in write-file-functions
    nil)


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

_______________________________________________
Emacs-devel mailing list
Emacs-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/emacs-devel

Re: [R.Ekker@ai.rug.nl: highlight-changes-rotate-faces sets buffer modified flag]

[Richard Stallman]

Please install your change in the trunk.