[AC21.5R21.4] Draft fix for metacity maximization bug

Stephen J. Turnbull stephen at xemacs.org
Sat May 6 13:48:24 EDT 2006


APPROVE COMMIT 21.5 RECOMMEND 21.4

Vin, there's a bunch of "future work" cruft here, so I don't
necessarily recommend this patch as-is.  Also, I haven't actually
checked if it applies, and the backwards-compatibility variable might
be a little bogus in the stable line.  Let me know what, if anything
of this you want.

Thanks to Glynn Clements, without whose discussion I never would have
figured this out, to the good folks at Sun, HP, etc, who funded Real
Documentation that Actually Helps, and to the various users who kept
reporting the issue even after we thought we (mostly) had it licked.

I provided an earlier patch, about which I wrote:

    sjt> It is possible to inhibit this, with no huge ill-effect on
    sjt> XEmacs functionality.  I'm supplying a patch, with this
    sjt> action controlled from Lisp by the variable `wedge-metacity',
    sjt> defaulting to nil.

This patch is *superseded* by the following patch, which may not apply
to 21.4 (sorry), and if you applied the earlier patch you'll have to
revert it (sorry again).  If you can apply it and are willing to test
it, it should "just work", no need to fiddle with anything.  I will
work up a 21.4 patch per Vin's comments (if any) within a day or so.
And it's being committed to 21.5 immediately.

If you run with the patch and have minor problems (but enough that
you're willing to avoid maximizing under metacity et al), you can
restore the old buggy behavior with (setq wedge-metacity t).

I'm pretty sure this patch is "right", although I haven't been able to
test with metacity.

(1) The Xt conventions say "don't call geometry request functions from
the resize callback".  This patch moves the request from a resize
callback into a function explicitly intended to change size.  The
theory is right.

(2) With the old behavior, the resize callback called itself
recursively, at least once.  Now it doesn't.

(3) The fluxbox window manager now gives "true maximization" where it
used to flash to the true maximum, then drop back to the largest
cell-oriented window.


Index: src/ChangeLog
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/ChangeLog,v
retrieving revision 1.954
diff -u -r1.954 ChangeLog
--- src/ChangeLog	6 May 2006 08:46:38 -0000	1.954
+++ src/ChangeLog	6 May 2006 17:26:00 -0000
@@ -0,0 +1,26 @@
+2006-04-30  Stephen J. Turnbull  <stephen at xemacs.org>
+
+	Move geometry management from EmacsFrameResize to x_set_frame_size.
+	Should fix "metacity maximization" bug.
+	Provide (deprecated, temporary) backward compatibility option.
+
+	* EmacsFrame.c (EmacsFrameResize):
+	* frame-x.c (x_set_frame_size):
+	Move call of ChangeEmacsManagerSize.
+	Don't bogusly notify WM about size changes the WM asked for.
+
+	* console-x.c (wedge-metacity): New builtin Boolean Lisp variable.
+	* console-x-impl.h (wedge_metacity): Declare C variable.
+	* console-x.c (vars_of_console_x): New function to initialize it.
+	* symsinit.h (vars_of_console_x): declare it.
+	* emacs.c (main_1): Call vars_of_console_x.
+
+	* EmacsFrameP.h (struct EmacsFrame):
+	* EmacsManager.c (ChangeEmacsManagerSize):
+	* EmacsShell-sub.c (SuperClassRootGeometryManager):
+	* console-x-impl.h (wedge_metacity):
+	Various comments, some improved documentation, mostly sad comments
+	on the state of the art of Xt programming.
+
+	* frame-x.c (defi): #undef it.  (Random code cleanliness.)
+

Index: src/EmacsFrame.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/EmacsFrame.c,v
retrieving revision 1.28
diff -u -r1.28 EmacsFrame.c
--- src/EmacsFrame.c	25 Oct 2005 11:16:20 -0000	1.28
+++ src/EmacsFrame.c	6 May 2006 17:26:00 -0000
@@ -360,19 +360,32 @@
   pixel_to_char_size (f, ew->core.width, ew->core.height, &columns, &rows);
   change_frame_size (f, rows, columns, 0);
 
-  /* Now we tell the EmacsShell that we've changed the size of the non-fixed
-     portion of the frame.  Note that, if we the resize occurred as a result
-     of EmacsFrameSetCharSize(), this information will be stored twice.
-     This is not a big deal, as storing this information doesn't actually
-     do anything until the next resize. */
-  if (FRAME_X_TOP_LEVEL_FRAME_P (f))
-    x_wm_set_variable_size (FRAME_X_SHELL_WIDGET (f), columns, rows);
+  /* The code below is just plain wrong.  If the EmacsShell or EmacsManager
+     needs to know, they should just ask.  If needed information is being
+     updated here, then we should set a dirty flag and have it updated on an
+     as-needed basis.
+     For now, conditionalize so people can get work done if this breaks
+     something. */
+  if (wedge_metacity)		/* cf. x_set_frame_size */
+    {
+      /* Now we tell the EmacsShell that we've changed the size of the
+	 non-fixed portion of the frame.  Note that, if the resize occurred
+	 as a result of EmacsFrameSetCharSize(), this information will be
+	 stored twice.  This is not a big deal, as storing this information
+	 doesn't actually do anything until the next resize. */
+      if (FRAME_X_TOP_LEVEL_FRAME_P (f))
+	x_wm_set_variable_size (FRAME_X_SHELL_WIDGET (f), columns, rows);
 
-  /* Kick the manager so that it knows we've changed size. */
-  req.request_mode = 0;
-  XtQueryGeometry (FRAME_X_CONTAINER_WIDGET (f), &req, &repl);
-  EmacsManagerChangeSize (FRAME_X_CONTAINER_WIDGET (f), repl.width,
-			  repl.height);
+      /* Kick the manager so that it knows we've changed size.
+	 #### No, no, no!  If this does anything at all, it will involve
+	 changing the manager's size.  That's not something that a child
+	 widget should initialize as part of a purely informational
+	 method!! */
+      req.request_mode = 0;
+      XtQueryGeometry (FRAME_X_CONTAINER_WIDGET (f), &req, &repl);
+      EmacsManagerChangeSize (FRAME_X_CONTAINER_WIDGET (f),
+			      repl.width, repl.height);
+    }
 }
 
 static Boolean
Index: src/EmacsFrameP.h
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/EmacsFrameP.h,v
retrieving revision 1.7
diff -u -r1.7 EmacsFrameP.h
--- src/EmacsFrameP.h	4 May 2003 02:34:35 -0000	1.7
+++ src/EmacsFrameP.h	6 May 2006 17:26:00 -0000
@@ -65,13 +65,16 @@
   Boolean	iconic;			/* whether this frame is iconic */
 
   /* The rest of this is crap and should be deleted.
+     #### Comments that start with + are fields that actually get referred 
+     to somewhere aside from the init function.
+     I guess the "crap" has mostly moved to specifiers?
    */
   Boolean	minibuffer;	/* 0: normal frames with minibuffers.
 				 * 1: frames without minibuffers
 				 * 2: minibuffer only. */
   Boolean	unsplittable;	/* frame can only have one window */
 
-  int		internal_border_width;	/* internal borders */
+  int		internal_border_width;	/* + internal borders */
   int		scrollbar_width;	/* width of frame vertical sb's */
   int		scrollbar_height;	/* height of frame horizontal sb's */
   int		top_toolbar_height;	/* height of top toolbar */
@@ -82,9 +85,9 @@
   int		bottom_toolbar_border_width;	/* ... of bottom toolbar */
   int		left_toolbar_border_width;	/* ... of left toolbar */
   int		right_toolbar_border_width;	/* ... of right toolbar */
-  Dimension	toolbar_shadow_thickness;
+  Dimension	toolbar_shadow_thickness;	/* + of shadows */
   unsigned char scrollbar_placement;
-  int		interline;		/* skips between lines */
+  int		interline;		/* + skips between lines */
 
   XFontStruct*	font;			/* font */
   Pixel		foreground_pixel;	/* foreground */
@@ -97,7 +100,7 @@
   int		bell_volume;		/* how loud is beep */
 
   Boolean	menubar_p;		/* initially show a menubar? */
-  Boolean	initially_unmapped;	/* inhibit initial window mapping */
+  Boolean	initially_unmapped;	/* + inhibit initial window mapping */
   Boolean	use_backing_store;	/* backing store for menubar & ew? */
 
   Dimension     preferred_width;        /* if non-zero, preferred size for */
Index: src/EmacsManager.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/EmacsManager.c,v
retrieving revision 1.7
diff -u -r1.7 EmacsManager.c
--- src/EmacsManager.c	22 Dec 2004 10:59:09 -0000	1.7
+++ src/EmacsManager.c	6 May 2006 17:26:00 -0000
@@ -241,6 +241,12 @@
   if (height == 0)
     height = w->core.height;
 
+  /* #### AFAICT this gets called in two places.  One is in ChangeManaged(),
+     above.  The other is in EmacsFrameResize().  Perhaps ChangeManaged()
+     should initiate resize requests, but EmacsFrameResize() should not.
+     Unfortunately, I've tried making this conditional on whether we're
+     called from EmacsFrameResize() or not, but that results in an infloop
+     via the callback to x_layout_widgets() in Resize().  Whee! */
   /* do nothing if we're already that size */
   if (w->core.width != width || w->core.height != height)
     {
Index: src/EmacsShell-sub.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/EmacsShell-sub.c,v
retrieving revision 1.7
diff -u -r1.7 EmacsShell-sub.c
--- src/EmacsShell-sub.c	24 Jan 2005 23:33:46 -0000	1.7
+++ src/EmacsShell-sub.c	6 May 2006 17:26:00 -0000
@@ -278,7 +278,8 @@
   GenericClassExtRec *gcer;
 
   /* find the shell extension record that specifies the
-     root geometry manager method */
+     root geometry manager method
+     #### We could use XtGetClassExtension here. */
   for (gcer = (GenericClassExtRec *) swc->shell_class.extension;
        gcer;
        gcer = (GenericClassExtRec *) gcer->next_extension)
@@ -287,6 +288,9 @@
 	break;
     }
 
+  /* #### The R11.6.4 Xt specification says if we don't find NULLQUARK here,
+     we should assume root_geometry_manager = XtInheritRootGeometryManager.
+     Is that actually callable? */
   if (!gcer)
     ABORT ();
 
Index: src/console-x-impl.h
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/console-x-impl.h,v
retrieving revision 1.5
diff -u -r1.5 console-x-impl.h
--- src/console-x-impl.h	26 Nov 2005 11:46:07 -0000	1.5
+++ src/console-x-impl.h	6 May 2006 17:26:00 -0000
@@ -40,6 +40,8 @@
 
 DECLARE_CONSOLE_TYPE (x);
 
+extern int wedge_metacity;
+
 struct x_device
 {
 #ifdef NEW_GC
@@ -234,7 +236,8 @@
 
 /* The maximum number of widgets that can be displayed above the text
    area at one time.  Currently no more than 3 will ever actually be
-   displayed (menubar, psheet, debugger panel). */
+   displayed (menubar, psheet, debugger panel).
+   #### Are "psheet" and "debugger panel" relevant any more? */
 #define MAX_CONCURRENT_TOP_WIDGETS 8
 
 struct x_frame
@@ -243,12 +246,17 @@
   struct lrecord_header header;
 #endif /* NEW_GC */
 
-  /* The widget of this frame.  This is an EmacsShell or an
-     ExternalShell. */
+  /* The widget of this frame.
+     This is an EmacsShell or an ExternalShell.
+     It negotiates with the window manager or containing app on behalf of
+     the container widget.  Should be (but isn't) invisible to Emacs. */
   Widget widget;
 
   /* The parent of the EmacsFrame, the menubar, and the scrollbars.
-     This is an EmacsManager. */
+     This is an EmacsManager.
+     It is responsible for managing the geometry of the frame.  This is what
+     Emacs mostly talks to.  Anything that affects its geometry will be
+     reflected in the Shell widget, and thus cause WM interaction. */
   Widget container;
 
   /* The widget of the menubar, of whatever widget class it happens to be. */
Index: src/console-x.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/console-x.c,v
retrieving revision 1.16
diff -u -r1.16 console-x.c
--- src/console-x.c	17 Dec 2005 19:47:02 -0000	1.16
+++ src/console-x.c	6 May 2006 17:26:00 -0000
@@ -41,6 +41,8 @@
 
 DEFINE_CONSOLE_TYPE (x);
 
+int wedge_metacity;	/* nonzero means update WM_HINTS always */
+
 extern void x_has_keysym (KeySym, Lisp_Object, int);
 
 static int
@@ -399,6 +401,22 @@
   CONSOLE_HAS_METHOD (x, perhaps_init_unseen_key_defaults);
 }
 
+
+void
+vars_of_console_x (void)
+{
+  DEFVAR_BOOL ("wedge-metacity", &wedge_metacity /*
+When non-nil, frame geometry management is backward-compatible.
+This is known to create inflooping window jitter in metacity, et al.
+It also does not conform to Xt conventions for geometry management.
+Specifically, all frame resizes, XEmacs-initiated or not, update WM_HINTS.
+Furthermore, geometry changes occur in the widget resize method.
+
+The default is nil.  This probably gives correct behavior regardless of the
+window manager used.
+This variable is deprecated and will be removed.
+*/ );
+}
 
 void
 reinit_console_type_create_x (void)
Index: src/emacs.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/emacs.c,v
retrieving revision 1.166
diff -u -r1.166 emacs.c
--- src/emacs.c	25 Apr 2006 14:02:08 -0000	1.166
+++ src/emacs.c	6 May 2006 17:26:01 -0000
@@ -2186,6 +2186,7 @@
 #ifdef HAVE_BALLOON_HELP
       vars_of_balloon_x ();
 #endif
+      vars_of_console_x ();
       vars_of_device_x ();
 #ifdef HAVE_X_DIALOGS
       vars_of_dialog_x ();
Index: src/frame-x.c
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/frame-x.c,v
retrieving revision 1.72
diff -u -r1.72 frame-x.c
--- src/frame-x.c	26 Nov 2005 11:46:08 -0000	1.72
+++ src/frame-x.c	6 May 2006 17:26:01 -0000
@@ -507,6 +507,7 @@
   defi(Qtop, XtNy);
 
 #undef def
+#undef defi
 }
 
 static Lisp_Object
@@ -2307,6 +2308,17 @@
 x_set_frame_size (struct frame *f, int cols, int rows)
 {
   EmacsFrameSetCharSize (FRAME_X_TEXT_WIDGET (f), cols, rows);
+
+  if (!wedge_metacity)		/* cf. EmacsFrameResize */
+    {
+      /* Kick the manager so that it knows we've changed size. */
+      XtWidgetGeometry req, repl;
+      req.request_mode = 0;
+      XtQueryGeometry (FRAME_X_CONTAINER_WIDGET (f), &req, &repl);
+      EmacsManagerChangeSize (FRAME_X_CONTAINER_WIDGET (f),
+			      repl.width, repl.height);
+    }
+
 #if 0
     /* this is not correct.  x_set_frame_size() is called from
        Fset_frame_size(), which may or may not have been called
Index: src/symsinit.h
===================================================================
RCS file: /pack/xemacscvs/XEmacs/xemacs/src/symsinit.h,v
retrieving revision 1.57
diff -u -r1.57 symsinit.h
--- src/symsinit.h	25 Apr 2006 14:02:09 -0000	1.57
+++ src/symsinit.h	6 May 2006 17:26:01 -0000
@@ -306,6 +306,7 @@
 void vars_of_console_stream (void);
 void vars_of_console_mswindows (void);
 void vars_of_console_tty (void);
+void vars_of_console_x (void);
 void vars_of_data (void);
 void vars_of_database (void);
 void vars_of_debug (void);


-- 
School of Systems and Information Engineering http://turnbull.sk.tsukuba.ac.jp
University of Tsukuba                    Tennodai 1-1-1 Tsukuba 305-8573 JAPAN
               Ask not how you can "do" free software business;
              ask what your business can "do for" free software.




More information about the XEmacs-Beta mailing list