GNU bug report logs - #321
Underline drawing that leaves gaps for descenders

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 321 in the body.
You can then email your comments to 321 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> emacsbugs.donarmstrong.com (full text, mbox):

From: David De La Harpe Golden <david <at> harpegolden.net>
To: submit <at> debbugs.gnu.org
Cc: Kenichi Handa <handa <at> m17n.org>
Subject: Underline drawing that leaves gaps for descenders
Date: Tue, 27 May 2008 05:44:56 +0100

[Message part 1 (text/plain, inline)]

Package: emacs
Version: 23.0.60
Severity: wishlist
Tags: patch

Ideally, underlining would be able to break at (leave gaps for)
descenders (in latin script typically gjpqy and some old-style
numerals), to improve legibility.

See attached screenshots of a proof-of-concept implementation - with
"the quick brown fox..." and the M-x view-hello-file HELLO (I don't know
about actual conventional underline positions for the non-latin scripts,
just provided a source of descenders for testing)

I've attached the proof-of-concept patch where I tried an old "cheat"
way to do this, by drawing the text jittered horizontally and vertically
by one display pixel in background color several times "over" the
underline (and clipped to the underline area) but "under" the final
foreground text.   This is obviously quite inefficient and not perfect
(e.g. will draw underline inside looping descenders if loop is big
enough, and as it stands the patch doesn't jitter prev/next string's
overhanging descenders if any).  I also only used it and tested with the
xft FontBackend.

There are probably many better ways to do this than this patch, but I
only intended it as a proof-of-concept.

[x-underline-break-at-descenders.diff (text/x-diff, inline)]

Index: src/xterm.c
===================================================================
RCS file: /sources/emacs/emacs/src/xterm.c,v
retrieving revision 1.994
diff -U 8 -r1.994 xterm.c
--- src/xterm.c	26 May 2008 11:37:42 -0000	1.994
+++ src/xterm.c	27 May 2008 03:21:33 -0000
@@ -178,16 +178,27 @@
 /* Non-zero means make use of UNDERLINE_POSITION font properties.  */
 
 int x_use_underline_position_properties;
 
 /* Non-zero means to draw the underline at the same place as the descent line.  */
 
 int x_underline_at_descent_line;
 
