bug#23568: 25.0.94; Mode line menus appear incorrectly in some monitor configurations

[Alex <agrambot@gmail.com>]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Alex <agrambot@gmail.com>
>> Date: Fri, 02 Jun 2017 18:54:03 -0600
>> Cc: 23568@debbugs.gnu.org

>> Don't you want to throw an error when x and y are used, but are not
>> integers?
>
> If this is supposed to be used as part of mode-line display, then no.
> Signaling errors in the middle of redisplay is generally a bad idea,
> because they cause another redisplay cycle, which again signals an
> error, and Emacs just freezes.

I see. In the general case, is it recommended to silently fallback to a
different method, or should there be a log message?

I changed the procedure so that it only uses the coordinates if they're
numbers.

>> +(defun display-monitor-attribute (attribute &optional display x y)
>> +  "Return the value of the ATTRIBUTE of the current monitor.
>
> The doc string should say something about what "the current monitor"
> means, or have a link to where that is explained.

I expanded that section, copying a chunk of it from
`frame-monitor-attributes'.

>> +DISPLAY can be a display name, a terminal name, or a frame.
>
> "Terminal name" or "terminal object"?

`display-monitor-attributes' can take both, which is where I copied that
line from.

I don't think I should have done that, actually. I don't believe this
function makes sense for a terminal/display input, as a
terminal/display, as I understand it, can have multiple physical
monitors.

I changed all instances of `display' to `frame'. I'm still not sure on
the name, though. Perhaps a better prefix would be `physical-monitor-*',
or just `monitor-*'?

What do you think?

>> diff --git a/src/xmenu.c b/src/xmenu.c
>> index 2805249164..04d5bde2ba 100644
>> --- a/src/xmenu.c
>> +++ b/src/xmenu.c
>
> Why is this in xmenu.c?  Is the problem unique to X window system?

The xmenu.c part contains the actual bugfix, while the frame.el part is
just a partial abstraction for the fix so that other procedures can use
it (such as compute_tip_xy in xfns.c).

The procedure I'm editing (menu_position_func) is inside an #ifdef
USE_GTK, and uses x_display_pixel_{height,width} currently. I'm not sure
if it's useful outside of X currently.

I haven't tested the issue on other systems, but I don't think it's an
issue in Windows as half of the code came from Windows code according to
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=22549#61.

Here's an updated diff:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v4 --]
[-- Type: text/x-diff, Size: 5214 bytes --]

diff --git a/lisp/frame.el b/lisp/frame.el
index 02871e0551..d0ac29694c 100644
--- a/lisp/frame.el
+++ b/lisp/frame.el
@@ -1498,6 +1498,75 @@ frame-monitor-attributes
 	   for frames = (cdr (assq 'frames attributes))
 	   if (memq frame frames) return attributes))
 
+(defun frame-monitor-attribute (attribute &optional frame x y)
+  "Return the value of the ATTRIBUTE of the current monitor.
+If FRAME is omitted or nil, use currently selected frame.
+
+By default, the current monitor is said to be the physical
+monitor dominating FRAME.  A frame is dominated by a physical
+monitor when either the largest area of the frame resides in the
+monitor, or the monitor is the closest to the frame if the frame
+does not intersect any physical monitors.
+
+If X and Y are both numbers, then ignore the value of FRAME; the
+current monitor is determined to be the physical monitor that
+contains the pixel coordinate (X, Y).
+
+See `display-monitor-attributes-list' for the list of attribute
+keys and their meanings."
+  (if (and (numberp x)
+           (numberp y))
+      (cl-loop for monitor in (display-monitor-attributes-list)
+               for geometry = (alist-get 'geometry monitor)
+               for min-x = (pop geometry)
+               for min-y = (pop geometry)
+               for max-x = (+ min-x (pop geometry))
+               for max-y = (+ min-y (car geometry))
+               when (and (<= min-x x)
+                         (< x max-x)
+                         (<= min-y y)
+                         (< y max-y))
+               return (alist-get attribute monitor))
+    (alist-get attribute (frame-monitor-attributes frame))))
+
+(defun frame-monitor-geometry (&optional frame x y)
+    "Return the geometry of the current monitor.
+FRAME can be a frame name, a terminal name, or a frame.
+If FRAME is omitted or nil, use the currently selected frame.
+
+By default, the current monitor is said to be the physical
+monitor dominating FRAME.  A frame is dominated by a physical
+monitor when either the largest area of the frame resides in the
+monitor, or the monitor is the closest to the frame if the frame
+does not intersect any physical monitors.
+
+If X and Y are both numbers, then ignore the value of FRAME; the
+current monitor is determined to be the physical monitor that
+contains the pixel coordinate (X, Y).
+
+See `display-monitor-attributes-list' for information on the
+geometry attribute."
+  (frame-monitor-attribute 'geometry frame x y))
+
+(defun frame-monitor-workarea (&optional frame x y)
+  "Return the workarea of the current monitor.
+FRAME can be a frame name, a terminal name, or a frame.
+If FRAME is omitted or nil, use currently selected frame.
+
+By default, the current monitor is said to be the physical
+monitor dominating FRAME.  A frame is dominated by a physical
+monitor when either the largest area of the frame resides in the
+monitor, or the monitor is the closest to the frame if the frame
+does not intersect any physical monitors.
+
+If X and Y are both numbers, then ignore the value of FRAME; the
+current monitor is determined to be the physical monitor that
+contains the pixel coordinate (X, Y).
+
+See `display-monitor-attributes-list' for information on the
+workarea attribute."
+  (frame-monitor-attribute 'workarea frame x y))
+
 (declare-function x-frame-list-z-order "xfns.c" (&optional display))
 (declare-function w32-frame-list-z-order "w32fns.c" (&optional display))
 (declare-function ns-frame-list-z-order "nsfns.m" (&optional display))
diff --git a/src/xmenu.c b/src/xmenu.c
index 2805249164..5aee85360d 100644
--- a/src/xmenu.c
+++ b/src/xmenu.c
@@ -1160,9 +1160,33 @@ menu_position_func (GtkMenu *menu, gint *x, gint *y, gboolean *push_in, gpointer
 {
   struct next_popup_x_y *data = user_data;
   GtkRequisition req;
-  struct x_display_info *dpyinfo = FRAME_DISPLAY_INFO (data->f);
-  int disp_width = x_display_pixel_width (dpyinfo);
-  int disp_height = x_display_pixel_height (dpyinfo);
+  int disp_width = -1;
+  int disp_height = -1;
+
+  Lisp_Object frame, geometry;
+
+  XSETFRAME (frame, data->f);
+  geometry = call3 (Qdisplay_monitor_geometry,
+                    Qnil,
+                    make_number (data->x),
+                    make_number (data->y));
+
+  if (CONSP (geometry))
+    {
+      int min_x, min_y;
+
+      min_x = XINT (XCAR (geometry));
+      min_y = XINT (Fnth (make_number (1), geometry));
+      disp_width = min_x + XINT (Fnth (make_number (2), geometry));
+      disp_height = min_y + XINT (Fnth (make_number (3), geometry));
+    }
+  if ( disp_width < 0 || disp_height < 0 )
+    {
+      struct x_display_info *dpyinfo = FRAME_DISPLAY_INFO (data->f);
+
+      disp_width = x_display_pixel_width (dpyinfo);
+      disp_height = x_display_pixel_height (dpyinfo);
+    }
 
   *x = data->x;
   *y = data->y;
@@ -2361,6 +2385,10 @@ syms_of_xmenu (void)
   DEFSYM (Qdebug_on_next_call, "debug-on-next-call");
   defsubr (&Smenu_or_popup_active_p);
 
+#ifdef USE_GTK
+  DEFSYM (Qdisplay_monitor_geometry, "display-monitor-geometry");
+#endif
+
 #if defined (USE_GTK) || defined (USE_X_TOOLKIT)
   defsubr (&Sx_menu_bar_open_internal);
   Ffset (intern_c_string ("accelerate-menu"),