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

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

[busk]

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. Confirmed this on three different machines
with different releases of the ubuntu gnu/linux distribution and three
different releases of emacs (22.2.1, 23.1.1 and 23.1.50), and this
misbehaviour does not exist in 22.2.1, at least. I have not been able
to test this on a bzr snapshot, however, so in theory this might
already have been discovered and fixed, I guess.


In GNU Emacs 23.1.1 (x86_64-pc-linux-gnu, GTK+ Version 2.18.3)
 of 2010-03-26 on crested, modified by Debian
Windowing system distributor `The X.Org Foundation', version 11.0.10604000
configured using `configure  '--build=x86_64-linux-gnu'
'--host=x86_64-linux-gnu' '--prefix=/usr' '--sharedstatedir=/var/lib'
'--libexecdir=/usr/lib' '--localstatedir=/var/lib'
'--infodir=/usr/share/info' '--mandir=/usr/share/man' '--with-pop=yes'
'--enable-locallisppath=/etc/emacs23:/etc/emacs:/usr/local/share/emacs/23.1/site-lisp:/usr/local/share/emacs/site-lisp:/usr/share/emacs/23.1/site-lisp:/usr/share/emacs/site-lisp:/usr/share/emacs/23.1/leim'
'--with-x=yes' '--with-x-toolkit=gtk' '--with-toolkit-scroll-bars'
'build_alias=x86_64-linux-gnu' 'host_alias=x86_64-linux-gnu'
'CFLAGS=-DDEBIAN -g -O2' 'LDFLAGS=-g' 'CPPFLAGS=''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: nil
  locale-coding-system: utf-8-unix
  default-enable-multibyte-characters: t

Major mode: Lisp Interaction

Minor modes in effect:
  display-time-mode: t
  global-hl-line-mode: t
  show-paren-mode: t
  sml-modeline-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  column-number-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
M-x r e p o <tab> r <tab> <return>

Recent messages:
Loading /etc/emacs/site-start.d/55ecb.el (source)...done
Loading /etc/emacs/site-start.d/65wl-beta.el (source)...done
/usr/bin/mail is not an executable.  Setting mail-interactive to t.
Loading /home/busk/elisp/color-theme-6.6.0/themes/color-theme-example.el
(source)...done
Loading /home/busk/elisp/color-theme-6.6.0/themes/color-theme-insp1.el
(source)...done
Loading /home/busk/elisp/color-theme-6.6.0/themes/color-theme-library.el
(source)...done
Loading /home/busk/.emacs.d/custom.el (source)...done
Starting Emacs daemon.
When done with this frame, type C-x 5 0
Making completion list...

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

[Daniel Koning]

busk <busk@lysator.liu.se> 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))

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

[martin rudalics]

Thank you very much, especially for the detailed explanation.

 > 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.

 > 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.

We could rename it to `posn-window-or-frame' and provide an alias.

 > 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.

 > +the second element of any mouse event in the same way. However, the
                                                          ^
Please always use two spaces after the end of a sentence.

 > +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.

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.)

 > +location outside the boundaries of the selected frame, in which case
 > +the list contains the selected frame in place of a window.

Same as before.

 > +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.

 > +         (window          (if (windowp frame-or-window)
 > +                              frame-or-window
 > +                            nil))

Please use either

(and (windowp frame-or-window) frame-or-window)

or

(when (windowp frame-or-window) frame-or-window)

here.

 > +      (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.

 > diff --git a/lisp/textmodes/artist.el b/lisp/textmodes/artist.el

I didn't look into these but just trust your experience here.

Thanks again, martin

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

[Daniel Koning]

martin rudalics <rudalics@gmx.at> 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))

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

[martin rudalics]

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

Thanks.  It usually takes some time and it might be a good idea to
complain after a few weeks in order to speed things up.

 >> `posnp' also looks strange in this regard.
 >>
 >
 > It does indeed, and I added a line to check for frames.

No - this might be dangerous for now.  Suppose we have callers that
relied on `posnp' to return nil in that case.  They would all of sudden
have to deal with the fact that they get a frame now, so more or less we
could reintroduce the problem you try to fix presently.  Please take
this out for the moment but state in the doc-string that `posnp' returns
nil if the first element of OBJ is a frame.  Later on we can change this
as you did and see what happens.

 > 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.

Fine.  I suppose it would defeat the purpose of mouse dragging if the
selected frame could ever change in between but wanted to make sure.
Here I was not able to produce a frame select event either.

 > 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, [...]"

Good.

 >> (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.)

OK.  Given your observations above it should not matter.

 >>> 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'.

