From tov@ece.cmu.edu  Tue Feb 13 22:40:37 2001
From: tov@ece.cmu.edu (tov@ece.cmu.edu)
Date: Tue, 13 Feb 2001 23:40:37 +0100 (MET)
Subject: [Rd] X11 device doesn't handle destroy events correcly (PR#848)
Message-ID: <200102132240.XAA10562@pubhealth.ku.dk>


Hi, there are two problems in devX11.c.  The one is an undocumented
nuisance and the other isn't a bug until you try to embed an X11
device in another window (think tktoplevel() in tcltk package...).

Let's take a look at locator() first: Assuming you call locator() with
a current X11 device, this call is handled in X11_Locator
(src/unix/X11/devX11.c).  When you have one window open and call
locator, you have three options to quit the locator: click Button 1
(to locate), click Button 2 (to abort), or destroy the window.  The
later results in the error message: "Error in locator(1) : No graphics
device is active". That's OK, sort of.

This gets problematic when you open two windows:
> x11()  # assume that's device 2
> x11()  # assume that's device 3
> plot(1)
> locator(1)  # with device 3 == dev.cur()!

When you now destroy device 3 (where locator expects the click), you have
*no alternative but to close _all_ windows* to abort the locator.  The
locator continues to wait for mouse clicks in device 3 where of course
no further clicks can come from.  That's the nuisance part.

The second problem is that the device may try to plot() into a
non-existant window.  Assume that the window was destroyed by some
other part of the program.  The device (at some point in the future)
will receive a DestroyNotify event that handleEvent ignores.
Blissfully!  *No KillDevice is called* when the window is destroyed by
something other than the WM_DELETE_WINDOW mechanism!  This happens
when you embed the X11 device into another window/application.  For
starters, the device no longer receives WM_DELETE_WINDOW client
messages.  But it does receive a DestroyNotify event when the parent
(the container window) is destroyed.  HandleEvent should, well
_handle_ this _event_.

The patch below is a _suggestion_ how to solve both problems.  It is
against the current r-devel, 1.3, as of 2/13.  I believe this behavior
exists in version 1.2 as well and can be solved there the same way.
The patch is just meant as a convenient and concise way to describe
my suggestion.  I'd greatly appreciate feedback or other ideas!

Implementation:

-- the variable inclose is now in the device specific structure (why
   was it global?)  After the window is mapped, inclose is TRUE until
   the window is destroyed explicitly or a DestroyNotify event has been
   received.
-- the deviceSpecific pointer is set to NULL after the memory has been
   freed. This (side?) effect is used in X11_Locator to detect the
   closing of the current device  which _must_ lead to an error.
-- the if statement to handle events of type DestroyEvent is added to 
   handleEvent
-- when the user closes the device (dev.off()), the window is
   destroyed in X11_Close
-- when the user pushes the close button in the window frame, the
   window is destroyed in handleEvent and X11_Close is initiated
   (the WM_DELETE_WINDOW mechanism already in place.)
-- when the window is destroyed for another reason (e.g. the parent
   being destroyed), it is not destroyed again but X11_Close is still
   initiated.

Regards,
  -tom


--------------------------------------------------
*** devX11.c_orig	Tue Feb 13 15:32:24 2001
--- devX11.c	Tue Feb 13 17:24:04 2001
***************
*** 116,121 ****
--- 116,122 ----
      FILE *fp;				/* file for a bitmap device */
      int quality;			/* JPEG quality */
  
+     Rboolean inclose;                   /* TRUE if window is being closed */
  } x11Desc;
  
  
***************
*** 145,151 ****
  static Atom _XA_WM_PROTOCOLS, protocol;
  
  static Rboolean displayOpen = FALSE;
- static Rboolean inclose = FALSE;
  static int numX11Devices = 0;
  
  	/********************************************************/
--- 146,151 ----
***************
*** 619,632 ****
  	xd->windowHeight = event.xconfigure.height;
  	xd->resize = 1;
      }
      else if ((event.type == ClientMessage) &&
! 	     (event.xclient.message_type == _XA_WM_PROTOCOLS))
! 	if (!inclose && event.xclient.data.l[0] == protocol) {
! 	    XFindContext(display, event.xclient.window,
  			 devPtrContext, &temp);
  	    dd = (DevDesc *) temp;
! 	    KillDevice(dd);
  	}
  }
  
  static void R_ProcessEvents(void *data)
--- 619,652 ----
  	xd->windowHeight = event.xconfigure.height;
  	xd->resize = 1;
      }
