momentary-string-display

momentary-string-display

[Juanma Barranquero]

`momentary-string-display' is a little weird function, or at least it
*acts* a little weird in my tests:

  1) It does not check that POS is valid.

  2) It does not check whether POS satisfies any restriction, nor uses
`widen' and `save-restriction' to circumvent it.

  3) Its docstring says: "Momentarily display STRING in the buffer at
POS.", which could be taken as this being valid:

  (momentary-string-display "test"
                            (with-current-buffer MY-BUFFER
                              (point-marker)))

   which isn't; the buffer temporarily modified is (current-buffer),
not MY-BUFFER.

  4) From its docstring, it seems as if STRING should always be
displayed, which isn't true (STRING is only shown when POS and point
both can be simultaneously displayed on the window).

Worse yet, in cases 1) to 3), `momentary-string-display' can err out
after displaying STRING, leaving the buffer modified and with
`buffer-file-name' set to nil.

                    /L/e/k/t/u

Re: momentary-string-display

[Richard Stallman]

momentary-string-display is a kludge, which should be used as little
as possible.  It is used very little, so it has little potential to do
any harm.

I see two uses of momentary-string-display in table.el which perhaps
should be replaced by calls to display-warning.
Takaaki-san, do you think that is a good idea?

It would be clean to make momentary-string-display use overlays
and not change the buffer text at all.  Would you like to try
writing that?  But let's not delay the release for it.

Re: momentary-string-display

[Juanma Barranquero]

On 12/27/06, Richard Stallman <rms@gnu.org> wrote:

> momentary-string-display is a kludge, which should be used as little
> as possible.

I couldn't agree more.

> It is used very little, so it has little potential to do
> any harm.

Still, points 1) to 3) in my post would be fixed or circumvented just by adding

  (setq pos (point))

after

  (goto-char pos)

and clarifying the docstring to note that, when passed a marker, only
the position is used, not the buffer.

> It would be clean to make momentary-string-display use overlays
> and not change the buffer text at all.  Would you like to try
> writing that?

I assume you're asking to Takaaki :)

> But let's not delay the release for it.

Sure. What is delaying the release, BTW?

                    /L/e/k/t/u

Re: momentary-string-display

[Richard Stallman]

    Still, points 1) to 3) in my post would be fixed or circumvented just by adding

      (setq pos (point))

    after

      (goto-char pos)

    and clarifying the docstring to note that, when passed a marker, only
    the position is used, not the buffer.

That change is ok.

    > But let's not delay the release for it.

    Sure. What is delaying the release, BTW?

Please see the list in FOR-RELEASE.

Re: momentary-string-display

[Juanma Barranquero]

On 12/29/06, Richard Stallman <rms@gnu.org> wrote:

> That change is ok.

I've installed my one-line hack to avoid data loss (plus one-line
docstring clarification).

                    /L/e/k/t/u

Re: momentary-string-display

[Kevin Rodgers]

Richard Stallman wrote:
> It would be clean to make momentary-string-display use overlays
> and not change the buffer text at all.  Would you like to try
> writing that?  But let's not delay the release for it.

I'd like to try.  Note that I removed all the code that tries to
compensate for the text being inserted in such a way that its end or
start is not displayed in the selected window (under the assumption that
the display engine will DTRT with the overlay); but it should be
possible to restore that code, replacing the reference to message-end
with (+ pos (length string)).

I also added a small feature: a new face, used to display the text.


*** subr.el~	Thu Dec 11 09:37:00 2006
--- subr.el	Thu Dec 28 21:26:33 2006
***************
*** 1884,1889 ****
--- 1884,1894 ----
     (if all (save-excursion (set-buffer (other-buffer))))
     (set-buffer-modified-p (buffer-modified-p)))

