Subject: bug#6130: 23.1; artist-mode spray-can malfunction

From: Daniel Koning <[email-masked]>
Date: Fri, 16 Jan 2015 23:25:01 -0600

busk <[email-masked]> writes:

> I get a malfunction with the artist-mode spray-can when my drawing
> goes "over" the borders of the frame, which manifests itself thusly:
> It doesn't stop drawing on that particular are, but rather keeps
> filling #'s in an area where I drawed until emacs is
> killed. "Activating" another frame belonging to the same emacs process
> moves the behaviour to that frame. Activating the mini-buffer makes it
> draw in the minibuffer.

I ran into this one while playing around with artist-mode and trying the
various drawing tools. I really don't think it should have been
classified as minor -- it's a nasty little loop that you can only get
out of by killing the emacs process. If you're prudent, you might
maintain the presence of mind to do so without trying to kill the
artist-mode buffer first, which just makes the spray can spew garbage
characters into whatever buffer was behind it.

What's happening is this. The spray can tool is based on a generic
helper function called `artist-mouse-draw-continuously'. (Actually, it's
misspelled as "continously," but I'm ignoring that.) This function has a
loop that checks each mouse event that comes in and exits when the
button is released. At the end of the body, it restarts a timer that
repeatedly calls `draw-fn', causing (say) the spray can to keep spraying
as long as the mouse is held down.

The problem is that the loop body tries to determine the latest event's
buffer coordinates with `posn-col-row', which, since Emacs 23, has
retrieved the buffer-local line spacing by taking the event's position
object and calling `window-buffer' on its window element. This is an
error on the part of `posn-col-row'. The position object only contains
a window element when the position is inside the boundaries of the
selected frame. Otherwise it just contains the frame.

(Of course, this behavior is completely counterintuitive: you only get
an out-of-frame position with drag events, or with motion events inside
a `track-mouse' macro; the function for retrieving the frame/window
element is just called `posn-window'; and the possibility that the
object contains a frame is totally undocumented.)

Because `window-buffer' doesn't accept a frame as an argument, it throws
an uncaught error when it tries to handle the motion event that takes
the pointer outside the frame. `artist-mouse-draw-continuously' then
unwinds in the middle of the while loop. This happens before the
previous `draw-fn' timer gets cancelled, so you end up with a zombie
timer spewing garbage characters.

There are a few different things that could stand to be fixed here:

1. The draw-continuously mouse tracking loop really needs to be inside
an `unwind-protect' with a cleanup form that gets rid of the timer.

2. `posn-col-row' should correctly handle the case in which
`posn-window' returns a frame. Preferably, it should give an estimate of
where the position *would* be if the window were larger; this is the
pre-23 behavior.

3. The elisp docs ought to reflect that the so-called `window' slot in a
position object could actually be a frame.

4. Someone should search-and-replace "continously" to "continuously"
throughout artist.el. Those functions are only referenced internally, I
think, so doing so isn't likely to break anything.

Here's a patch that handles 1 through 3. The extra explanatory material
in the docs might be an inelegant half measure, though, considering the
function and variable names still refer to the object as a window
regardless of its actual type.

I also went ahead and searched the lisp/ tree for other places that
looked risky -- that is, where a position object was assumed to hold a
window in a context where there was no such guarantee. Nothing jumped
out at me, but there could be any number of issues with third-party
code.

diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index 36c7445..aab58f1 100644
--- a/doc/lispref/commands.texi
+++ b/doc/lispref/commands.texi
@@ -1489,8 +1489,10 @@ prefix @samp{drag-}.  For example, dragging the mouse with button 2
 held down generates a @code{drag-mouse-2} event.  The second and third
 elements of the event give the starting and ending position of the
 drag, as mouse position lists (@pxref{Click Events}).  You can access