Ahh.  I didn't notice.  In that case we could commit your patch as a
tiny change.

 > Here's the revised patch.

Looks good.  Please fix the `posnp' thingy I mentioned above, add some
ChangeLog entries and resend the patch as an attachment. If there are no
objections I'll commit it then to the emacs-24 branch.

Thanks, martin

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

[Daniel Koning]

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

martin rudalics <rudalics@gmx.at> writes:

>> I added a line to check for frames.
>
> No - this might be dangerous for now.  Suppose we have callers that
> relied on `posnp' to return nil in that case.  They would all of sudden
> have to deal with the fact that they get a frame now, so more or less we
> could reintroduce the problem you try to fix presently.  Please take
> this out for the moment but state in the doc-string that `posnp' returns
> nil if the first element of OBJ is a frame.  Later on we can change this
> as you did and see what happens.

Okay, but I think this should be a reasonably high priority for the
maintainers of this part of the lisp tree. Aren't there likely to be
quite a few undiscovered bugs, some perhaps quite destructive, that
result from following the behavior as it was documented, just as there
are (presumably) places where new bugs would manifest if `posnp' were
brought in line with its advertised behavior? In any case, I've appended
a FIXME comment in addition to revising the docstring.

I added the log entry to the highest-level ChangeLog file, since I
edited files in lisp/ and doc/ (even though the doc/ changes were only
in reference to functionality implemented under lisp/). Is that right?

Daniel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Fix for #6130 --]
[-- Type: text/x-patch, Size: 9833 bytes --]