+     else if (event.type == DestroyNotify) {
+         /* The window is being destroyed, kill the device, too. */
+         XFindContext(display, event.xdestroywindow.window,
+                      devPtrContext, &temp);
+         dd = (DevDesc *) temp;
+         xd = (x11Desc *) dd->deviceSpecific;
+         if (!xd->inclose) {
+             xd->inclose = TRUE;
+             KillDevice(dd);
+         }
+     }
      else if ((event.type == ClientMessage) &&
! 	     (event.xclient.message_type == _XA_WM_PROTOCOLS)) {
! 	if (event.xclient.data.l[0] == protocol) {
!             XFindContext(display, event.xclient.window,
  			 devPtrContext, &temp);
  	    dd = (DevDesc *) temp;
! 	    xd = (x11Desc *) dd->deviceSpecific;
! 	    if (!xd->inclose) {
! 		/* Dispatch destroy only once */
! 		xd->inclose = TRUE;
! 		XDestroyWindow(display, xd->window);
! 		XSync (display, 0);
! 		KillDevice(dd);
! 	    }
  	}
+     }
+ 
  }
  
  static void R_ProcessEvents(void *data)
***************
*** 1208,1213 ****
--- 1228,1234 ----
  		     ExposureMask | ButtonPressMask | StructureNotifyMask);
  	XMapWindow(display, xd->window);
  	XSync(display, 0);
+         xd->inclose = FALSE;
  
  	/* Gobble expose events */
  
***************
*** 1464,1476 ****
      x11Desc *xd = (x11Desc *) dd->deviceSpecific;
  
      if (xd->type == WINDOW) {
! 	/* process pending events */
! 	/* set block on destroy events */
! 	inclose = TRUE;
  	R_ProcessEvents((void*) NULL);
  
  	XFreeCursor(display, xd->gcursor);
- 	XDestroyWindow(display, xd->window);
  	XSync(display, 0);
      } else {
  	if (xd->npages && xd->type != XIMAGE) {
--- 1485,1506 ----
      x11Desc *xd = (x11Desc *) dd->deviceSpecific;
  
      if (xd->type == WINDOW) {
! 	/* There are three reasons to be here: */
! 	/* 1) The user clicked on a button in the window frame */
! 	/*    and we received a DELETE_WINDOW from the wm, */
! 	/* 2) Somebody destroyed our window directly, or if */
! 	/*    embedded, the parent is being destroyed, and so */
! 	/*    we got a DestroyNotify event, too, */
! 	/* 3) The user used dev.off() to get rid of us. Bye. */
! 
! 	if (!xd->inclose) {
! 	    xd->inclose = TRUE;
! 	    XDestroyWindow(display, xd->window);
! 	}
! 	/* process pending events (esp. reap events for this window) */
  	R_ProcessEvents((void*) NULL);
  
  	XFreeCursor(display, xd->gcursor);
  	XSync(display, 0);
      } else {
  	if (xd->npages && xd->type != XIMAGE) {
***************
*** 1517,1523 ****
      }
  
      free(xd);
!     inclose = FALSE;
  }
  
  	/********************************************************/
--- 1547,1553 ----
      }
  
      free(xd);
!     dd->deviceSpecific = NULL;
  }
  
  	/********************************************************/
***************
*** 1858,1866 ****
  		else
  		    done = 2;
  	    }
! 	}
! 	else
  	    handleEvent(event);
      }
      /* if it was a Button1 succeed, otherwise fail */
      return (done == 1);
--- 1888,1901 ----
  		else
  		    done = 2;
  	    }
! 	} else {
  	    handleEvent(event);
+ 	    if (!dd->deviceSpecific) {
+ 		/* Allow new active window to redraw */
+ 		R_ProcessEvents((void*)NULL);
+ 		error ("Device closed while waiting for input.\n");
+ 	    }
+         }
      }
      /* if it was a Button1 succeed, otherwise fail */
      return (done == 1);
--------------------------------------------------




--please do not edit the information below--

Version:
 platform = i686-pc-linux-gnu
 arch = i686
 os = linux-gnu
 system = i686, linux-gnu
 status = Under development (unstable)
 major = 1
 minor = 3.0
 year = 2001
 month = 02
 day = 12
 language = R

Search Path:
 .GlobalEnv, package:ctest, Autoloads, package:base


-- 
mailto:tov@ece.cmu.edu (Tom Vogels)   Tel: (412) 268-6638   FAX: -3204


-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-.-
r-devel mailing list -- Read http://www.ci.tuwien.ac.at/~hornik/R/R-FAQ.html
Send "info", "help", or "[un]subscribe"
(in the "body", not the subject !)  To: r-devel-request@stat.math.ethz.ch
_._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._._


[Rd] X11 device doesn't handle destroy events correcly (PR#848)
tov@ece.cmu.edu