-the second element of any mouse event in the same way, with no need to
-distinguish drag events from others.
+the second element of any mouse event in the same way. However, the
+drag event may end outside the boundaries of the selected frame. In
+that case, the third element's position list contains the selected
+frame in place of a window.
 
 The @samp{drag-} prefix follows the modifier key prefixes such as
 @samp{C-} and @samp{M-}.
@@ -1635,7 +1637,10 @@ represented by lists that look like this:
 
 @noindent
 @var{position} is a mouse position list (@pxref{Click Events}),
-specifying the current position of the mouse cursor.
+specifying the current position of the mouse cursor. As with the
+end-position of a drag event, this position list may represent a
+location outside the boundaries of the selected frame, in which case
+the list contains the selected frame in place of a window.
 
 The special form @code{track-mouse} enables generation of motion
 events within its body.  Outside of @code{track-mouse} forms, Emacs
@@ -1850,6 +1855,14 @@ into another window.  That produces a pair of events like these:
                    -453816))
 @end smallexample
 
+The frame with input focus might not take up the entire screen, and
+the user might move the mouse outside the scope of the frame. Inside
+the @code{track-mouse} special form, that produces an event like this:
+
+@smallexample
+(mouse-movement (#<frame *ielm* 0x102849a30> nil (563 . 205) 532301936))
+@end smallexample
+
 To handle a SIGUSR1 signal, define an interactive function, and
 bind it to the @code{signal usr1} event sequence:
 
@@ -2014,7 +2027,9 @@ Events}); and @code{nil} otherwise.
 various parts of it:
 
 @defun posn-window position
-Return the window that @var{position} is in.
+Return the window that @var{position} is in. If the frame with input
+focus does not have any window at @var{position}, return the frame
+instead.
 @end defun
 
 @defun posn-area position
diff --git a/lisp/subr.el b/lisp/subr.el
index 0534585..94f6d01 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1142,24 +1142,29 @@ For a scroll-bar event, the result column is 0, and the row
 corresponds to the vertical position of the click in the scroll bar.
 POSITION should be a list of the form returned by the `event-start'
 and `event-end' functions."
