Skip to content

X11 device doesn't handle destroy events correcly (PR#848)

2 messages · Thomas J Vogels, Peter Dalgaard

Original
Thomas J Vogels
# 
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:
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
Peter Dalgaard
# 
tov@ece.cmu.edu writes:
Heh. Quite probably, this is also the source of the FVWM trouble
reported earlier although that triggered an infinite loop which should
probably be guarded against by other means as well. 

Thanks for filing this (good to have as bug report so that we don't
forget - it's the sort of thing that often has to wait until someone
has a clear enough mind and sufficient stamina to try and get it
right.)