diff --git a/ChangeLog b/ChangeLog
index 309b04f..53d7bb4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,14 @@
+2015-01-20  Daniel Koning  <dk@danielkoning.com>
+
+	Prevent artist-mode from creating runaway timers.
+	* lisp/textmodes/artist.el: Cancel timers if an error occurs
+	during continuous drawing.
+	* lisp/subr.el: Make `posn-col-row' work with all mouse position
+	objects. Correct docstring of `posnp'.
+	* doc/lispref/commands.texi: Describe actual range of values that
+	mouse position objects can have.
+	Fixes: bug#6130
+
 2015-01-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	Give up on -Wsuggest-attribute=const
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..b37d17f 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -1082,7 +1082,10 @@ The return value is a positive integer."
 ;;;; Extracting fields of the positions in an event.
 
 (defun posnp (obj)
-  "Return non-nil if OBJ appears to be a valid `posn' object."
+  "Return non-nil if OBJ appears to be a valid `posn' object that specifies a window. If OBJ is a valid `posn' object, but specifies a frame rather than a window, return nil."
+  ;; FIXME: Correct the behavior of this function so that all valid
+  ;; `posn' objects are recognized, after updating other code that
+  ;; depends on its present behavior.
   (and (windowp (car-safe obj))
        (atom (car-safe (setq obj (cdr obj))))                ;AREA-OR-POS.
        (integerp (car-safe (car-safe (setq obj (cdr obj))))) ;XOFFSET.
@@ -1142,24 +1145,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))

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

[martin rudalics]

 > Okay, but I think this should be a reasonably high priority for the
 > maintainers of this part of the lisp tree. Aren't there likely to be
 > quite a few undiscovered bugs, some perhaps quite destructive, that
 > result from following the behavior as it was documented, just as there
 > are (presumably) places where new bugs would manifest if `posnp' were
 > brought in line with its advertised behavior? In any case, I've appended
 > a FIXME comment in addition to revising the docstring.

Thanks.  I slightly tweaked the doc-string of `posnp' to make its first
line fit.  We can change the behavior of `posnp' for Emacs 25 as soon as
you received your confirmation from the FSF.

 > I added the log entry to the highest-level ChangeLog file, since I
 > edited files in lisp/ and doc/ (even though the doc/ changes were only
 > in reference to functionality implemented under lisp/). Is that right?

Not really.  But the way of writing ChangeLog entries will change in the
near future anyway so there's not much use to tell you the precise
details.  I used your entry to create more appropriate ones.  I pushed
your changes to the Emacs-24 branch as commit

    4c09e3a..3ea1b31  emacs-24 -> emacs-24

They should appear on trunk/master with the next merge.  Please have a
look, my experience with git is still rather poor.  If you think I've
done everything right, please close the bug.

Thanks, martin

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

[Stefan Monnier]

The Right Thing to do, AFAICT, is for posnp to return non-nil also when
the `car' is a frame.

For posn-window, the question is more tricky.  I don't think it should
ever return a frame, since its name makes it clear it returns a window.

So we should either make it return nil when the car is a frame, or
signal an error, or return the frame's selected-window, and/or deprecate
it (in favor of a new posn-window-or-frame).

Can someone look at its various users to see what would be best?


        Stefan

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

[martin rudalics]

 > The Right Thing to do, AFAICT, is for posnp to return non-nil also when
 > the `car' is a frame.

If the caller of `posnp' is not prepared to deal with a frame but used
`posnp' to check that the position is "valid", it currently will not
fail.  If we have `posnp' return t for a frame as well, it will fail.
And as Daniel has shown, such a failure can be quite annoying in its
consequences.  It probably could have been avoided if artist had used
`posnp'.  So at least for Emacs 24.5 I would rather not change the
current behavior.  I would change it for Emacs 25.

 > For posn-window, the question is more tricky.  I don't think it should
 > ever return a frame, since its name makes it clear it returns a window.

Agreed.  But `posn-window' is just a mnemonic for "get me the first
element of a position".

 > So we should either make it return nil when the car is a frame, or
 > signal an error, or return the frame's selected-window,

The latter would be certainly wrong.  We are NOT in that window.

 > and/or deprecate
 > it (in favor of a new posn-window-or-frame).

That's what I had in mind.

 > Can someone look at its various users to see what would be best?

martin

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

[Stefan Monnier]

>> The Right Thing to do, AFAICT, is for posnp to return non-nil also when
>> the `car' is a frame.
> If the caller of `posnp' is not prepared to deal with a frame but used
> `posnp' to check that the position is "valid", it currently will not
> fail.

Yes, I understand.  We can't always Do The Right Thing.  But that's
still The Right Thing.

> `posnp'.  So at least for Emacs 24.5 I would rather not change the
> current behavior.

That's for sure, yes.

> I would change it for Emacs 25.

We violently agree.

>> For posn-window, the question is more tricky.  I don't think it should
>> ever return a frame, since its name makes it clear it returns a window.
> Agreed.  But `posn-window' is just a mnemonic for "get me the first
> element of a position".

Could you take a look at the existing callers to see how they would
react to receiving nil (instead of a frame), or a frame (as now), or an
error (instead of a frame)?

>> So we should either make it return nil when the car is a frame, or
>> signal an error, or return the frame's selected-window,
> The latter would be certainly wrong.  We are NOT in that window.

Indeed.

>> and/or deprecate it (in favor of a new posn-window-or-frame).
> That's what I had in mind.

But that only makes sense if most callers of posn-window can (or would
like to) also handle a frame.


        Stefan

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

[martin rudalics]

 > Could you take a look at the existing callers to see how they would
 > react to receiving nil (instead of a frame), or a frame (as now), or an
 > error (instead of a frame)?

Daniel in his first post wrote that

   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.

so I think this has been taken care of already.

 >>> and/or deprecate it (in favor of a new posn-window-or-frame).
 >> That's what I had in mind.
 >
 > But that only makes sense if most callers of posn-window can (or would
 > like to) also handle a frame.

Then we have still another choice: Provide a function `posn-frame' and
have `posn-window' return nil when the first element is a frame.  This
would backfire for people who would like `posn-window' to always return
the first element of a position.

martin

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

[Stefan Monnier]

> Daniel in his first post wrote that
>   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.
> so I think this has been taken care of already.

To be honest, I don't know what that means.  IIUC it means that most of
the code doesn't care if posn-window sometimes returns a frame.

It's wrong for posn-window to return a frame.  So are there callers that
actually rely on this wrong behavior?  Are there callers where returning
nil instead of a frame would be a problem?  Are there callers where
signaling an error instead of returning a frame would be a problem?

> This would backfire for people who would like `posn-window' to always
> return the first element of a position.

That's OK, in the sense that we don't care if people's feelings
are hurt.  But if it breaks existing packages it's more problematic.


        Stefan

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

[martin rudalics]

 > It's wrong for posn-window to return a frame.

Is it right for it to return nil?  There seems to be no particular
restriction on what a "mouse position list" as returned by `event-start'
and `event-end' has to contain.  Many callers simply use the selected
window or the window buffer of the selected window when the return value
of `posn-window' is nil.

 > So are there callers that
 > actually rely on this wrong behavior?  Are there callers where returning
 > nil instead of a frame would be a problem?  Are there callers where
 > signaling an error instead of returning a frame would be a problem?

`handle-delete-frame' seems to be the only function that expects
`posn-window' to return a frame (unconditionally, BTW).  I don't
understand `handle-delete-frame' but it hardly will cause problems when
it gets nil or an error.

I didn't check for occurrences of things like "(nth 0 position".

 >> This would backfire for people who would like `posn-window' to always
 >> return the first element of a position.
 >
 > That's OK, in the sense that we don't care if people's feelings
 > are hurt.  But if it breaks existing packages it's more problematic.

If we really cared, we'd probably have to write something like

(defsubst posn-window (position)
   "Return the window in POSITION.
POSITION should be a list of the form returned by the `event-start'
and `event-end' functions."
   (if (window-live-p (nth 0 position))
       (nth 0 position)
     'none))

martin

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

[Eli Zaretskii]

> Date: Fri, 23 Jan 2015 09:26:56 +0100
> From: martin rudalics <rudalics@gmx.at>
> Cc: 6130@debbugs.gnu.org, busk <busk@lysator.liu.se>,
> 	Daniel Koning <dk@danielkoning.com>
> 
>  > So are there callers that
>  > actually rely on this wrong behavior?  Are there callers where returning
>  > nil instead of a frame would be a problem?  Are there callers where
>  > signaling an error instead of returning a frame would be a problem?
> 
> `handle-delete-frame' seems to be the only function that expects
> `posn-window' to return a frame (unconditionally, BTW).

It's not the only one, AFAICS.  Any function that calls x-popup-menu
with a position constructed from what posn-window returns also depends
on that, albeit indirectly.  See, for example, mouse-select-buffer in
msb.el and popup-menu-normalize-position in menu-bar.el.

Other functions provide useful features based on this "misfeature".
One is handle-delete-frame already mentioned above; in that case, the
mouse click that deletes the frame is always on the frame, not on any
window.  Another user of this is mouse-buffer-menu in mouse.el.

> I don't understand `handle-delete-frame' but it hardly will cause
> problems when it gets nil or an error.

??? How can this support deleting a frame by clicking on some of the
frame's decorations?

>  > That's OK, in the sense that we don't care if people's feelings
>  > are hurt.  But if it breaks existing packages it's more problematic.

It sounds like it's the latter.

According to my grepping, most users of posn-window pass the result to
select-window or with-selected-window or window-buffer, and will
certainly bomb if the object is not a window.  Some of them presumably
already hit this problem, because they defend themselves against such
a calamity (example: dframe.el).

Btw, I'm not sure I understand why you said:

>  > It's wrong for posn-window to return a frame.

Can you explain why it's wrong?  If this is just about insufficient
documentation and people's surprise when they see a frame coming out
of that, then we could improve the docs.

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

[martin rudalics]

 >> `handle-delete-frame' seems to be the only function that expects
 >> `posn-window' to return a frame (unconditionally, BTW).
 >
 > It's not the only one, AFAICS.  Any function that calls x-popup-menu
 > with a position constructed from what posn-window returns also depends
 > on that, albeit indirectly.

Yes.  In these cases we'd probably pop up the menu at a position
calculated from the upper left corner of the selected window.

 > See, for example, mouse-select-buffer in
 > msb.el and popup-menu-normalize-position in menu-bar.el.

IIUC `popup-menu-normalize-position' relies on `posnp' so it would
already fail now with a frame.

 > Other functions provide useful features based on this "misfeature".
 > One is handle-delete-frame already mentioned above; in that case, the
 > mouse click that deletes the frame is always on the frame, not on any
 > window.  Another user of this is mouse-buffer-menu in mouse.el.

I suppose that

       (select-window
        (if (framep window) (frame-selected-window window)
          window))

would select the `frame-selected-window' of the selected frame.

 >> I don't understand `handle-delete-frame' but it hardly will cause
 >> problems when it gets nil or an error.
 >
 > ??? How can this support deleting a frame by clicking on some of the
 > frame's decorations?

It wouldn't.  We'd have to use `posn-frame' here.  But I fail to
understand what happens when we call this with a window in EVENT.  Or
don't we ever?

 >>   > It's wrong for posn-window to return a frame.
 >
 > Can you explain why it's wrong?  If this is just about insufficient
 > documentation and people's surprise when they see a frame coming out
 > of that, then we could improve the docs.

We've hidden the semantics of this function from the users for very long
time.  It's not easy to get out of this situation without compromises.

martin

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

[Stefan Monnier]

>>> > It's wrong for posn-window to return a frame.
>> Can you explain why it's wrong?

If you take a step back and read my sentence you'll see it's pretty
obvious: since it says "window" it shouldn't return a "frame".


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  6130@debbugs.gnu.org,  busk@lysator.liu.se,  dk@danielkoning.com
> Date: Fri, 23 Jan 2015 16:05:33 -0500
> 
> >>> > It's wrong for posn-window to return a frame.
> >> Can you explain why it's wrong?
> 
> If you take a step back and read my sentence you'll see it's pretty
> obvious: since it says "window" it shouldn't return a "frame".

We do that all the time.  For example:

  (display-graphic-p &optional DISPLAY)

  Return non-nil if DISPLAY is a graphic display.
  [...]
  DISPLAY can be a display name, a frame, or nil (meaning the selected
  frame's display).

In polymorphic interfaces, the alternative is to say
DISPLAY-OR-FRAME-OR-... which is too tedious.

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

[Daniel Koning]

Eli Zaretskii <eliz@gnu.org> writes:

> We do that all the time.  For example:
>
>   (display-graphic-p &optional DISPLAY)
>
>   Return non-nil if DISPLAY is a graphic display.
>   [...]
>   DISPLAY can be a display name, a frame, or nil (meaning the selected
>   frame's display).

From a user perspective, there is a crucial distinction between these
two examples of ambiguous naming.

When an argument is called DISPLAY even though it can be other things
besides a display, the function still works in every case that its name
and its arguments' names suggest that it should work.

But when the return value is called WINDOW when it might not be one,
then it becomes a source of potential error merely to rely on the
function to behave as its name suggests. Sure, you can write
documentation to warn users about the pitfall, but misleading naming
conventions are one of the factors that cause people to write mistaken
documentation. (Cf. the documentation of `posn-window' until a few days
ago!)