+/* Require underline to be at least this many screen pixels below baseline
+   This to avoid underline "merging" with the base of letters at small
+   font sizes, particularly when x_use_underline_position_properties is on. */
+
+int x_underline_minimum_display_offset;
+
+/* Non-zero means try to break underline drawing at descenders. Prettier
+   but potentially resource-intensive */
+
+int x_underline_break_at_descenders;
+
 /* This is a chain of structures for all the X displays currently in
    use.  */
 
 struct x_display_info *x_display_list;
 
 /* This is a list of cons cells, each of the form (NAME
    . FONT-LIST-CACHE), one for each element of x_display_list and in
    the same order.  NAME is the name of the frame.  FONT-LIST-CACHE
@@ -932,16 +943,17 @@
 
 static void x_set_glyph_string_clipping P_ ((struct glyph_string *));
 static void x_set_glyph_string_gc P_ ((struct glyph_string *));
 static void x_draw_glyph_string_background P_ ((struct glyph_string *,
 						int));
 static void x_draw_glyph_string_foreground P_ ((struct glyph_string *));
 static void x_draw_composite_glyph_string_foreground P_ ((struct glyph_string *));
 static void x_draw_glyph_string_box P_ ((struct glyph_string *));
+static void x_draw_glyph_string_underline P_ ((struct glyph_string *));
 static void x_draw_glyph_string  P_ ((struct glyph_string *));
 static void x_compute_glyph_string_overhangs P_ ((struct glyph_string *));
 static void x_set_cursor_gc P_ ((struct glyph_string *));
 static void x_set_mode_line_face_gc P_ ((struct glyph_string *));
 static void x_set_mouse_face_gc P_ ((struct glyph_string *));
 static int x_alloc_lighter_color P_ ((struct frame *, Display *, Colormap,
 				      unsigned long *, double, int));
 static void x_setup_relief_color P_ ((struct frame *, struct relief *,
@@ -2615,16 +2627,158 @@
       if (background_width > 0)
 	x_draw_glyph_string_bg_rect (s, x, s->y, background_width, s->height);
     }
 
   s->background_filled_p = 1;
 }
 
 
+/* Draw underline for glyph string S. */
+
+static void
+x_draw_glyph_string_underline (s)
+     struct glyph_string *s;
+{
+  /* Draw underline. */
+  if (!s->for_overlaps && s->face->underline_p)
+    {
+      unsigned long thickness, position;
+      int y;
+
+      if (s->prev && s->prev->face->underline_p)
+	{
+	  /* We use the same underline style as the previous one.  */
+	  thickness = s->prev->underline_thickness;
+	  position = s->prev->underline_position;
+	}
+      else
+	{
+	  /* Get the underline thickness.  Default is 1 pixel.  */
+	  if (s->font && s->font->underline_thickness > 0)
+	    thickness = s->font->underline_thickness;
+	  else
+	    thickness = 1;
+	  if (x_underline_at_descent_line)
+	    position = (s->height - thickness) - (s->ybase - s->y);
+	  else
+	    {
+	      /* Get the underline position.  This is the recommended
+		 vertical offset in pixels from the baseline to the top of
+		 the underline.  This is a signed value according to the
+		 specs, and its default is
+
+		 ROUND ((maximum descent) / 2), with
+		 ROUND(x) = floor (x + 0.5)  */
+
+	      if (x_use_underline_position_properties
+		  && s->font && s->font->underline_position >= 0)
+		position = s->font->underline_position;
+	      else if (s->font)
+		position = (s->font->descent + 1) / 2;
+	    }
+	  if (x_underline_minimum_display_offset)
+	    position = max (position, eabs (x_underline_minimum_display_offset));
+	}
+      /* Check the sanity of thickness and position.  We should
+	 avoid drawing underline out of the current line area.  */
+      if (s->y + s->height <= s->ybase + position)
+	position = (s->height - 1) - (s->ybase - s->y);
+      if (s->y + s->height < s->ybase + position + thickness)
+	thickness = (s->y + s->height) - (s->ybase + position);
+      s->underline_thickness = thickness;
+      s->underline_position = position;
+      y = s->ybase + position;
+      if (s->face->underline_defaulted_p)
+	XFillRectangle (s->display, s->window, s->gc,
+			s->x, y, s->width, thickness);
+      else
+	{
+	  XGCValues xgcv;
+	  XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
+	  XSetForeground (s->display, s->gc, s->face->underline_color);
+	  XFillRectangle (s->display, s->window, s->gc,
+			  s->x, y, s->width, thickness);
+	  XSetForeground (s->display, s->gc, xgcv.foreground);
+	}
+
+      /* Break underline at descenders, by cheating:
+	 Repeatedly draws glyphs in background color, jittered
+	 left/right/up/down, clipped to underline area.
+	 Yes, this method is very wasteful.
+	 Underline drawing must happen before glyph foreground drawing
+	 for this trick to be successful, this is presently ensured in
+	 x_draw_glyph_string.
+	 KLUDGE cross-ref: xft fontbackend xftfont_get_colors() was too clever
+	 - caches colors based on comparisons of GC and face info, so disabled
+	 caching in that fontbackend for this to work.
+
+	 Really, underline (and overline and strikethrough) drawing itself
+	 (though maybe not positioning computation) might belong in the
+	 fontbackends. */
+
+      /* Only try to break for CHAR_GLYPH and COMPOSITE_GLYPH. */
+      if (x_underline_break_at_descenders &&
+	  ((s->first_glyph->type == CHAR_GLYPH) ||
+	   (s->first_glyph->type == COMPOSITE_GLYPH)))
+	{
+	  int x_save, y_save, ybase_save;
+	  int x_jitter, y_jitter;
+	  /* maybe shouldn't merrily modify fields of s, but currently works. */
+	  XGCValues xgcv;
+	  XRectangle r;
+
+	  XGetGCValues (s->display, s->gc, GCForeground | GCBackground, &xgcv);
+	  x_save = s->x;
+	  y_save = s->y;
+	  ybase_save = s->ybase;
+
+	  /* temprorarily clip to underline area */
+	  r.x = s->x;
+	  r.y = y;
+	  r.width = s->width;
+	  r.height = thickness;
+	  XSetClipRectangles (s->display, s->gc, 0, 0, &r, 1, Unsorted);
+
+	  XSetForeground (s->display, s->gc, xgcv.background);
+	  if (s->face->underline_defaulted_p)
+	    XSetBackground (s->display, s->gc, xgcv.foreground);
+	  else
+	    XSetBackground (s->display, s->gc, s->face->underline_color);
+
+	  for (y_jitter = -1; y_jitter <= 1; y_jitter++) {
+	    s->y = y_save + y_jitter;
+	    s->ybase = ybase_save + y_jitter;
+	    for (x_jitter = -1 ; x_jitter <= 1; x_jitter++) {
+	      s->x = x_save + x_jitter;
+	      switch (s->first_glyph->type)
+		{
+		case CHAR_GLYPH:
+		  x_draw_glyph_string_foreground (s);
+		  break;
+		case COMPOSITE_GLYPH:
+		  x_draw_composite_glyph_string_foreground (s);
+		  break;
+		}
+	    }
+	  }
+
+	  s->ybase = ybase_save;
+	  s->y = y_save;
+	  s->x = x_save;
+	  XSetBackground (s->display, s->gc, xgcv.background);
+	  XSetForeground (s->display, s->gc, xgcv.foreground);
+	  XSetClipRectangles (s->display, s->gc, 0, 0, s->clip, s->num_clips, Unsorted);
+	}
+
+      s->underline_drawn_p = 1;
+    }
+}
+
+
 /* Draw glyph string S.  */
 
 static void
 x_draw_glyph_string (s)
      struct glyph_string *s;
 {
   int relief_drawn_p = 0;
 
@@ -2682,94 +2836,51 @@
     case STRETCH_GLYPH:
       x_draw_stretch_glyph_string (s);
       break;
 
     case CHAR_GLYPH:
       if (s->for_overlaps)
 	s->background_filled_p = 1;
       else
-	x_draw_glyph_string_background (s, 0);
+	{
+	  /* Draw fancy broken underline before foreground */
+	  if (s->face->underline_p && x_underline_break_at_descenders)
+	    {
+	      x_draw_glyph_string_background (s, 1); /* must redraw bg */
+	      x_draw_glyph_string_underline (s);
+	    }
+	  else
+	    x_draw_glyph_string_background (s, 0);
+	}
       x_draw_glyph_string_foreground (s);
       break;
 
     case COMPOSITE_GLYPH:
       if (s->for_overlaps || s->gidx > 0)
 	s->background_filled_p = 1;
       else
+	{
 	x_draw_glyph_string_background (s, 1);
+	/* Draw fancy broken underline before foreground */
+      	if (s->face->underline_p && x_underline_break_at_descenders)
+	  x_draw_glyph_string_underline (s);
+	}
       x_draw_composite_glyph_string_foreground (s);
       break;
 
     default:
       abort ();
     }
 
   if (!s->for_overlaps)
     {
-      /* Draw underline.  */
-      if (s->face->underline_p)
-	{
-	  unsigned long thickness, position;
-	  int y;
-
-	  if (s->prev && s->prev->face->underline_p)
-	    {
-	      /* We use the same underline style as the previous one.  */
-	      thickness = s->prev->underline_thickness;
-	      position = s->prev->underline_position;
-	    }
-	  else
-	    {
-	      /* Get the underline thickness.  Default is 1 pixel.  */
-	      if (s->font && s->font->underline_thickness > 0)
-		thickness = s->font->underline_thickness;
-	      else
-		thickness = 1;
-	      if (x_underline_at_descent_line)
-		position = (s->height - thickness) - (s->ybase - s->y);
-	      else
-		{
-		  /* Get the underline position.  This is the recommended
-		     vertical offset in pixels from the baseline to the top of
-		     the underline.  This is a signed value according to the
-		     specs, and its default is
-
-		     ROUND ((maximum descent) / 2), with
-		     ROUND(x) = floor (x + 0.5)  */
-
-		  if (x_use_underline_position_properties
-		      && s->font && s->font->underline_position >= 0)
-		    position = s->font->underline_position;
-		  else if (s->font)
-		    position = (s->font->descent + 1) / 2;
-		}
-	    }
-	  /* Check the sanity of thickness and position.  We should
-	     avoid drawing underline out of the current line area.  */
-	  if (s->y + s->height <= s->ybase + position)
-	    position = (s->height - 1) - (s->ybase - s->y);
-	  if (s->y + s->height < s->ybase + position + thickness)
-	    thickness = (s->y + s->height) - (s->ybase + position);
-	  s->underline_thickness = thickness;
-	  s->underline_position = position;
-	  y = s->ybase + position;
-	  if (s->face->underline_defaulted_p)
-	    XFillRectangle (s->display, s->window, s->gc,
-			    s->x, y, s->background_width, thickness);
-	  else
-	    {
-	      XGCValues xgcv;
-	      XGetGCValues (s->display, s->gc, GCForeground, &xgcv);
-	      XSetForeground (s->display, s->gc, s->face->underline_color);
-	      XFillRectangle (s->display, s->window, s->gc,
-			      s->x, y, s->background_width, thickness);
-	      XSetForeground (s->display, s->gc, xgcv.foreground);
-	    }
-	}
+      /* Draw underline if not yet drawn */
+      if (s->face->underline_p && !s->underline_drawn_p)
+	x_draw_glyph_string_underline (s);
 
       /* Draw overline.  */
       if (s->face->overline_p)
 	{
 	  unsigned long dy = 0, h = 1;
 
 	  if (s->face->overline_color_defaulted_p)
 	    XFillRectangle (s->display, s->window, s->gc, s->x, s->y + dy,
@@ -10778,27 +10889,45 @@
   staticpro (&last_mouse_press_frame);
   last_mouse_press_frame = Qnil;
 
   DEFVAR_BOOL ("x-use-underline-position-properties",
 	       &x_use_underline_position_properties,
      doc: /* *Non-nil means make use of UNDERLINE_POSITION font properties.
 A value of nil means ignore them.  If you encounter fonts with bogus
 UNDERLINE_POSITION font properties, for example 7x13 on XFree prior
-to 4.1, set this to nil.  */);
+to 4.1, set this to nil. Variable `x-underline-minimum-display-offset' may
+be used to override the font's UNDERLINE_POSITION for small font display
+sizes. */);
   x_use_underline_position_properties = 1;
 
   DEFVAR_BOOL ("x-underline-at-descent-line",
 	       &x_underline_at_descent_line,
      doc: /* *Non-nil means to draw the underline at the same place as the descent line.
 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 = 0;
 
+  DEFVAR_INT ("x-underline-minimum-display-offset",
+	       &x_underline_minimum_display_offset,
+     doc: /* *When > 0, underline is drawn at least that many screen pixels below baseline.
+This can improve legibility of underlined text at small font sizes,
+particularly when using variable `x-use-underline-position-properties'
+with fonts that specify an UNDERLINE_POSITION relatively close to the
+baseline. The default value is 0. */);
+  x_underline_minimum_display_offset = 0;
+
+  DEFVAR_BOOL ("x-underline-break-at-descenders",
+	       &x_underline_break_at_descenders,
+     doc: /* *Non-nil means to break underline where it crosses text descenders e.g. gjpqy.
+This improves legibility.  Note that the current implementation may be slow,
+particularly on remote displays. The default value is nil. */);
+  x_underline_break_at_descenders = 0;
+
   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.
 This variable is only used when the window manager requires that you
 click on a frame to select it (give it focus).  In that case, a value
 of nil, means that the selected window and cursor position changes to
 reflect the mouse click position, while a non-nil value means that the
 selected window or cursor position is preserved.  */);
Index: src/dispextern.h
===================================================================
RCS file: /sources/emacs/emacs/src/dispextern.h,v
retrieving revision 1.245
diff -U 8 -r1.245 dispextern.h
--- src/dispextern.h	22 May 2008 14:52:03 -0000	1.245
+++ src/dispextern.h	27 May 2008 03:21:35 -0000
@@ -1172,16 +1172,19 @@
 
   /* 1 means this glyph strings face has to be drawn to the right end
      of the window's drawing area.  */
   unsigned extends_to_end_of_line_p : 1;
 
   /* 1 means the background of this string has been drawn.  */
   unsigned background_filled_p : 1;
 
+  /* 1 means the underline of this string has been drawn. */
+  unsigned underline_drawn_p : 1;
+
   /* 1 means glyph string must be drawn with 16-bit functions.  */
   unsigned two_byte_p : 1;
 
   /* 1 means that the original font determined for drawing this glyph
      string could not be loaded.  The member `font' has been set to
      the frame's default font in this case.  */
   unsigned font_not_found_p : 1;
 
Index: src/xftfont.c
===================================================================
RCS file: /sources/emacs/emacs/src/xftfont.c,v
retrieving revision 1.9
diff -U 8 -r1.9 xftfont.c
--- src/xftfont.c	25 May 2008 11:04:53 -0000	1.9
+++ src/xftfont.c	27 May 2008 03:21:36 -0000
@@ -74,17 +74,17 @@
 static void
 xftfont_get_colors (f, face, gc, xftface_info, fg, bg)
      FRAME_PTR f;
      struct face *face;
      GC gc;
      struct xftface_info *xftface_info;
      XftColor *fg, *bg;
 {
-  if (xftface_info && face->gc == gc)
+  if ( 0 && xftface_info && face->gc == gc) /* 0: Disabled for underline breaking kludge */
     {
       *fg = xftface_info->xft_fg;
       if (bg)
 	*bg = xftface_info->xft_bg;
     }
   else
     {
       XGCValues xgcv;
Index: lisp/cus-start.el
===================================================================
RCS file: /sources/emacs/emacs/lisp/cus-start.el,v
retrieving revision 1.122
diff -U 8 -r1.122 cus-start.el
--- lisp/cus-start.el	6 May 2008 07:57:29 -0000	1.122
+++ lisp/cus-start.el	27 May 2008 03:21:36 -0000
@@ -388,16 +388,18 @@
 				 (repeat (directory :format "%v")))
 	     (x-gtk-use-old-file-dialog menu boolean "22.1")
 	     (x-gtk-show-hidden-files menu boolean "22.1")
 	     (x-gtk-file-dialog-help-text menu boolean "22.1")
 	     (x-gtk-whole-detached-tool-bar x boolean "22.1")
 	     ;; xterm.c
 	     (x-use-underline-position-properties display boolean "22.1")
 	     (x-underline-at-descent-line display boolean "22.1")
+	     (x-underline-minimum-display-offset display integer "23.1")
+	     (x-underline-break-at-descenders display boolean "23.1")
 	     (x-stretch-cursor display boolean "21.1")))
       this symbol group type standard version native-p
       ;; This function turns a value
       ;; into an expression which produces that value.
       (quoter (lambda (sexp)
 		(if (or (memq sexp '(t nil))
 			(keywordp sexp)
 			(and (listp sexp)

[with_underline_desc_break_qbf_bw_mo1.png (image/png, inline)]

[without_underline_desc_break_qbf_bw_mo1.png (image/png, inline)]

[with_underline_desc_break_hello_bw_mo2.png (image/png, inline)]

[with_underline_desc_break_hello_color_mo2.png (image/png, inline)]

Message #10 received at 321-close <at> debbugs.gnu.org (full text, mbox):

From: Lars Magne Ingebrigtsen <larsi <at> gnus.org>
To: David De La Harpe Golden <david <at> harpegolden.net>
Cc: 321-close <at> debbugs.gnu.org, Kenichi Handa <handa <at> m17n.org>
Subject: Re: bug#321: Underline drawing that leaves gaps for descenders
Date: Wed, 11 Apr 2012 14:08:34 +0200

David De La Harpe Golden <david <at> harpegolden.net> writes:

> Ideally, underlining would be able to break at (leave gaps for)
> descenders (in latin script typically gjpqy and some old-style
> numerals), to improve legibility.

Looking at the images, it does improve legibility in some cases, but in
other cases it looks kinda awkward -- as if the underlining stops and
then starts again.

So I don't really feel that something like this would be a display
improvement.

-- 
(domestic pets only, the antidote for overdose, milk.)
  bloggy blog http://lars.ingebrigtsen.no/

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.