bug#29353: OSX/MacOS: Adding support for window-divider-first/last-pixel

bug#29353: OSX/MacOS: Adding support for window-divider-first/last-pixel

[Keith David Bershatsky]

Upon grepping the source code for window_divider_first_pixel and window_divider_last_pixel, I see no existing support for OSX/MacOS builds.

Although I have not done any testing on other platforms, it appears that support for these features already exists for Windows and X11 builds.

It would be nifty if OSX/MacOS users could also enjoy this cool feature.

Thanks,

Keith

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Alan Third]

* src/nsterm.m (ns_draw_window_divider): Use
window-divider-first-pixel and window-divider-last-pixel faces.
---
 src/nsterm.m | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 5c29f039e5..b63c85fc56 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3174,18 +3174,50 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
    -------------------------------------------------------------------------- */
 {
   struct frame *f = XFRAME (WINDOW_FRAME (w));
-  struct face *face;
-  NSRect r = NSMakeRect (x0, y0, x1-x0, y1-y0);
+  struct face *face = FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_FACE_ID);
+  struct face *face_first
+    = FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_FIRST_PIXEL_FACE_ID);
+  struct face *face_last
+    = FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_LAST_PIXEL_FACE_ID);
+  unsigned long color = face ? face->foreground : FRAME_FOREGROUND_PIXEL (f);
+  unsigned long color_first = (face_first
+			       ? face_first->foreground
+			       : FRAME_FOREGROUND_PIXEL (f));
+  unsigned long color_last = (face_last
+			      ? face_last->foreground
+			      : FRAME_FOREGROUND_PIXEL (f));
+  NSRect divider = NSMakeRect (x0, y0, x1-x0, y1-y0);
 
   NSTRACE ("ns_draw_window_divider");
 
-  face = FACE_FROM_ID_OR_NULL (f, WINDOW_DIVIDER_FACE_ID);
+  ns_focus (f, &divider, 1);
 
-  ns_focus (f, &r, 1);
-  if (face)
-    [ns_lookup_indexed_color(face->foreground, f) set];
+  if (y1 - y0 > x1 - x0 && x1 - x0 > 2)
+    /* Vertical.  */
+    {
+      [ns_lookup_indexed_color(color_first, f) set];
+      NSRectFill(NSMakeRect (x0, y0, 1, y1 - y0));
+      [ns_lookup_indexed_color(color, f) set];
+      NSRectFill(NSMakeRect (x0 + 1, y0, x1 - x0 - 2, y1 - y0));
+      [ns_lookup_indexed_color(color_last, f) set];
+      NSRectFill(NSMakeRect (x1 - 1, y0, 1, y1 - y0));
+    }
+  else if (x1 - x0 > y1 - y0 && y1 - y0 > 3)
+    /* Horizontal.  */
+    {
+      [ns_lookup_indexed_color(color_first, f) set];
+      NSRectFill(NSMakeRect (x0, y0, x1 - x0, 1));
+      [ns_lookup_indexed_color(color, f) set];
+      NSRectFill(NSMakeRect (x0, y0 + 1, x1 - x0, y1 - y0 -2));
+      [ns_lookup_indexed_color(color_last, f) set];
+      NSRectFill(NSMakeRect (x0, y1 - 1, x1 - x0, 1));
+    }
+  else
+    {
+      [ns_lookup_indexed_color(color, f) set];
+      NSRectFill(divider);
+    }
 
-  NSRectFill(r);
   ns_unfocus (f);
 }
 
-- 
Hi Keith,

Can you please test this patch? I’m unsure how to set these faces so I
don’t know if it’s working right.

-- 
Alan Third

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Keith David Bershatsky]

Thank you, Alan, for bringing this feature to Emacs on the OSX/MacOS platforms.

The only issue I observed with the patch applied is when a user selects a divider width of 3 for right and bottom.  In that situation, the bottom divider is entirely one color -- window-divider face.  And, the right divider is 2 pixels in the window-divider-first-pixel face and 1 pixel in the window-divider-last-pixel face.  When there are exactly 3 pixels, both dividers should have the rainbow of all three available colors in the applicable order.

The resolution on my screen is not the greatest in the world, but it looks like all other width variations are working properly.  I.e., 1 and 2 pixel widths properly have just the window-divider color (since there is agreeably no room for both the first/last pixel colors), and 4 pixels in width and above have the rainbow effect.