-  (let* ((pair   (posn-x-y position))
-	 (window (posn-window position))
-	 (area   (posn-area position)))
+  (let* ((pair            (posn-x-y position))
+         (frame-or-window (posn-window position))
+         (frame           (if (framep frame-or-window)
+                              frame-or-window
+                            (window-frame frame-or-window)))
+         (window          (if (windowp frame-or-window)
+                              frame-or-window
+                            nil))
+         (area            (posn-area position)))
     (cond
-     ((null window)
+     ((null frame-or-window)
       '(0 . 0))
      ((eq area 'vertical-scroll-bar)
       (cons 0 (scroll-bar-scale pair (1- (window-height window)))))
      ((eq area 'horizontal-scroll-bar)
       (cons (scroll-bar-scale pair (window-width window)) 0))
      (t
-      (let* ((frame (if (framep window) window (window-frame window)))
-	     ;; FIXME: This should take line-spacing properties on
-	     ;; newlines into account.
-	     (spacing (when (display-graphic-p frame)
-			(or (with-current-buffer (window-buffer window)
-			      line-spacing)
-			    (frame-parameter frame 'line-spacing)))))
+      ;; FIXME: This should take line-spacing properties on
+      ;; newlines into account.
+      (let* ((spacing (when (display-graphic-p frame)
+                 (or (with-current-buffer (window-buffer window)
+                       line-spacing)
+                     (frame-parameter frame 'line-spacing)))))
 	(cond ((floatp spacing)
 	       (setq spacing (truncate (* spacing
 					  (frame-char-height frame)))))
diff --git a/lisp/textmodes/artist.el b/lisp/textmodes/artist.el
index 8a2383c..85d9410 100644
--- a/lisp/textmodes/artist.el
+++ b/lisp/textmodes/artist.el
@@ -4963,52 +4963,55 @@ The event, EV, is the mouse event."
     (artist-funcall init-fn x1 y1)
     (if (not artist-rubber-banding)
 	(artist-no-rb-set-point1 x1 y1))
-    (track-mouse
-      (while (or (mouse-movement-p ev)
-		 (member 'down (event-modifiers ev)))
-	(setq ev-start-pos (artist-coord-win-to-buf
-			    (posn-col-row (event-start ev))))
-	(setq x1 (car ev-start-pos))
-	(setq y1 (cdr ev-start-pos))
-
-	;; Cancel previous timer
-	(if timer
-	    (cancel-timer timer))
-
-	(if (not (eq initial-win (posn-window (event-start ev))))
-	    ;; If we moved outside the window, do nothing
-	    nil
-
-	  ;; Still in same window:
-	  ;;
-	  ;; Check if user presses or releases shift key
-	  (if (artist-shift-has-changed shift-state ev)
-
-	      ;; First check that the draw-how is the same as we
-	      ;; already have. Otherwise, ignore the changed shift-state.
-	      (if (not (eq draw-how
-			   (artist-go-get-draw-how-from-symbol
-			    (if (not shift-state) shifted unshifted))))
-		  (message "Cannot switch to shifted operation")
-
-		;; progn is "implicit" since this is the else-part
-		(setq shift-state (not shift-state))
-		(setq op          (if shift-state shifted unshifted))
-		(setq draw-how    (artist-go-get-draw-how-from-symbol op))
-		(setq draw-fn     (artist-go-get-draw-fn-from-symbol op))))
-
-	  ;; Draw the new shape
-	  (setq shape (artist-funcall draw-fn x1 y1))
-	  (artist-move-to-xy x1 y1)
-
-	  ;; Start the timer to call `draw-fn' repeatedly every
-	  ;; `interval' second
-	  (if (and interval draw-fn)
-	      (setq timer (run-at-time interval interval draw-fn x1 y1))))
-
-	;; Read next event
-	(setq ev (read-event))))
-
+    (unwind-protect
+        (track-mouse
+          (while (or (mouse-movement-p ev)
+                     (member 'down (event-modifiers ev)))
+            (setq ev-start-pos (artist-coord-win-to-buf
+                                (posn-col-row (event-start ev))))
+            (setq x1 (car ev-start-pos))
+            (setq y1 (cdr ev-start-pos))
+
+            ;; Cancel previous timer
+            (if timer
+                (cancel-timer timer))
+
+            (if (not (eq initial-win (posn-window (event-start ev))))
+                ;; If we moved outside the window, do nothing
+                nil
+
+              ;; Still in same window:
+              ;;
+              ;; Check if user presses or releases shift key
+              (if (artist-shift-has-changed shift-state ev)
+
+                  ;; First check that the draw-how is the same as we
+                  ;; already have. Otherwise, ignore the changed shift-state.
+                  (if (not (eq draw-how
+                               (artist-go-get-draw-how-from-symbol
+                                (if (not shift-state) shifted unshifted))))
+                      (message "Cannot switch to shifted operation")
+
+                    ;; progn is "implicit" since this is the else-part
+                    (setq shift-state (not shift-state))
+                    (setq op          (if shift-state shifted unshifted))
+                    (setq draw-how    (artist-go-get-draw-how-from-symbol op))
+                    (setq draw-fn     (artist-go-get-draw-fn-from-symbol op))))
+
+              ;; Draw the new shape
+              (setq shape (artist-funcall draw-fn x1 y1))
+              (artist-move-to-xy x1 y1)
+
+              ;; Start the timer to call `draw-fn' repeatedly every
+              ;; `interval' second
+              (if (and interval draw-fn)
+                  (setq timer (run-at-time interval interval draw-fn x1 y1))))
+
+            ;; Read next event
+            (setq ev (read-event))))
+      ;; Cleanup: get rid of any active timer.
+      (if timer
+          (cancel-timer timer)))
     ;; Cancel any timers
     (if timer
 	(cancel-timer timer))