+ (defface momentary
+   '((t (:inherit mode-line)))
+   "Face for momentarily displaying text in the current buffer."
+   :group 'display)
+
   (defun momentary-string-display (string pos &optional exit-char message)
     "Momentarily display STRING in the buffer at POS.
   Display remains until next event is input.
***************
*** 1894,1923 ****
   Display MESSAGE (optional fourth arg) in the echo area.
   If MESSAGE is nil, instructions to type EXIT-CHAR are displayed there."
     (or exit-char (setq exit-char ?\s))
!   (let ((inhibit-read-only t)
! 	;; Don't modify the undo list at all.
! 	(buffer-undo-list t)
! 	(modified (buffer-modified-p))
! 	(name buffer-file-name)
! 	insert-end)
       (unwind-protect
   	(progn
! 	  (save-excursion
! 	    (goto-char pos)
! 	    ;; defeat file locking... don't try this at home, kids!
! 	    (setq buffer-file-name nil)
! 	    (insert-before-markers string)
! 	    (setq insert-end (point))
! 	    ;; If the message end is off screen, recenter now.
! 	    (if (< (window-end nil t) insert-end)
! 		(recenter (/ (window-height) 2)))
! 	    ;; If that pushed message start off the screen,
! 	    ;; scroll to start it at the top of the screen.
! 	    (move-to-window-line 0)
! 	    (if (> (point) pos)
! 		(progn
! 		  (goto-char pos)
! 		  (recenter 0))))
   	  (message (or message "Type %s to continue editing.")
   		   (single-key-description exit-char))
   	  (let (char)
--- 1899,1909 ----
   Display MESSAGE (optional fourth arg) in the echo area.
   If MESSAGE is nil, instructions to type EXIT-CHAR are displayed there."
     (or exit-char (setq exit-char ?\s))
!   (let ((momentary-overlay (make-overlay pos pos nil t)))
       (unwind-protect
   	(progn
! 	  (overlay-put momentary-overlay 'before-string
! 		       (propertize string 'face 'momentary))
   	  (message (or message "Type %s to continue editing.")
   		   (single-key-description exit-char))
   	  (let (char)
***************
*** 1937,1947 ****
   	      (or (eq char exit-char)
   		  (eq char (event-convert-list exit-char))
   		  (setq unread-command-events (list char))))))
!       (if insert-end
! 	  (save-excursion
! 	    (delete-region pos insert-end)))
!       (setq buffer-file-name name)
!       (set-buffer-modified-p modified))))

   \f
   ;;;; Overlay operations
--- 1923,1930 ----
   	      (or (eq char exit-char)
   		  (eq char (event-convert-list exit-char))
   		  (setq unread-command-events (list char))))))
!       (when (overlay-get momentary-overlay 'before-string)
! 	(delete-overlay momentary-overlay)))))

   \f
   ;;;; Overlay operations


-- 
Kevin Rodgers
Denver, Colorado, USA

Re: momentary-string-display

[Richard Stallman]

Could you please try your changed version with various commands that use
momentary-string-display, and see if it fully works?

As for this code,

    ! 	    ;; If the message end is off screen, recenter now.
    ! 	    (if (< (window-end nil t) insert-end)
    ! 		(recenter (/ (window-height) 2)))
    ! 	    ;; If that pushed message start off the screen,
    ! 	    ;; scroll to start it at the top of the screen.
    ! 	    (move-to-window-line 0)
    ! 	    (if (> (point) pos)
    ! 		(progn
    ! 		  (goto-char pos)
    ! 		  (recenter 0))))

you could replace it with some reasonable heuristic that
doesn't go wrong very often.

Re: momentary-string-display

[Kevin Rodgers]

Richard Stallman wrote:
> Could you please try your changed version with various commands that use
> momentary-string-display, and see if it fully works?

The only calls to momentary-string-display are in ada-prj.el,
fortran.el, pascal.el, and table.el; but I don't use any of those
libraries, so I wouldn't be able to tell whether the function is
working the same, better, or worse in those contexts.

> As for this code,
> 
>     ! 	    ;; If the message end is off screen, recenter now.
>     ! 	    (if (< (window-end nil t) insert-end)
>     ! 		(recenter (/ (window-height) 2)))
>     ! 	    ;; If that pushed message start off the screen,
>     ! 	    ;; scroll to start it at the top of the screen.
>     ! 	    (move-to-window-line 0)
>     ! 	    (if (> (point) pos)
>     ! 		(progn
>     ! 		  (goto-char pos)
>     ! 		  (recenter 0))))
> 
> you could replace it with some reasonable heuristic that
> doesn't go wrong very often.