(face-spec-set 'default '((t :background "black" :foreground "red")))

(face-spec-set 'window-divider-first-pixel '((t :foreground "red")))

(face-spec-set 'window-divider '((t :foreground "DarkBlue")))

(face-spec-set 'window-divider-last-pixel '((t :foreground "magenta")))

(with-selected-frame
  (make-frame '((right-divider-width . 3) (bottom-divider-width . 3)))
  (split-window-horizontally))

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-19-2017 03:53:13] <19 Nov 2017 11:53:13 +0000>
FROM:  Alan Third <alan@idiocy.org>
> 
> * src/nsterm.m (ns_draw_window_divider): Use
> window-divider-first-pixel and window-divider-last-pixel faces.
> ---
> * * *

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Alan Third]

On Sun, Nov 19, 2017 at 12:40:09PM -0800, Keith David Bershatsky wrote:
> The only issue I observed with the patch applied is when a user
> selects a divider width of 3 for right and bottom. In that
> situation, the bottom divider is entirely one color --
> window-divider face.

I can replicate this. It’s because I copied the code verbatim from
xterm.c, and it does a comparison against 3 instead of 2, so this
might be a bug in X builds too.

> And, the right divider is 2 pixels in the window-divider-first-pixel
> face and 1 pixel in the window-divider-last-pixel face. When there
> are exactly 3 pixels, both dividers should have the rainbow of all
> three available colors in the applicable order.

I can’t replicate this. I definitely see three colours in the vertical
divider.

modified   src/nsterm.m
@@ -3202,7 +3202,7 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
       [ns_lookup_indexed_color(color_last, f) set];
       NSRectFill(NSMakeRect (x1 - 1, y0, 1, y1 - y0));
     }
-  else if (x1 - x0 > y1 - y0 && y1 - y0 > 3)
+  else if (x1 - x0 > y1 - y0 && y1 - y0 > 2)
     /* Horizontal.  */
     {
       [ns_lookup_indexed_color(color_first, f) set];

-- 
Alan Third

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[martin rudalics]

 > I can replicate this. It’s because I copied the code verbatim from
 > xterm.c, and it does a comparison against 3 instead of 2, so this
 > might be a bug in X builds too.

And in w32 builds.  Hopefully fixed now in both.

Thanks for noting this and the analysis, martin

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Keith David Bershatsky]

Alan:

It appears that the right divider is encroaching on the fringe to a tune of 1 pixel, i.e., the vertical divider is drawing on top of the left side of fringe bitmaps.  Is this related to the patch, or a new issue?

(face-spec-set 'default '((t :background "black" :foreground "red")))

(face-spec-set 'window-divider-first-pixel '((t :foreground "red")))

(face-spec-set 'window-divider '((t :foreground "DarkBlue")))

(face-spec-set 'window-divider-last-pixel '((t :foreground "magenta")))

(with-selected-frame
  (make-frame '((right-divider-width . 8) (bottom-divider-width . 8)))
  (split-window-horizontally))

(visual-line-mode)

(setq fringe-indicator-alist '(
  (truncation . (left-arrow right-arrow))
  (continuation . (left-curly-arrow right-curly-arrow))
  (overlay-arrow . right-triangle)
  (up . up-arrow)
  (down . down-arrow)
  (top . top-left-angle)
  (bottom . bottom-right-angle)
  (top-bottom . (left-bracket
                 right-bracket
                 top-right-angle
                 top-left-angle))
  (empty-line . empty-line)
  (unknown . question-mark)))

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Alan Third]

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

On Sun, Nov 19, 2017 at 02:11:23PM -0800, Keith David Bershatsky wrote:
> Alan:
> 
> It appears that the right divider is encroaching on the fringe to a
> tune of 1 pixel, i.e., the vertical divider is drawing on top of the
> left side of fringe bitmaps. Is this related to the patch, or a new
> issue?

It looks OK here. Screenshot attached. Am I misinterpreting you, or
are you seeing something different?
-- 
Alan Third

[-- Attachment #2: fringes.png --]
[-- Type: image/png, Size: 24750 bytes --]

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Keith David Bershatsky]

Thank you, Alan, for taking a look at that potential issue.

