bug#22105: 25.1.50; REGRESSION - `modify-frame-parameters'

[martin rudalics <rudalics@gmx.at>]

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

 > IIUC the problem I see can be described more easily as follows.
 > Evaluate in sequence the following expressions with emacs -Q:
 >
 > (modify-frame-parameters nil '((menu-bar-lines . 0) (height . 30)))
 > (modify-frame-parameters nil '((menu-bar-lines . 1) (height . 40)))
 > (frame-parameter nil 'height)
 >
 > Here the height reported by the last form is 41.

Actually, the immediate cause of this problem goes as follows: When you
specify both addition of the menu bar and the new frame height,
‘modify-frame-parameters’ will first issue a request to add the menu bar
and then a request to calculate the new frame height as specified.

However, Emacs does not immediately add the menu bar but delay this
until prepare_menu_bars is run by the display engine.  Till then, only
the value stored in the external_menu_bar slot of the frame tells
whether the frame shall have a menu bar or not.

As a consequence, the request to set the frame height will be delivered
first to the toolkit.  The calculation of the new frame height on
Windows is done by calling the function AdjustWindowRect.  The third
argument of that function has to specify whether a menu bar should be
included in the calculation or not.  Currently, this value is set to
whatever the external_menu_bar slot stores.  But if the frame does not
have a menu bar so far but should get one in the future, the value
stored there is inherently wrong for calculating the size of the frame
height via AdjustWindowRect.

I attach a patch against Emasc-25 which fixes this bug here.  I also
added a remark to the doc-string of ‘modify-frame-parameters’ in the
sense that if you specify several parameters which might affect the size
of the frame, Emacs cannot always guarantee for every toolkit that the
final size will meet the expectations of the caller.  Inherently this
means, that the application programmer has to experiment with this
function on each and every toolkit involved.

I leave it to Eli to decide whether the patch should go into Emacs-25 or
master/trunk.

Thanks for your attention, martin

[-- Attachment #2: add-menu-bar.diff --]
[-- Type: text/plain, Size: 5762 bytes --]

diff --git a/doc/lispref/frames.texi b/doc/lispref/frames.texi
index 3c1a87a..0a43929 100644
--- a/doc/lispref/frames.texi
+++ b/doc/lispref/frames.texi
@@ -1007,12 +1007,28 @@ Parameter Access
 parameter.  If you don't mention a parameter in @var{alist}, its value
 doesn't change.  If @var{frame} is @code{nil}, it defaults to the selected
 frame.
+
+When @var{alist} specifies more than one parameter whose value can
+affect the new size of @var{frame}, the final size of the frame may
+differ according to the toolkit used.  For example, specifying that a
+frame should from now on have a menu and/or tool bar instead of none and
+simultaneously specifying the new height of the frame will inevitably
+lead to a recalculation of the frame's height.  Conceptually, in such
+case, this function will try to have the explicit height specification
+prevail.  It cannot be excluded, however, that the addition (or removal)
+of the menu or tool bar, when eventually performed by the toolkit, will
+defeat this intention.
+
+Sometimes, binding @code{frame-inhibit-implied-resize} (@pxref{Implied
+Frame Resizing}) to a non-@code{nil} value around calls to this function
+may fix the problem sketched here.  Sometimes, however, exactly such
+binding may be hit by the problem.
 @end defun

 @defun set-frame-parameter frame parm value
 This function sets the frame parameter @var{parm} to the specified
-@var{value}.  If @var{frame} is @code{nil}, it defaults to the
-selected frame.
+@var{value}.  If @var{frame} is @code{nil}, it defaults to the selected
+frame.
 @end defun

 @defun modify-all-frames-parameters alist
diff --git a/src/w32fns.c b/src/w32fns.c
index 208c980..f9ce762 100644
--- a/src/w32fns.c
+++ b/src/w32fns.c
@@ -1666,10 +1666,7 @@ x_set_menu_bar_lines (struct frame *f, Lisp_Object value, Lisp_Object oldval)
   FRAME_MENU_BAR_LINES (f) = 0;
   FRAME_MENU_BAR_HEIGHT (f) = 0;
   if (nlines)
-    {
-      FRAME_EXTERNAL_MENU_BAR (f) = 1;
-      windows_or_buffers_changed = 23;
-    }
+    FRAME_EXTERNAL_MENU_BAR (f) = 1;
   else
     {
       if (FRAME_EXTERNAL_MENU_BAR (f) == 1)
@@ -4620,8 +4617,7 @@ my_create_tip_window (struct frame *f)
   rect.right = FRAME_PIXEL_WIDTH (f);
   rect.bottom = FRAME_PIXEL_HEIGHT (f);

-  AdjustWindowRect (&rect, f->output_data.w32->dwStyle,
-		    FRAME_EXTERNAL_MENU_BAR (f));
+  AdjustWindowRect (&rect, f->output_data.w32->dwStyle, false);

   tip_window = FRAME_W32_WINDOW (f)
     = CreateWindow (EMACS_CLASS,
@@ -6681,8 +6677,7 @@ Text larger than the specified size is clipped.  */)
     rect.left = rect.top = 0;
     rect.right = width;
     rect.bottom = height;
-    AdjustWindowRect (&rect, f->output_data.w32->dwStyle,
-		      FRAME_EXTERNAL_MENU_BAR (f));
+    AdjustWindowRect (&rect, f->output_data.w32->dwStyle, false);

     /* Position and size tooltip, and put it in the topmost group.
        The add-on of FRAME_COLUMN_WIDTH to the 5th argument is a
diff --git a/src/w32menu.c b/src/w32menu.c
index 6af69f4..964b965 100644
--- a/src/w32menu.c
+++ b/src/w32menu.c
@@ -494,7 +494,10 @@ set_frame_menubar (struct frame *f, bool first_time, bool deep_p)
     /* Force the window size to be recomputed so that the frame's text
        area remains the same, if menubar has just been created.  */
     if (old_widget == NULL)
-      adjust_frame_size (f, -1, -1, 2, false, Qmenu_bar_lines);
+      {
+	windows_or_buffers_changed = 23;
+	adjust_frame_size (f, -1, -1, 2, false, Qmenu_bar_lines);
+      }
   }

   unblock_input ();
diff --git a/src/w32term.c b/src/w32term.c
index f48e725..0b8bef2 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -6115,9 +6115,22 @@ x_set_window_size (struct frame *f, bool change_gravity,
   int pixelwidth, pixelheight;
   Lisp_Object fullscreen = get_frame_param (f, Qfullscreen);
   RECT rect;
+  MENUBARINFO info;
+  int menu_bar_height;

   block_input ();

+  /* Get the height of the menu bar here.  It's used below to detect
+     whether the menu bar is wrapped.  It's also used to specify the
+     third argument for AdjustWindowRect.  FRAME_EXTERNAL_MENU_BAR which
+     has been used before for that reason is unreliable because it only
+     specifies whether we _want_ a menu bar for this frame and not
+     whether this frame _has_ a menu bar.  See bug#22105.  */
+  info.cbSize = sizeof (info);
+  info.rcBar.top = info.rcBar.bottom = 0;
+  GetMenuBarInfo (FRAME_W32_WINDOW (f), 0xFFFFFFFD, 0, &info);
+  menu_bar_height = info.rcBar.bottom - info.rcBar.top;
+
   if (pixelwise)
     {
       pixelwidth = FRAME_TEXT_TO_PIXEL_WIDTH (f, width);
@@ -6135,17 +6148,11 @@ x_set_window_size (struct frame *f, bool change_gravity,
 	 height of the frame then the wrapped menu bar lines are not
 	 accounted for (Bug#15174 and Bug#18720).  Here we add these
 	 extra lines to the frame height.  */
-      MENUBARINFO info;
       int default_menu_bar_height;
-      int menu_bar_height;

       /* Why is (apparently) SM_CYMENUSIZE needed here instead of
 	 SM_CYMENU ??  */
       default_menu_bar_height = GetSystemMetrics (SM_CYMENUSIZE);
-      info.cbSize = sizeof (info);
-      info.rcBar.top = info.rcBar.bottom = 0;
-      GetMenuBarInfo (FRAME_W32_WINDOW (f), 0xFFFFFFFD, 0, &info);
-      menu_bar_height = info.rcBar.bottom - info.rcBar.top;

       if ((default_menu_bar_height > 0)
 	  && (menu_bar_height > default_menu_bar_height)
@@ -6160,8 +6167,7 @@ x_set_window_size (struct frame *f, bool change_gravity,
   rect.right = pixelwidth;
   rect.bottom = pixelheight;

-  AdjustWindowRect (&rect, f->output_data.w32->dwStyle,
-		    FRAME_EXTERNAL_MENU_BAR (f));
+  AdjustWindowRect (&rect, f->output_data.w32->dwStyle, menu_bar_height > 0);

   if (!(f->after_make_frame)
       && !(f->want_fullscreen & FULLSCREEN_WAIT)