Re: HiDPI support for wave style underlines

[Stephen Pegoraro <spegoraro@tutive.com>]

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

Hi Eli,

Thanks for reviewing!

The patch isn't _only_ for hidpi displays but I can see how my
original wording comes across that way. It provides correct scaled
underlining for all displays, hidpi or otherwise. I meant to say that
I have only implemented this for the Xlib side of things, no W32 or
mac.

Good point regarding the emacs_abort(), I honestly didn't think through that.
I have modified x_get_scale_factor to return a new struct
x_display_scale with x and y components which are used in the wave
length and height calculation.
Indentations and comment grammar have been fixed up as well.


Cheers,
Steve

On 29 July 2017 at 15:47, Stephen Pegoraro <spegoraro@tutive.com> wrote:
> Hi Eli,
>
> Thanks for reviewing!
>
> The patch isn't _only_ for hidpi displays but I can see how my
> original wording comes across that way. It provides correct scaled
> underlining for all displays, hidpi or otherwise. I meant to say that
> I have only implemented this for the Xlib side of things, no W32 or
> mac.
>
> Good point regarding the emacs_abort(), I honestly didn't think through that.
> I have modified x_get_scale_factor to return a new struct
> x_display_scale with x and y components which are used in the wave
> length and height calculation.
> Indentations and comment grammar have been fixed up as well.
>
>
> Cheers,
> Steve
>
> On 29 July 2017 at 15:13, Eli Zaretskii <eliz@gnu.org> wrote:
>>> From: Stephen Pegoraro <spegoraro@tutive.com>
>>> Date: Sat, 29 Jul 2017 11:29:43 +0800
>>>
>>> I have made an attempt at implementing scaled drawing of wave style
>>> underlines for hidpi displays, X only for now.
>>
>> Thanks.  But why do you say this is only for HiDPI displays?  What
>> aspects of the change are specific to that kind of display?
>>
>>> +static int x_get_scale_factor(Display *disp)
>>> +{
>>> +    struct x_display_info * dpyinfo = x_display_info_for_display (disp);
>>> +    if (!dpyinfo)
>>> +        emacs_abort ();
>>> +
>>> +    return floor(dpyinfo->resy / 96);
>>> +}
>>
>> The indentation here is not according to our conventions: we use the
>> offset of 2, not 4.
>>
>> Also, why call emacs_abort here?  I think it's better to return 1
>> instead.  Think about this being called from a daemon, or during early
>> stages of Emacs startup, when some of the stuff is not yet set up.
>>
>> And finally, why do you use only resy?  Isn't it better to return 2
>> values, for X and Y separately, and use them accordingly in the
>> calling function?
>>
>>>  static void
>>>  x_draw_underwave (struct glyph_string *s)
>>>  {
>>> -  int wave_height = 3, wave_length = 2;
>>> +    /* Adjust for scale/HiDPI */
>>
>> A comment should end with a period, and should have 2 spaces after the
>> period.  Like this:
>>
>>      /* Adjust for scale/HiDPI.  */
>>
>>> +    int scale = x_get_scale_factor (s->display);
>>> +    int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
>>
>> Same issue with indentation here.
>>
>> Thanks again for working on this.

[-- Attachment #2: 0001-Implement-HiDPI-support-for-wave-style-underlines (1).patch --]
[-- Type: text/x-patch, Size: 3415 bytes --]

From 7d657b57e63587293b8ee753fe10971c1b0d1d55 Mon Sep 17 00:00:00 2001
From: Stephen Pegoraro <spegoraro@tutive.com>
Date: Sat, 29 Jul 2017 11:12:03 +0800
Subject: [PATCH] Implement HiDPI support for wave style underlines

* src/xterm.h (x_get_scale_factor): New function declaration.
* src/xterm.c (x_draw_underwave): Compute height, length and thickness
based on scale factor.
(x_get_scale_factor): New function.
---
 src/xterm.c | 27 ++++++++++++++++++++++-----
 src/xterm.h |  8 ++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/src/xterm.c b/src/xterm.c
index a214cd8103..3c0440379b 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -23,9 +23,7 @@ along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.  */
 #include <config.h>
 #include <stdio.h>
 #include <stdlib.h>
-#ifdef USE_CAIRO
 #include <math.h>
-#endif
 
 #include "lisp.h"
 #include "blockinput.h"
@@ -3475,6 +3473,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
+struct x_display_scale x_get_scale_factor(Display *disp)
+{
+  int base_res = 96;
+  struct x_display_scale scale = { 1, 1 };
+  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
+
+  if (!dpyinfo)
+    return scale;
+
+  scale.x = floor(dpyinfo->resx / base_res);
+  scale.y = floor(dpyinfo->resy / base_res);
+
+  return scale;
+}
+
 /*
    Draw a wavy line under S. The wave fills wave_height pixels from y0.
 
@@ -3485,11 +3498,13 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
     wave_height = 3  | *   *   *   *
 
 */
-
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  int wave_height = 3, wave_length = 2;
+  /* Adjust for scale/HiDPI.  */
+  struct x_display_scale scale = x_get_scale_factor (s->display);
+  int wave_height = 3 * scale.y, wave_length = 2 * scale.x, thickness = scale.y;
+
 #ifdef USE_CAIRO
   x_draw_horizontal_wave (s->f, s->gc, s->x, s->ybase - wave_height + 3,
 			  s->width, wave_height, wave_length);
@@ -3501,7 +3516,7 @@ x_draw_underwave (struct glyph_string *s)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase - wave_height + 3;
+  y0 = s->ybase + wave_height / 2;
   width = s->width;
   xmax = x0 + width;
 
@@ -3535,6 +3550,8 @@ x_draw_underwave (struct glyph_string *s)
 
   while (x1 <= xmax)
     {
+      XSetLineAttributes (s->display, s->gc, thickness, LineSolid, CapButt,
+                          JoinRound);
       XDrawLine (s->display, FRAME_X_DRAWABLE (s->f), s->gc, x1, y1, x2, y2);
       x1  = x2, y1 = y2;
       x2 += dx, y2 = y0 + odd*dy;
diff --git a/src/xterm.h b/src/xterm.h
index 803feda99f..ffd19dcb8f 100644
--- a/src/xterm.h
+++ b/src/xterm.h
@@ -172,6 +172,13 @@ Status x_parse_color (struct frame *f, const char *color_name,
 		      XColor *color);
 
 \f
+struct x_display_scale
+{
+  int x;
+  int y;
+};
+
+\f
 /* For each X display, we have a structure that records
    information about it.  */
 
@@ -489,6 +496,7 @@ extern bool use_xim;
 /* This is a chain of structures for all the X displays currently in use.  */
 extern struct x_display_info *x_display_list;
 
+extern struct x_display_scale x_get_scale_factor(Display *);
 extern struct x_display_info *x_display_info_for_display (Display *);
 extern struct frame *x_top_window_to_frame (struct x_display_info *, int);
 extern struct x_display_info *x_term_init (Lisp_Object, char *, char *);
-- 
2.13.3