As I meant to say, the same heuristic can be used by replacing the
reference to insert-end with (+ pos (length string)) -- as long as
the new call to overlay-put causes a redisplay that updates window-end
just as the original call to insert does.

Is there a problem with the current heuristic?  I think now that I
should have left that code in (with insert-end replaced as described).

-- 
Kevin Rodgers
Denver, Colorado, USA

Re: momentary-string-display

[Stephen Leake]

Kevin Rodgers <kevin.d.rodgers@gmail.com> writes:

> Richard Stallman wrote:
>> Could you please try your changed version with various commands that use
>> momentary-string-display, and see if it fully works?
>
> The only calls to momentary-string-display are in ada-prj.el,

momentary-string-display in ada-prj.el is working fine, with the
latest CVS.


-- 
-- Stephe

Re: momentary-string-display

[Richard Stallman]

    As I meant to say, the same heuristic can be used by replacing the
    reference to insert-end with (+ pos (length string)) -- as long as
    the new call to overlay-put causes a redisplay that updates window-end
    just as the original call to insert does.

I just made a change in window-end which I think will make it handle
that case correctly.

So let's have your patch and install it.

Re: momentary-string-display

[Kevin Rodgers]

Richard Stallman wrote:
>     As I meant to say, the same heuristic can be used by replacing the
>     reference to insert-end with (+ pos (length string)) -- as long as
>     the new call to overlay-put causes a redisplay that updates window-end
>     just as the original call to insert does.
> 
> I just made a change in window-end which I think will make it handle
> that case correctly.
> 
> So let's have your patch and install it.

2007-01-04  Kevin Rodgers  <kevin.d.rodgers@gmail.com>

	* subr.el (momentary): New face.
	(momentary-string-display): Display the string via a temporary
	overlay using the new face, instead of inserting it in the buffer.


*** subr.el~	Mon Dec 11 09:37:43 2006
--- subr.el	Thu Jan  4 08:16:31 2007
***************
*** 1884,1889 ****
--- 1884,1894 ----
     (if all (save-excursion (set-buffer (other-buffer))))
     (set-buffer-modified-p (buffer-modified-p)))

+ (defface momentary
+   '((t (:inherit mode-line)))
+   "Face for momentarily displaying text in the current buffer."
+   :group 'display)
+
   (defun momentary-string-display (string pos &optional exit-char message)
     "Momentarily display STRING in the buffer at POS.
   Display remains until next event is input.
***************
*** 1894,1915 ****
   Display MESSAGE (optional fourth arg) in the echo area.
   If MESSAGE is nil, instructions to type EXIT-CHAR are displayed there."
     (or exit-char (setq exit-char ?\s))
!   (let ((inhibit-read-only t)
! 	;; Don't modify the undo list at all.
! 	(buffer-undo-list t)
! 	(modified (buffer-modified-p))
! 	(name buffer-file-name)
! 	insert-end)
       (unwind-protect
   	(progn
! 	  (save-excursion
! 	    (goto-char pos)
! 	    ;; defeat file locking... don't try this at home, kids!
! 	    (setq buffer-file-name nil)
! 	    (insert-before-markers string)
! 	    (setq insert-end (point))
   	    ;; If the message end is off screen, recenter now.
! 	    (if (< (window-end nil t) insert-end)
   		(recenter (/ (window-height) 2)))
   	    ;; If that pushed message start off the screen,
   	    ;; scroll to start it at the top of the screen.
--- 1899,1911 ----
   Display MESSAGE (optional fourth arg) in the echo area.
   If MESSAGE is nil, instructions to type EXIT-CHAR are displayed there."
     (or exit-char (setq exit-char ?\s))
!   (let ((momentary-overlay (make-overlay pos pos nil t)))
       (unwind-protect
   	(progn
! 	  (overlay-put momentary-overlay 'before-string
! 		       (propertize string 'face 'momentary))
   	    ;; If the message end is off screen, recenter now.
! 	    (if (< (window-end nil t) (+ pos (length string)))
   		(recenter (/ (window-height) 2)))
   	    ;; If that pushed message start off the screen,
   	    ;; scroll to start it at the top of the screen.
***************
*** 1937,1947 ****
   	      (or (eq char exit-char)
   		  (eq char (event-convert-list exit-char))
   		  (setq unread-command-events (list char))))))
!       (if insert-end
! 	  (save-excursion
! 	    (delete-region pos insert-end)))
!       (setq buffer-file-name name)
!       (set-buffer-modified-p modified))))

   \f
   ;;;; Overlay operations
--- 1933,1941 ----
   	      (or (eq char exit-char)
   		  (eq char (event-convert-list exit-char))
   		  (setq unread-command-events (list char))))))
