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

From: Daniel Koning <[email-masked]>
Date: Sat, 17 Jan 2015 23:47:29 -0600

martin rudalics <[email-masked]> writes:

>> Here's a patch that handles 1 through 3.
>
> I think your patch should go into Emacs 24.5.  Have you signed a FSF
> copyright form?  If not, please do that as soon as possible.

I've put in a request. I'll write back when I've received and returned
the form.

>> 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.
>
> `posnp' also looks strange in this regard.
>

It does indeed, and I added a line to check for frames.

> Please always use two spaces after the end of a sentence.

> Please use either
> (and (windowp frame-or-window) frame-or-window)
> or
> (when (windowp frame-or-window) frame-or-window)

Done and done.

> I'd expect it to be the selected frame but are we 100% sure?  Could this
> frame not still be the frame selected at the time mouse dragging started
> while the selected frame has changed under our feet?  Think of weird
> things like `focus-follows-mouse' and `mouse-autoselect-window'.  (This
> remark might be silly but I was too lazy to test its validity right now.)

Good eye. Upon testing it out, it does seem to be the case that the
event's end position will always reflect the originally selected
frame. I'll reword my commentary to reflect that.

That said, I don't *think* it's ever possible in practice to change the
selected frame in the middle of a drag event through user interaction
alone -- either implicitly or by a keypress bound to `other-frame'. I'm
basing this on having just tried it in several window systems on both OS
X and BSD, including with focus-follows-mouse behavior enabled (and with
the corresponding Emacs variable turned on). I was only able to test it
by means of an event-reading loop which programmatically called
`other-frame' after every down-mouse event.

>> +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.
>
> Hmmm...  here you use "frame with input focus".  This looks better but
> I'm still not entirely convinced.

Come to think of it, this is wrong either way, because the implication
of "the frame with input focus" is the frame with input focus at the
time of evaluation, whereas the position list reflects the state at the
time it was generated.

How about: "If @var{position} represents a location outside the frame
where the event was initiated, [...]"

>> +      (let* ((spacing (when (display-graphic-p frame)
>> +                 (or (with-current-buffer (window-buffer window)
>
> Here `window' is the selected window.  IMHO
>
> (frame-selected-window frame)
>
> sounds more accurate, given what I said about the selected frame above.

I haven't changed these lines except for taking them out of a
now-superfluous let*, but I agree that they look wrong. (Technically
`window' is either the window from the position list or nil. If it is
nil, though, `window-buffer' does return the buffer of the selected
window. The calculation should be made w/r/t the selected window of the
position list's frame, so I'll make that change.)

>> diff --git a/lisp/textmodes/artist.el b/lisp/textmodes/artist.el
>
> I didn't look into these but just trust your experience here.

This one looks like a major change because of all the diffed lines, but
that's just due to adding an extra layer of indentation. The only
semantic change is wrapping the `track-mouse' form in an `unwind-protect'.

Here's the revised patch.

diff --git a/doc/lispref/commands.texi b/doc/lispref/commands.texi
index 36c7445..6fdc8e2 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 frame that was
+initially selected.  In that case, the third element's position list
+contains that 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 initially selected frame, in
+which case the list contains that 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 @var{position}
+represents a location outside the frame where the event was initiated,
+return that frame instead.
 @end defun
 
 @defun posn-area position
diff --git a/lisp/subr.el b/lisp/subr.el
index 0534585..2eb293a 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1083,7 +1083,7 @@ The return value is a positive integer."
 
 (defun posnp (obj)
   "Return non-nil if OBJ appears to be a valid `posn' object."
-  (and (windowp (car-safe obj))
+  (and (or (windowp (car-safe obj)) (framep (car-safe obj)))
        (atom (car-safe (setq obj (cdr obj))))                ;AREA-OR-POS.
        (integerp (car-safe (car-safe (setq obj (cdr obj))))) ;XOFFSET.
        (integerp (car-safe (cdr obj)))))                     ;TIMESTAMP.
@@ -1142,24 +1142,28 @@ 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          (when (windowp frame-or-window) frame-or-window))
+         (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 (frame-selected-window frame))
+                              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))