I tried again -- this time with my nose touching the screen -- and I think the issue may be with my vision and/or the screen quality.  Perhaps this is the excuse I've been waiting for to get a higher resolution monitor.  :)

I will defer to your better vision on this.

Thanks,

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-19-2017 14:53:06] <19 Nov 2017 22:53:06 +0000>
FROM:  Alan Third <alan@idiocy.org>
> 
> On Sun, Nov 19, 2017 at 02:11:23PM -0800, Keith David Bershatsky wrote:
> > Alan:
> > 
> > It appears that the right divider is encroaching on the fringe to a
> > tune of 1 pixel, i.e., the vertical divider is drawing on top of the
> > left side of fringe bitmaps. Is this related to the patch, or a new
> > issue?
> 
> It looks OK here. Screenshot attached. Am I misinterpreting you, or
> are you seeing something different?
> -- 
> Alan Third
> [* fringes.png]
>

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Alan Third]

On Sun, Nov 19, 2017 at 03:06:54PM -0800, Keith David Bershatsky wrote:
> Thank you, Alan, for taking a look at that potential issue.
> 
> I tried again -- this time with my nose touching the screen -- and I
> think the issue may be with my vision and/or the screen quality.
> Perhaps this is the excuse I've been waiting for to get a higher
> resolution monitor. :)

If it’s an old monitor is it possibly some colour bleed? Try inverting
the colours.

The only other way I can think of to be sure is to take a screenshot
and zoom in past 100% in a viewer (you can use emacs if you’ve
compiled it against imagemagick!).
-- 
Alan Third

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Keith David Bershatsky]

Thank you, Alan, for teaching me to use screenshots and zooming to see what was really happening.  Yes, it ended up being colors bleeding across pixel boundaries.

Everything looks great with a screenshot and an image viewer with magnification.  And, it sure is much easier than moving filing cabinets out of the way so that I can put my nose on the screen to get a better vantage point.  :)

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-19-2017 15:27:13] <19 Nov 2017 23:27:13 +0000>
FROM:  Alan Third <alan@idiocy.org>
> 
> On Sun, Nov 19, 2017 at 03:06:54PM -0800, Keith David Bershatsky wrote:
> > Thank you, Alan, for taking a look at that potential issue.
> > 
> > I tried again -- this time with my nose touching the screen -- and I
> > think the issue may be with my vision and/or the screen quality.
> > Perhaps this is the excuse I've been waiting for to get a higher
> > resolution monitor. :)
> 
> If it's an old monitor is it possibly some colour bleed? Try inverting
> the colours.
> 
> The only other way I can think of to be sure is to take a screenshot
> and zoom in past 100% in a viewer (you can use emacs if you've
> compiled it against imagemagick!).
> -- 
> Alan Third

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Alan Third]

On Sun, Nov 19, 2017 at 10:31:22PM -0800, Keith David Bershatsky wrote:
> Thank you, Alan, for teaching me to use screenshots and zooming to
> see what was really happening. Yes, it ended up being colors
> bleeding across pixel boundaries.
> 
> Everything looks great with a screenshot and an image viewer with
> magnification. And, it sure is much easier than moving filing
> cabinets out of the way so that I can put my nose on the screen to
> get a better vantage point. :)

Thanks for checking. I’ll push my change, and I think we can mark this
as done.
-- 
Alan Third

bug#29353: [PATCH] Add window divider faces to NS (bug#29353)

[Keith David Bershatsky]

Yes, this latest patch fixed the issue that I was observing when the width of the dividers is exactly 3.

Thank you again for bringing this feature to OSX/MacOS users.

Greatly appreciated,

Keith

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

DATE:  [11-19-2017 15:23:01] <19 Nov 2017 23:23:01 +0000>
FROM:  Alan Third <alan@idiocy.org>
> 
> * * *
> 
> modified   src/nsterm.m
> @@ -3202,7 +3202,7 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
>        [ns_lookup_indexed_color(color_last, f) set];
>        NSRectFill(NSMakeRect (x1 - 1, y0, 1, y1 - y0));
>      }
> -  else if (x1 - x0 > y1 - y0 && y1 - y0 > 3)
> +  else if (x1 - x0 > y1 - y0 && y1 - y0 > 2)
>      /* Horizontal.  */
>      {
>        [ns_lookup_indexed_color(color_first, f) set];
> 
> -- 
> Alan Third