bug#26905: 25.2: MacOS: tooltips show in wrong display

bug#26905: 25.2: MacOS: tooltips show in wrong display

[Charles A. Roelli]

Continuing from bug#26816 
(https://debbugs.gnu.org/cgi/bugreport.cgi?bug=26816#32):

On 11/05/2017 23:43, Alan Third wrote:

>> This now reminds me of a related problem, though: with Emacs 25.2 (or in
>> Emacs 26, with the above change applied to NS_PARENT_WINDOW_TOP_POS(f)),
>> tooltips originating from an area with a help-echo property (like "Lisp
>> Interaction" in the mode line in emacs -Q) in a frame on the secondary
>> monitor actually show up in the primary monitor instead -- as if the tooltip
>> frame is constrained to having a positive x-coordinate only.  I haven't
>> found where it happens, but I guess the cause is similar.
>
> Look at compute_tip_xy in nsfns.m. It moves tooltips into the positive
> screen space. I’ve not managed to get to grips with this code yet.
>
> I think what we want is for it to try to keep the tooltip on one
> screen, so it’s not spanning two monitors, but allow it to go into
> negative space.
>
> Perhaps this should be a separate bug report.

"Primary" and "secondary" monitors are as follows:

(display-monitor-attributes-list) =>

(((name . "Color LCD")
   (geometry 0 0 1280 800)
   (workarea 0 22 1280 714)
   (mm-size 290 180)
   (frames #<frame emacs-devel 0x105044260> #<frame *Backtrace* 
0x1199eca10> #<frame *vc-diff* 0x117dc82b8> #<frame nsterm.m 
0x121c49ad8> #<frame *shell* 0x119adf830> #<frame  *Minibuf-1* 0x119b33030>)
   (source . "NS"))
  ((name . "DELL 2007WFP")
   (geometry -1050 -880 1050 1680)
   (workarea -1050 -880 1050 1680)
   (mm-size 430 270)
   (frames #<frame nsterm.h 0x117c83fd0>)
   (source . "NS")))

bug#26905: 25.2: MacOS: tooltips show in wrong display

[Charles A. Roelli]

If I remove the marked lines in compute_tip_xy:

     if (INTEGERP (left) || INTEGERP (right))
       *root_x = pt.x;
=>  else if (pt.x + XINT (dx) <= 0)
=>    *root_x = 0; /* Can happen for negative dx */
     else if (pt.x + XINT (dx) + width
	     <= x_display_pixel_width (FRAME_DISPLAY_INFO (f)))
       /* It fits to the right of the pointer.  */
       *root_x = pt.x + XINT (dx);
     else if (width + XINT (dx) <= pt.x)
       /* It fits to the left of the pointer.  */
       *root_x = pt.x - width - XINT (dx);
     else
       /* Put it left justified on the screen -- it ought to fit that 
way.  */
       *root_x = 0;

Then the problem is gone.

I notice now, though, that the tooltip can end up partially offscreen, 
both with and without the above change (e.g. when you create a tooltip 
with the mouse pointer at the right edge of the primary monitor).  IIRC 
on GNU/Linux the tooltip is adjusted to fit on screen.  Maybe this 
adjustment works on newer versions of OS X?

bug#26905: 25.2: MacOS: tooltips show in wrong display

[Eli Zaretskii]

> From: "Charles A. Roelli" <charles@aurox.ch>
> Date: Sat, 13 May 2017 11:02:04 +0200
> 
> If I remove the marked lines in compute_tip_xy:
> 
>      if (INTEGERP (left) || INTEGERP (right))
>        *root_x = pt.x;
> =>  else if (pt.x + XINT (dx) <= 0)
> =>    *root_x = 0; /* Can happen for negative dx */
>      else if (pt.x + XINT (dx) + width
> 	     <= x_display_pixel_width (FRAME_DISPLAY_INFO (f)))
>        /* It fits to the right of the pointer.  */
>        *root_x = pt.x + XINT (dx);
>      else if (width + XINT (dx) <= pt.x)
>        /* It fits to the left of the pointer.  */
>        *root_x = pt.x - width - XINT (dx);
>      else
>        /* Put it left justified on the screen -- it ought to fit that way.  */
>        *root_x = 0;
> 
> Then the problem is gone.

Those lines were added to fix some problems (whose particulars I
cannot find at the moment), so we shouldn't remove them without
understanding what is going on.

bug#26905: 25.2: MacOS: tooltips show in wrong display

[Charles A. Roelli]

FWIW, I found this change to the Windows port of Emacs (not sure if it 
was ever committed):


https://lists.gnu.org/archive/html/bug-gnu-emacs/2007-07/msg00153.html


And a more recent change as a result of #22549: commit 
c77ffc8019bceb850a794c13f2e3ad991cc7e412, with changes to xfns.c.


c77ffc8..: Oscar Fuentes 2016-02-06 Use monitor's resolution for 
positioning tooltips
   commit c77ffc8019bceb850a794c13f2e3ad991cc7e412
   Author: Oscar Fuentes <ofv@wanadoo.es>
   Date:   Sat Feb 6 22:12:53 2016 +0100

   Use monitor's resolution for positioning tooltips

   * src/xfns.c (compute_tip_xy): Use the resolution of the monitor where
   the mouse pointer is to avoid placing the tooltip over the border of
   the monitor on multi-head displays. Fixes bug#22549.


Seems like the Windows and X versions nowadays have a check that looks 
like this:

   else if (*root_x + XINT (dx) <= min_x)
     *root_x = 0; /* Can happen for negative dx */

i.e. compare *root_x + XINT (dx) against the variable "min_x", rather 
than comparing against zero, as is done in the NS port. Since frames can 
be in the negative coordinate space, it makes sense to allow a tooltip 
to have a x-coordinate less than 0 -- so the NS port's code should 
probably be changed.


I'm also not sure why, in the above code from the Windows and X 
versions, *root_x is constrained to zero if *root_x + dx goes below 
min_x.  Wouldn't it make more sense to constrain to min_x, which could 
be negative?


On 13/05/2017 11:56, Eli Zaretskii wrote:
>> From: "Charles A. Roelli" <charles@aurox.ch>
>> Date: Sat, 13 May 2017 11:02:04 +0200
>>
>> If I remove the marked lines in compute_tip_xy:
>>
>>       if (INTEGERP (left) || INTEGERP (right))
>>         *root_x = pt.x;
>> =>  else if (pt.x + XINT (dx) <= 0)
>> =>    *root_x = 0; /* Can happen for negative dx */
>>       else if (pt.x + XINT (dx) + width
>> 	     <= x_display_pixel_width (FRAME_DISPLAY_INFO (f)))
>>         /* It fits to the right of the pointer.  */
>>         *root_x = pt.x + XINT (dx);
>>       else if (width + XINT (dx) <= pt.x)
>>         /* It fits to the left of the pointer.  */
>>         *root_x = pt.x - width - XINT (dx);
>>       else
>>         /* Put it left justified on the screen -- it ought to fit that way.  */
>>         *root_x = 0;
>>
>> Then the problem is gone.
> Those lines were added to fix some problems (whose particulars I
> cannot find at the moment), so we shouldn't remove them without
> understanding what is going on.

bug#26905: 25.2: MacOS: tooltips show in wrong display

[Alan Third]

On Sat, May 13, 2017 at 11:02:04AM +0200, Charles A. Roelli wrote:
> I notice now, though, that the tooltip can end up partially offscreen, both
> with and without the above change (e.g. when you create a tooltip with the
> mouse pointer at the right edge of the primary monitor).  IIRC on GNU/Linux
> the tooltip is adjusted to fit on screen.  Maybe this adjustment works on
> newer versions of OS X?

I was thinking about this earlier and I suspect that if we want to
avoid tooltips crossing monitors we’ll have to step through each
screen in [NSScreen screens], check if `pt` is within it’s bounds, and
when we find the right screen, use it’s origin and frame to calculate
the min/max x/y.

constrain_frame_rect in nsterm.m does something similar already.

There’s a method in CGRect called CGRectContainsPoint, which should
help us here, as NSRect is really just a CGRect. Unfortunately it
doesn’t appear to be available in GNUstep, so it may be necessary to
implement the check ourselves.

If we don’t care about spanning monitors, I think we still need to
step through each screen and calculate the overall min/max values.

What do you think?
-- 
Alan Third

bug#26905: [PATCH] Show tooltip on correct screen (bug#26905)

[Alan Third]

* src/nsfns.m (compute_tip_xy): Find the correct screen for the
tooltip and constrain it to that screen.
---
 src/nsfns.m | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/src/nsfns.m b/src/nsfns.m
index cbe0ffb858..04565a99bb 100644
--- a/src/nsfns.m
+++ b/src/nsfns.m
@@ -2760,6 +2760,7 @@ and GNUstep implementations ("distributor-specific release
   EmacsView *view = FRAME_NS_VIEW (f);
   struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   NSPoint pt;
+  NSScreen *screen;
 
   /* Start with user-specified or mouse position.  */
   left = Fcdr (Fassq (Qleft, parms));
@@ -2794,13 +2795,25 @@ and GNUstep implementations ("distributor-specific release
 	      - height);
     }
 
+  /* Find the screen that pt is on. */
+  for (screen in [NSScreen screens])
+#ifdef NS_IMPL_COCOA
+    if (CGRectContainsPoint ([screen frame], pt))
+#else
+    if (pt.x >= screen.frame.origin.x
+        && pt.x < screen.frame.origin.x + screen.frame.size.width
+        && pt.y >= screen.frame.origin.y
+        && pt.y < screen.frame.origin.y + screen.frame.size.height)
+#endif
+      break;
+
   /* Ensure in bounds.  (Note, screen origin = lower left.) */
   if (INTEGERP (left) || INTEGERP (right))
     *root_x = pt.x;
-  else if (pt.x + XINT (dx) <= 0)
-    *root_x = 0; /* Can happen for negative dx */
+  else if (pt.x + XINT (dx) <= screen.frame.origin.x)
+    *root_x = screen.frame.origin.x; /* Can happen for negative dx */
   else if (pt.x + XINT (dx) + width
-	   <= x_display_pixel_width (FRAME_DISPLAY_INFO (f)))
+	   <= screen.frame.origin.x + screen.frame.size.width)
     /* It fits to the right of the pointer.  */
     *root_x = pt.x + XINT (dx);
   else if (width + XINT (dx) <= pt.x)
@@ -2808,20 +2821,20 @@ and GNUstep implementations ("distributor-specific release
     *root_x = pt.x - width - XINT (dx);
   else
     /* Put it left justified on the screen -- it ought to fit that way.  */
-    *root_x = 0;
+    *root_x = screen.frame.origin.x;
 
   if (INTEGERP (top) || INTEGERP (bottom))
     *root_y = pt.y;
-  else if (pt.y - XINT (dy) - height >= 0)
+  else if (pt.y - XINT (dy) - height >= screen.frame.origin.y)
     /* It fits below the pointer.  */
     *root_y = pt.y - height - XINT (dy);
   else if (pt.y + XINT (dy) + height
-	   <= x_display_pixel_height (FRAME_DISPLAY_INFO (f)))
+	   <= screen.frame.origin.y + screen.frame.size.height)
     /* It fits above the pointer */
       *root_y = pt.y + XINT (dy);
   else
     /* Put it on the top.  */
-    *root_y = x_display_pixel_height (FRAME_DISPLAY_INFO (f)) - height;
+    *root_y = screen.frame.origin.y + screen.frame.size.height - height;
 }
 
 
-- 

Here’s my go at this. It seems to work OK on single and multi‐monitor
setups in macOS, and in GNUstep in single monitor (I can’t test it
multi‐monitor).

-- 
Alan Third

bug#26905: [PATCH] Show tooltip on correct screen (bug#26905)

[Charles A. Roelli]

Works fine for me too! Thanks for coming up with this fix.


> On 16 May 2017, at 23:56, Alan Third <alan@idiocy.org> wrote:
> 
> * src/nsfns.m (compute_tip_xy): Find the correct screen for the
> tooltip and constrain it to that screen.
> ---
> src/nsfns.m | 27 ++++++++++++++++++++-------
> 1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/src/nsfns.m b/src/nsfns.m
> index cbe0ffb858..04565a99bb 100644
> --- a/src/nsfns.m
> +++ b/src/nsfns.m
> @@ -2760,6 +2760,7 @@ and GNUstep implementations ("distributor-specific release
>   EmacsView *view = FRAME_NS_VIEW (f);
>   struct ns_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
>   NSPoint pt;
> +  NSScreen *screen;
> 
>   /* Start with user-specified or mouse position.  */
>   left = Fcdr (Fassq (Qleft, parms));
> @@ -2794,13 +2795,25 @@ and GNUstep implementations ("distributor-specific release
>          - height);
>     }
> 
> +  /* Find the screen that pt is on. */
> +  for (screen in [NSScreen screens])
> +#ifdef NS_IMPL_COCOA
> +    if (CGRectContainsPoint ([screen frame], pt))
> +#else
> +    if (pt.x >= screen.frame.origin.x
> +        && pt.x < screen.frame.origin.x + screen.frame.size.width
> +        && pt.y >= screen.frame.origin.y
> +        && pt.y < screen.frame.origin.y + screen.frame.size.height)
> +#endif
> +      break;
> +
>   /* Ensure in bounds.  (Note, screen origin = lower left.) */
>   if (INTEGERP (left) || INTEGERP (right))
>     *root_x = pt.x;
> -  else if (pt.x + XINT (dx) <= 0)
> -    *root_x = 0; /* Can happen for negative dx */
> +  else if (pt.x + XINT (dx) <= screen.frame.origin.x)
> +    *root_x = screen.frame.origin.x; /* Can happen for negative dx */
>   else if (pt.x + XINT (dx) + width
> -       <= x_display_pixel_width (FRAME_DISPLAY_INFO (f)))
> +       <= screen.frame.origin.x + screen.frame.size.width)
>     /* It fits to the right of the pointer.  */
>     *root_x = pt.x + XINT (dx);
>   else if (width + XINT (dx) <= pt.x)
> @@ -2808,20 +2821,20 @@ and GNUstep implementations ("distributor-specific release
>     *root_x = pt.x - width - XINT (dx);
>   else
>     /* Put it left justified on the screen -- it ought to fit that way.  */
> -    *root_x = 0;
> +    *root_x = screen.frame.origin.x;
> 
>   if (INTEGERP (top) || INTEGERP (bottom))
>     *root_y = pt.y;
> -  else if (pt.y - XINT (dy) - height >= 0)
> +  else if (pt.y - XINT (dy) - height >= screen.frame.origin.y)
>     /* It fits below the pointer.  */
>     *root_y = pt.y - height - XINT (dy);
>   else if (pt.y + XINT (dy) + height
> -       <= x_display_pixel_height (FRAME_DISPLAY_INFO (f)))
> +       <= screen.frame.origin.y + screen.frame.size.height)
>     /* It fits above the pointer */
>       *root_y = pt.y + XINT (dy);
>   else
>     /* Put it on the top.  */
> -    *root_y = x_display_pixel_height (FRAME_DISPLAY_INFO (f)) - height;
> +    *root_y = screen.frame.origin.y + screen.frame.size.height - height;
> }
> 
> 
> -- 
> 
> Here’s my go at this. It seems to work OK on single and multi‐monitor
> setups in macOS, and in GNUstep in single monitor (I can’t test it
> multi‐monitor).
> 
> -- 
> Alan Third

bug#26905: [PATCH] Show tooltip on correct screen (bug#26905)

[Alan Third]

"Charles A. Roelli" <charles@aurox.ch> writes:

> Works fine for me too! Thanks for coming up with this fix.
>> 
>> Here’s my go at this. It seems to work OK on single and multi‐monitor
>> setups in macOS, and in GNUstep in single monitor (I can’t test it
>> multi‐monitor).

Pushed to master.

-- 
Alan Third