> In polymorphic interfaces, the alternative is to say
> DISPLAY-OR-FRAME-OR-... which is too tedious.

If that's the main problem with assigning accurate names in polymorphic
contexts -- that the names end up too long when you include every
possible type -- how about defining a supertype which includes (in this
case) both windows and frames?  That may seem cumbersome, but realize
that this is effectively what is already happening, insofar as "window"
is being coerced into serving as metonymy for the entire class of
mouse-position containers.

Daniel

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

[Eli Zaretskii]

> From: Daniel Koning <dk@danielkoning.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  rudalics@gmx.at,  6130@debbugs.gnu.org,  busk@lysator.liu.se
> Date: Fri, 23 Jan 2015 15:52:54 -0600
> 
> When an argument is called DISPLAY even though it can be other things
> besides a display, the function still works in every case that its name
> and its arguments' names suggest that it should work.
> 
> But when the return value is called WINDOW when it might not be one,
> then it becomes a source of potential error merely to rely on the
> function to behave as its name suggests. Sure, you can write
> documentation to warn users about the pitfall, but misleading naming
> conventions are one of the factors that cause people to write mistaken
> documentation. (Cf. the documentation of `posn-window' until a few days
> ago!)

I agree that the documentation was (or maybe still is) in the need of
improvement (documentation of events is quite obscure, to tell the
truth, and was like that for a very long time).  But I fail to see the
crucial difference between these two examples.  In any case, the
example I brought up was just the first thing that popped up in my
mind, you can find plenty of examples of the other kind as well.

IOW, in Emacs one must read the documentation before they believe the
literal meaning of an argument's name.

> If that's the main problem with assigning accurate names in polymorphic
> contexts -- that the names end up too long when you include every
> possible type -- how about defining a supertype which includes (in this
> case) both windows and frames?

Try proposing one, maybe we will adopt it.  However, we have bad
experience with terminology that only Emacs uses, or, worse, where the
Emacs meaning is different from that of the rest of the world.  So I'm
not sure this path is better.

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

[martin rudalics]

 > IOW, in Emacs one must read the documentation before they believe the
 > literal meaning of an argument's name.

Unfortunately, in the case at hand the documentation was wrong and
misleading as most uses of `posn-window' still reflect.

martin

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

[Eli Zaretskii]

> Date: Sat, 24 Jan 2015 10:08:45 +0100
> From: martin rudalics <rudalics@gmx.at>
> CC: monnier@iro.umontreal.ca, 6130@debbugs.gnu.org, busk@lysator.liu.se
> 
>  > IOW, in Emacs one must read the documentation before they believe the
>  > literal meaning of an argument's name.
> 
> Unfortunately, in the case at hand the documentation was wrong and
> misleading

I agree.

> as most uses of `posn-window' still reflect.

I'm not sure.  It could be the case that most of those uses will never
get a frame from posn-window, in which case there's nothing wrong with
their current code.  The only way to tell is by analyzing the events
that are input to those code fragments.

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

[Johan Busk Eriksson]

I can't seem to replicate the bug anymore in 24.5, so I'll assume it's
been fixed without being closed.

To be honest I kind of forgot about filing the bug between 2010 and
when the replies started coming in 2015, and didn't really bother
seeing if it persisted in the last few releases.

/Johan