!       (when (overlayp momentary-overlay)
! 	(delete-overlay momentary-overlay)))))
!

   \f
   ;;;; Overlay operations

-- 
Kevin Rodgers
Denver, Colorado, USA

Re: momentary-string-display

[Richard Stallman]

I installed your patch, but the parens needed some fixing.

Re: momentary-string-display

[Juanma Barranquero]

On 1/4/07, Richard Stallman <rms@gnu.org> wrote:

> I installed your patch, but the parens needed some fixing.

Hmm... This doesn't look like a mismatched paren:

make[1]: Entering directory `C:/emacs/HEAD/src'
"./oo-spd/i386/temacs.exe" -batch -l loadup dump
Loading loadup.el (source)...
Using load-path (../lisp)
Loading emacs-lisp/byte-run...
Loading emacs-lisp/backquote...
Loading subr...
Symbol's function definition is void: custom-declare-face
make[1]: *** [oo-spd/i386/emacs.exe] Error -1
make[1]: Leaving directory `C:/emacs/HEAD/src'
make: *** [all-other-dirs-gmake] Error 2

                    /L/e/k/t/u

Re: momentary-string-display

[Richard Stallman]

    make[1]: Entering directory `C:/emacs/HEAD/src'
    "./oo-spd/i386/temacs.exe" -batch -l loadup dump
    Loading loadup.el (source)...
    Using load-path (../lisp)
    Loading emacs-lisp/byte-run...
    Loading emacs-lisp/backquote...
    Loading subr...
    Symbol's function definition is void: custom-declare-face

That is a different problem -- subr.el is too early
for a defface.  I will move it into faces.el.

Re: momentary-string-display

[Stephen Leake]

Richard Stallman <rms@gnu.org> writes:

> I installed your patch, but the parens needed some fixing.

This has a problem; point is moved to the beginning of the window
while the momentary string is displayed, and is still there after the
momentary string is removed.

Previously, point stayed where it was when momentary-string-display
was invoked.

The previous behavior is preferable, but the current behavior is not
dangerous, just annoying.

I tested this with ada-prj. Recipe:

emacs -Q
M-x ada-mode
M-x ada-prj-edit
<tab><tab><tab><tab><tab><tab>
(point is now on a [Help] button)
<ret>
(momentary string is displayed)
<spc>
(momentary string is removed)


Looking at the code, the problem is the test for 'message start
scrolled off the screen'; it should restore point if the message was
_not_ scrolled off.

-- 
-- Stephe

Re: momentary-string-display

[Richard Stallman]

Does this patch fix that bug?

Index: subr.el
===================================================================
RCS file: /cvsroot/emacs/emacs/lisp/subr.el,v
retrieving revision 1.542
diff -c -c -r1.542 subr.el
*** subr.el	5 Jan 2007 17:49:43 -0000	1.542
--- subr.el	5 Jan 2007 19:58:39 -0000
***************
*** 1907,1913 ****
  	  ;; scroll to start it at the top of the screen.
  	  (move-to-window-line 0)
  	  (if (> (point) pos)
! 	      (progn
  		(goto-char pos)
  		(recenter 0)))
  	  (message (or message "Type %s to continue editing.")
--- 1907,1913 ----
  	  ;; scroll to start it at the top of the screen.
  	  (move-to-window-line 0)
  	  (if (> (point) pos)
! 	      (save-excursion
  		(goto-char pos)
  		(recenter 0)))
  	  (message (or message "Type %s to continue editing.")

Re: momentary-string-display

[Stephen Leake]

Richard Stallman <rms@gnu.org> writes:

> Does this patch fix that bug?

No, since the problem is the "move-to-window-line".

This fixes it; there may be a more elegant way:

Index: subr.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/subr.el,v
retrieving revision 1.542
diff -c -r1.542 subr.el
*** subr.el	5 Jan 2007 17:49:43 -0000	1.542
--- subr.el	6 Jan 2007 07:35:08 -0000
***************
*** 1905,1915 ****
  	      (recenter (/ (window-height) 2)))
  	  ;; If that pushed message start off the screen,
  	  ;; scroll to start it at the top of the screen.
! 	  (move-to-window-line 0)
! 	  (if (> (point) pos)
! 	      (progn
! 		(goto-char pos)
! 		(recenter 0)))
  	  (message (or message "Type %s to continue editing.")
  		   (single-key-description exit-char))
  	  (let (char)
--- 1905,1917 ----
  	      (recenter (/ (window-height) 2)))
  	  ;; If that pushed message start off the screen,
  	  ;; scroll to start it at the top of the screen.
!           (let ((start-pos (point)))
!             (move-to-window-line 0)
!             (if (> (point) pos)
!                 (progn
!                   (goto-char pos)
!                   (recenter 0))
!               (goto-char start-pos)))
  	  (message (or message "Type %s to continue editing.")
  		   (single-key-description exit-char))
  	  (let (char)

-- 
-- Stephe

Re: momentary-string-display

[Richard Stallman]

Does this code work?

	  ;; If that pushed message start off the screen,
	  ;; scroll to start it at the top of the screen.
	  (save-excursion
	    (move-to-window-line 0)
	    (if (> (point) pos)
		(goto-char pos)
	      (recenter 0)))

Re: momentary-string-display

[Stephen Leake]

Richard Stallman <rms@gnu.org> writes:

> Does this code work?
>
> 	  ;; If that pushed message start off the screen,
> 	  ;; scroll to start it at the top of the screen.
> 	  (save-excursion
> 	    (move-to-window-line 0)
> 	    (if (> (point) pos)
> 		(goto-char pos)
> 	      (recenter 0)))

Yes.

And that is more elegant. I had rejected using 'save-excursion'
because I thought point had to move in the case where the scrolling
had to happen. But in fact it is only the window that needs to move,
and the window position is _not_ preserved by 'save-excursion'.

Hmm. I guess I thought the window position _was_ preserved by
'save-excursion'. But the doc for 'save-excursion' does not say that.

-- 
-- Stephe

Re: momentary-string-display

[Tak Ota]

Wed, 27 Dec 2006 16:16:49 -0500: Richard Stallman <rms@gnu.org> wrote:

> momentary-string-display is a kludge, which should be used as little
> as possible.  It is used very little, so it has little potential to do
> any harm.

I see.  I had never looked its implementation before.  It certainly
looks kludge.

> I see two uses of momentary-string-display in table.el which perhaps
> should be replaced by calls to display-warning.
> Takaaki-san, do you think that is a good idea?

I compared the behavior of the two functions and I prefer
momentary-string-display.  However, I don't object replacing it with
display-warning.

> It would be clean to make momentary-string-display use overlays
> and not change the buffer text at all.  Would you like to try
> writing that?  But let's not delay the release for it.

If you are asking me I don't know how to introduce
momentary-string-display equivalent pop-up text effect by using
overlays.  Anyway if the cleaning up doesn't look feasible by the
release please replace momentary-string-display with something that
does similar work.  I agree the release should not be delayed by this
issue.

-Tak

Re: momentary-string-display

[Richard Stallman]

    > I see two uses of momentary-string-display in table.el which perhaps
    > should be replaced by calls to display-warning.
    > Takaaki-san, do you think that is a good idea?

    I compared the behavior of the two functions and I prefer
    momentary-string-display.  However, I don't object replacing it with
    display-warning.

Would you please do that, for the two uses in table.el?

Re: momentary-string-display

[Tak Ota]

[-- Attachment #1: Type: Text/Plain, Size: 723 bytes --]

Wed, 03 Jan 2007 16:11:01 -0500: Richard Stallman <rms@gnu.org> wrote:

>     > I see two uses of momentary-string-display in table.el which perhaps
>     > should be replaced by calls to display-warning.
>     > Takaaki-san, do you think that is a good idea?
> 
>     I compared the behavior of the two functions and I prefer
>     momentary-string-display.  However, I don't object replacing it with
>     display-warning.
> 
> Would you please do that, for the two uses in table.el?
> 

Sure.  Could you have someone check in the attached patch?

-Tak


2007-01-03  Takaaki Ota  <Takaaki.Ota@am.sony.com>

	* textmodes/table.el (table--warn-incompatibility): replace all
	momentary-string-display with display-warning.


[-- Attachment #2: table.patch --]
[-- Type: Text/Plain, Size: 2175 bytes --]

*** emacs-22.0.91/lisp/textmodes/table.el	Wed Jan  3 13:29:35 2007
--- ../../../emacs/emacs-22.0.91/lisp/textmodes/table.el	Wed Jan  3 13:23:46 2007
***************
*** 6,12 ****
  ;; Keywords: wp, convenience
  ;; Author: Takaaki Ota <Takaaki.Ota@am.sony.com>
  ;; Created: Sat Jul 08 2000 13:28:45 (PST)
! ;; Revised: Thu Jul 20 2006 17:30:09 (PDT)
  
  ;; This file is part of GNU Emacs.
  
--- 6,12 ----
  ;; Keywords: wp, convenience
  ;; Author: Takaaki Ota <Takaaki.Ota@am.sony.com>
  ;; Created: Sat Jul 08 2000 13:28:45 (PST)
! ;; Revised: Wed Jan 03 2007 13:23:46 (PST)
  
  ;; This file is part of GNU Emacs.
  
***************
*** 5358,5364 ****
      (cond ((and (featurep 'xemacs)
  		(not (get 'table-disable-incompatibility-warning 'xemacs)))
  	   (put 'table-disable-incompatibility-warning 'xemacs t)
! 	   (momentary-string-display
  	    "
  *** Warning ***
  
--- 5358,5364 ----
      (cond ((and (featurep 'xemacs)
  		(not (get 'table-disable-incompatibility-warning 'xemacs)))
  	   (put 'table-disable-incompatibility-warning 'xemacs t)
! 	   (display-warning 'table
  	    "
  *** Warning ***
  
***************
*** 5369,5380 ****
  aware of this.
  
  "
! 	    (save-excursion (forward-line 1) (point))))
  	  ((and (boundp 'flyspell-mode)
  		flyspell-mode
  		(not (get 'table-disable-incompatibility-warning 'flyspell)))
  	   (put 'table-disable-incompatibility-warning 'flyspell t)
! 	   (momentary-string-display
  	    "
  *** Warning ***
  
--- 5369,5380 ----
  aware of this.
  
  "
! 	    :warning))
  	  ((and (boundp 'flyspell-mode)
  		flyspell-mode
  		(not (get 'table-disable-incompatibility-warning 'flyspell)))
  	   (put 'table-disable-incompatibility-warning 'flyspell t)
! 	   (display-warning 'table
  	    "
  *** Warning ***
  
***************
*** 5383,5389 ****
  works better than the previous versions however not fully compatible.
  
  "
! 	    (save-excursion (forward-line 1) (point))))
  	  )))
  
  (defun table--cell-blank-str (&optional n)
--- 5383,5389 ----
  works better than the previous versions however not fully compatible.
  
  "
! 	    :warning))
  	  )))
  
  (defun table--cell-blank-str (&optional n)

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

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