Re: Should overlays evaporate by default? Conclusion: No!

[Peter Whaite <emacs@whaite.ca>]

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

Thien-Thi Nguyen <ttn@gnu.org> wrote:
> Richard Stallman <rms@gnu.org> writes:
> 
> > The goal now is to *find* possible bugs.  The patch should set the
> > property to something unusual (maybe `never-set') by default.  When
> > such an overlay becomes empty, there should be some sort of warning.
> > That way, people will find code that perhaps ought to set the
> > evaporate property to something explicitly.
> 
> ok, below is a patch to buffer.c that does this, more or less.  the
> (%d,%d) is perhaps redundant, but since we're in the checking mood...

Thanks Thi.  I had to make a couple of changes (see below), but I have
been running with it for a few days now.

>From what I've seen I do not think it would be a good idea to change the
default.

The main reason is that there is a third, and common, use case which I
had not thought of.  This is where an overlay is used dynamically to
respond to user input, e.g. mouse.el, hl-line.el, comint, ediff, etc.
Typically a global or buffer local variable is used to hold the overlay,
which is positioned dynamically by applying move-overlay in response to
user input.  The positioning can result in the overlay becoming of zero
length.  Obviously it should not evaporate.

I did start to patch files exposed by my limited testing (see below),
but there were many, and I found I was setting 'evaporate to nil, not t.
Given that, and given that magically disappearing is in general
surprising default behaviour, then it would be better to retain the
current overlay defaults.

There remains the question of how to prevent overlay leaks.  It would
help if the elisp overlay documentation outlined the problem and
remedies ((overlay-put ovl 'evaporate t) or (remove-overlays)).  Perhaps
my original suggestion that erase-buffer also remove-overlays should be
considered.

For the record I'm attaching the patches I made to the elisp files.  A
quick grep through the lisp directory reveals many more that would need
patching.


[-- Attachment #2: evaporate-el.patch --]
[-- Type: text/plain, Size: 5150 bytes --]

Index: comint.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/comint.el,v
retrieving revision 1.319
diff -u -r1.319 comint.el
--- comint.el	26 May 2005 12:39:32 -0000	1.319
+++ comint.el	27 May 2005 17:05:46 -0000
@@ -1748,6 +1748,7 @@
 		  ;; Need to create the overlay
 		  (setq comint-last-prompt-overlay
 			(make-overlay prompt-start (point)))
+		  (overlay-put comint-last-prompt-overlay 'evaporate nil)
 		  (overlay-put comint-last-prompt-overlay
 			       'font-lock-face 'comint-highlight-prompt))))
 
Index: compare-w.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/compare-w.el,v
retrieving revision 1.26
diff -u -r1.26 compare-w.el
--- compare-w.el	28 Nov 2004 07:56:01 -0000	1.26
+++ compare-w.el	27 May 2005 17:05:46 -0000
@@ -341,12 +341,14 @@
     (if compare-windows-overlay1
         (move-overlay compare-windows-overlay1 beg1 end1 b1)
       (setq compare-windows-overlay1 (make-overlay beg1 end1 b1))
+      (overlay-put compare-windows-overlay1 'evaporate 'nil)
       (overlay-put compare-windows-overlay1 'face 'compare-windows-face)
       (overlay-put compare-windows-overlay1 'priority 1))
     (overlay-put compare-windows-overlay1 'window w1)
     (if compare-windows-overlay2
         (move-overlay compare-windows-overlay2 beg2 end2 b2)
       (setq compare-windows-overlay2 (make-overlay beg2 end2 b2))
+      (overlay-put compare-windows-overlay2 'evaporate 'nil)
       (overlay-put compare-windows-overlay2 'face 'compare-windows-face)
       (overlay-put compare-windows-overlay2 'priority 1))
     (overlay-put compare-windows-overlay2 'window w2)
Index: hl-line.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/hl-line.el,v
retrieving revision 1.28
diff -u -r1.28 hl-line.el
--- hl-line.el	4 Apr 2005 08:57:54 -0000	1.28
+++ hl-line.el	27 May 2005 17:05:46 -0000
@@ -134,6 +134,7 @@
       (progn
         (unless hl-line-overlay
           (setq hl-line-overlay (make-overlay 1 1)) ; to be moved
+	  (overlay-put hl-line-overlay 'evaporate nil)
           (overlay-put hl-line-overlay 'face hl-line-face))
         (overlay-put hl-line-overlay
                      'window (unless hl-line-sticky-flag (selected-window)))
@@ -168,6 +169,7 @@
     (unless (window-minibuffer-p (selected-window))
       (unless global-hl-line-overlay
         (setq global-hl-line-overlay (make-overlay 1 1)) ; to be moved
+	(overlay-put global-hl-line-overlay 'evaporate nil)
         (overlay-put global-hl-line-overlay 'face hl-line-face))
       (overlay-put global-hl-line-overlay 'window (selected-window))
       (hl-line-move global-hl-line-overlay))))
Index: image.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/image.el,v
retrieving revision 1.46
diff -u -r1.46 image.el
--- image.el	27 May 2005 13:17:50 -0000	1.46
+++ image.el	27 May 2005 17:05:46 -0000
@@ -170,6 +170,7 @@
     (let ((overlay (make-overlay pos pos buffer))
 	  (prop (if (null area) image (list (list 'margin area) image))))
       (put-text-property 0 (length string) 'display prop string)
+      (overlay-put overlay 'evaporate nil)
       (overlay-put overlay 'put-image t)
       (overlay-put overlay 'before-string string))))
 
Index: minibuf-eldef.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/minibuf-eldef.el,v
retrieving revision 1.6
diff -u -r1.6 minibuf-eldef.el
--- minibuf-eldef.el	2 Jun 2004 22:43:42 -0000	1.6
+++ minibuf-eldef.el	27 May 2005 17:05:46 -0000
@@ -98,6 +98,7 @@
       (setq match (if (consp match) (cdr match) 0))
       (setq minibuf-eldef-overlay
 	    (make-overlay (match-beginning match) (match-end match)))
+      (overlay-put minibuf-eldef-overlay 'evaporate nil)
       (setq minibuf-eldef-showing-default-in-prompt t)
       (setq minibuf-eldef-initial-input
 	    (minibuffer-contents-no-properties))
Index: mouse.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/mouse.el,v
retrieving revision 1.271
diff -u -r1.271 mouse.el
--- mouse.el	23 Apr 2005 16:40:55 -0000	1.271
+++ mouse.el	27 May 2005 17:06:04 -0000
@@ -748,6 +748,7 @@
 
 ;; Create an overlay and immediately delete it, to get "overlay in no buffer".
 (defvar mouse-drag-overlay (make-overlay 1 1))
+(overlay-put mouse-drag-overlay 'evaporate nil)
 (delete-overlay mouse-drag-overlay)
 (overlay-put mouse-drag-overlay 'face 'region)
 
Index: gnus/gnus-cite.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/gnus/gnus-cite.el,v
retrieving revision 1.16
diff -u -r1.16 gnus-cite.el
--- gnus/gnus-cite.el	26 May 2005 15:03:29 -0000	1.16
+++ gnus/gnus-cite.el	27 May 2005 17:06:04 -0000
@@ -1017,6 +1017,7 @@
 	(when (< from to)
 	  (push (setq overlay (gnus-make-overlay from to))
 		gnus-cite-overlay-list)
+	  (gnus-overlay-put overlay 'evaporate t)
 	  (gnus-overlay-put overlay 'face face))))))
 
 (defun gnus-cite-toggle (prefix)

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


Also here is my version of Thi's patch to buffer.c.  I had to avoid
calling Foverlay_put from Fmake_overlay as it caused (make-overlay 1 1)
to immediately evaporate.  The only other change was to print more
information about the overlay so it was easier to track down.


[-- Attachment #4: evaporate.patch --]
[-- Type: text/plain, Size: 3123 bytes --]

Index: buffer.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/buffer.c,v
retrieving revision 1.480
diff -u -r1.480 buffer.c
--- buffer.c	27 May 2005 11:01:38 -0000	1.480
+++ buffer.c	27 May 2005 17:06:47 -0000
@@ -178,6 +178,12 @@
 Lisp_Object Qinsert_in_front_hooks;
 Lisp_Object Qinsert_behind_hooks;
 
+#define OVERLAY_EVAPORATE_TESTING
+#ifdef OVERLAY_EVAPORATE_TESTING
+Lisp_Object Qnever_set;
+static Lisp_Object evaporate_overlay P_ ((Lisp_Object));
+#endif
+
 static void alloc_buffer_text P_ ((struct buffer *, size_t));
 static void free_buffer_text P_ ((struct buffer *b));
 static struct Lisp_Overlay * copy_overlays P_ ((struct buffer *, struct Lisp_Overlay *));
@@ -3602,6 +3608,11 @@
   XOVERLAY (overlay)->plist = Qnil;
   XOVERLAY (overlay)->next = NULL;
 
+#ifdef OVERLAY_EVAPORATE_TESTING
+  XOVERLAY (overlay)->plist
+    = Fcons (Qevaporate, Fcons (Qnever_set, XOVERLAY (overlay)->plist));
+#endif
+
   /* Put the new overlay on the wrong list.  */
   end = OVERLAY_END (overlay);
   if (OVERLAY_POSITION (end) < b->overlay_center)
@@ -3656,6 +3667,24 @@
 \f
 Lisp_Object Fdelete_overlay ();
 
+#ifdef OVERLAY_EVAPORATE_TESTING
+static Lisp_Object
+evaporate_overlay (overlay)
+     Lisp_Object overlay;
+{
+  if (EQ (Qnever_set, Foverlay_get (overlay, Qevaporate)))
+    {
+	Lisp_Object args[3];
+	args[0] = build_string("WARNING: Evaporating %S %S");
+	args[1] = overlay;
+	args[2] = Foverlay_properties(overlay);
+	Fmessage(3,args);
+    }
+
+  return Fdelete_overlay (overlay);
+}
+#endif
+
 static struct Lisp_Overlay *
 unchain_overlay (list, overlay)
      struct Lisp_Overlay *list, *overlay;
@@ -3704,7 +3733,13 @@
   CHECK_NUMBER_COERCE_MARKER (end);
 
   if (XINT (beg) == XINT (end) && ! NILP (Foverlay_get (overlay, Qevaporate)))
-    return Fdelete_overlay (overlay);
+    return
+#ifndef OVERLAY_EVAPORATE_TESTING
+      Fdelete_overlay
+#else
+      evaporate_overlay
+#endif
+      (overlay);
 
   if (XINT (beg) > XINT (end))
     {
@@ -4088,7 +4123,13 @@
       if (EQ (prop, Qevaporate) && ! NILP (value)
 	  && (OVERLAY_POSITION (OVERLAY_START (overlay))
 	      == OVERLAY_POSITION (OVERLAY_END (overlay))))
-	Fdelete_overlay (overlay);
+#ifndef OVERLAY_EVAPORATE_TESTING
+	Fdelete_overlay
+#else
+        evaporate_overlay
+#endif
+          (overlay);
+
     }
   return value;
 }
@@ -4330,7 +4371,12 @@
 	  hit_list = Fcons (overlay, hit_list);
       }
   for (; CONSP (hit_list); hit_list = XCDR (hit_list))
-    Fdelete_overlay (XCAR (hit_list));
+#ifndef OVERLAY_EVAPORATE_TESTING
+    Fdelete_overlay
+#else
+    evaporate_overlay
+#endif
+      (XCAR (hit_list));
 }
 \f
 /* Somebody has tried to store a value with an unacceptable type
@@ -5207,6 +5253,10 @@
   staticpro (&Qoverlayp);
   Qevaporate = intern ("evaporate");
   staticpro (&Qevaporate);
+#ifdef OVERLAY_EVAPORATE_TESTING
+  Qnever_set = intern ("never-set");
+  staticpro (&Qnever_set);
+#endif
   Qmodification_hooks = intern ("modification-hooks");
   staticpro (&Qmodification_hooks);
   Qinsert_in_front_hooks = intern ("insert-in-front-hooks");

[-- Attachment #5: Type: text/plain, Size: 17 bytes --]


--
Peter Whaite

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

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