bug#30553: 26.0.91; underline appears beneath line-spacing rather than beneath text

[Aaron Jensen <aaronjensen@gmail.com>]

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

On Wed, Feb 21, 2018 at 9:56 AM, Eli Zaretskii <eliz@gnu.org> wrote:
> Feel free to submit patches to introduce this as yet another optional
> behavior.  We cannot just throw out the current behavior, because it
> was with us since Emacs 21.
>
> Note that this feature has 3 implementations, one each in every GUI
> back-end we support.

I changed my mind on this after discovering that these variables do
not work as buffer local variables. Whatever the current buffer has it
set to causes all other visible buffers to draw in that way, which
causes underlines to jump around if they're set differently in
different buffers.

Please see attached patch. I don't have a windows or linux machine to
test on, so if someone could do that it would be appreciated. There
are some subtle differences in the original code which I tried to
respect.

I'm open to any advice on the naming/doc strings or implementation. Thanks!

Aaron

[-- Attachment #2: 0001-Add-x-underline-below-font-descent.patch --]
[-- Type: application/octet-stream, Size: 4660 bytes --]

From 93892660ffb3d0ee80dbf1fc1976a429e1f3f104 Mon Sep 17 00:00:00 2001
From: Aaron Jensen <aaronjensen@gmail.com>
Date: Sun, 25 Feb 2018 14:06:49 -0800
Subject: [PATCH] Add x-underline-below-font-descent

---
 src/nsterm.m  |  7 +++++++
 src/w32term.c |  9 ++++++++-
 src/xterm.c   | 15 +++++++++++++--
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/src/nsterm.m b/src/nsterm.m
index 1919c6defa..bd2d887ba1 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -3495,6 +3495,8 @@ Note that CURSOR_WIDTH is meaningful only for (h)bar cursors.
               /* Determine the offset of underlining from the baseline. */
               if (x_underline_at_descent_line)
                 position = descent - thickness;
+              else if (x_underline_below_font_descent && font)
+                position = s->y - s->ybase + font->height;
               else if (x_use_underline_position_properties
                        && font && font->underline_position >= 0)
                 position = font->underline_position;
@@ -9471,6 +9473,11 @@ Nil means use fullscreen the old (< 10.7) way.  The old way works better with
      doc: /* SKIP: real doc in xterm.c.  */);
   x_underline_at_descent_line = 0;
 
+  DEFVAR_BOOL ("x-underline-below-font-descent",
+	       x_underline_below_font_descent,
+               doc: /* SKIP: real doc in xterm.c.  */);
+  x_underline_below_font_descent = 0;
+
   /* Tell Emacs about this window system.  */
   Fprovide (Qns, Qnil);
 
diff --git a/src/w32term.c b/src/w32term.c
index 97afb678c1..dec382a2f2 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -2493,7 +2493,9 @@ x_draw_glyph_string (struct glyph_string *s)
                          ROUND ((maximum_descent) / 2), with
                          ROUND (x) = floor (x + 0.5)  */
 
-                      if (x_use_underline_position_properties
+                      if (x_underline_below_font_descent)
+                        position = s->y - s->ybase + font->height;
+                      else if (x_use_underline_position_properties
                           && font->underline_position >= 0)
                         position = font->underline_position;
                       else
@@ -7391,6 +7393,11 @@ the cursor have no effect.  */);
      doc: /* SKIP: real doc in xterm.c.  */);
   x_underline_at_descent_line = 0;
 
+  DEFVAR_BOOL ("x-underline-below-font-descent",
+	       x_underline_below_font_descent
+     doc: /* SKIP: real doc in xterm.c.  */);
+  x_underline_below_font_descent = 0;
+
   DEFVAR_LISP ("x-toolkit-scroll-bars", Vx_toolkit_scroll_bars,
 	       doc: /* SKIP: real doc in xterm.c.  */);
   Vx_toolkit_scroll_bars = Qt;
diff --git a/src/xterm.c b/src/xterm.c
index 0d25c7f1a2..119e9943c2 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3725,7 +3725,9 @@ x_draw_glyph_string (struct glyph_string *s)
                          ROUND ((maximum descent) / 2), with
                          ROUND(x) = floor (x + 0.5)  */
 
-                      if (x_use_underline_position_properties
+                      if (x_underline_below_font_descent && font)
+                        position = s->y - s->ybase + font->height;
+                      else if (x_use_underline_position_properties
                           && font && font->underline_position >= 0)
                         position = font->underline_position;
                       else if (font)
@@ -13251,12 +13253,21 @@ small font display sizes.  */);
 	       x_underline_at_descent_line,
      doc: /* Non-nil means to draw the underline at the same place as the descent line.
 (If `line-spacing' is in effect, that moves the underline lower by
-that many pixels.)
+that many pixels. If this is not what you want, see `x-underline-below-font-descent')
 A value of nil means to draw the underline according to the value of the
 variable `x-use-underline-position-properties', which is usually at the
 baseline level.  The default value is nil.  */);
   x_underline_at_descent_line = false;
 
+  DEFVAR_BOOL ("x-underline-below-font-descent",
+	       x_underline_below_font_descent,
+     doc: /* Non-nil means to draw the underline below the font's descenders.
+(Ignored if `x-underline-at-descent-line' is non-nil)
+A value of nil means to draw the underline according to the value of the
+variable `x-use-underline-position-properties', which is usually at the
+baseline level.  The default value is nil.  */);
+  x_underline_below_font_descent = false;
+
   DEFVAR_BOOL ("x-mouse-click-focus-ignore-position",
 	       x_mouse_click_focus_ignore_position,
     doc: /* Non-nil means that a mouse click to focus a frame does not move point.
-- 
2.15.1