HiDPI support for wave style underlines

HiDPI support for wave style underlines

[Stephen Pegoraro]

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

Hi,

I have made an attempt at implementing scaled drawing of wave style
underlines for hidpi displays, X only for now.
This is my first contribution to emacs so any feedback would be great!
I've followed the commit and patch style from CONTRIBUTE as closely as
I could but let me know if there's any issue.

It works by determining a scale factor from x_display_info's resx and
resy members then setting the wave_height and wave_length values
accordingly. Thickness was added using XSetLineAttributes and scaled
as well. Tested on displays with various scales from 1 to 3.


Cheers,
Steve Pegoraro

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

From bc71dbc78a455b46a91619bcd38af45e4e9831fb 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.c (x_draw_underwave): Compute height, length and thickness
based on scale factor.
(x_get_scale_factor): New function.
---
 src/xterm.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/src/xterm.c b/src/xterm.c
index a214cd8103..5476c22a0b 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,15 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
+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);
+}
+
 /*
    Draw a wavy line under S. The wave fills wave_height pixels from y0.
 
@@ -3485,11 +3492,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 */
+    int scale = x_get_scale_factor (s->display);
+    int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
+
 #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 +3510,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 +3544,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;
-- 
2.13.3

Re: HiDPI support for wave style underlines

[Eli Zaretskii]

> 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.

Re: HiDPI support for wave style underlines

[Stephen Pegoraro]

[-- 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

Re: HiDPI support for wave style underlines

[Eli Zaretskii]

> From: Stephen Pegoraro <spegoraro@tutive.com>
> Date: Sat, 29 Jul 2017 17:12:20 +0800
> 
> 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.

Sorry for the long delay.

Thanks, I pushed this with minor changes.  Specifically, the function
you added is only used in the same file where it is defined, so it
should be static and its prototype doesn't need to be in xterm.h.
Also, for returning just 2 values, it is better to provide 2 pointer
arguments than to invent a new struct.

Re: HiDPI support for wave style underlines

[Yuri D'Elia]

On Fri, Aug 18 2017, Eli Zaretskii wrote:
>> 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.
>
> Sorry for the long delay.
>
> Thanks, I pushed this with minor changes.  Specifically, the function
> you added is only used in the same file where it is defined, so it
> should be static and its prototype doesn't need to be in xterm.h.
> Also, for returning just 2 values, it is better to provide 2 pointer
> arguments than to invent a new struct.

I actually get a floating point exception with this since I rebuilt
emacs on the 19th. I'm rebuilding emacs now with debug to check where
it's located, but I pinpointed it to flyspell so...

Re: HiDPI support for wave style underlines

[Yuri D'Elia]

On Mon, Aug 21 2017, Yuri D'Elia wrote:
> I actually get a floating point exception with this since I rebuilt
> emacs on the 19th. I'm rebuilding emacs now with debug to check where
> it's located, but I pinpointed it to flyspell so...

Thread 1 "emacs" received signal SIGFPE, Arithmetic exception.
0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
3541	  x1 = x0 - (x0 % dx);
(gdb) where
#0  0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
#1  0x0000000000518877 in x_draw_glyph_string (s=0x7fffffffbe20) at xterm.c:3685
#2  0x00000000004805d4 in draw_glyphs (w=0x148dc30 <bss_sbrk_buffer+8070032>, x=233, row=0x2eda080, area=TEXT_AREA, start=7, end=26, hl=DRAW_NORMAL_TEXT, overlaps=0) at xdisp.c:26595
(gdb) p dx
$1 = 0

Re: HiDPI support for wave style underlines

[Eli Zaretskii]

> From: Yuri D'Elia <wavexx@thregr.org>
> Date: Mon, 21 Aug 2017 14:32:07 +0200
> 
> On Mon, Aug 21 2017, Yuri D'Elia wrote:
> > I actually get a floating point exception with this since I rebuilt
> > emacs on the 19th. I'm rebuilding emacs now with debug to check where
> > it's located, but I pinpointed it to flyspell so...
> 
> Thread 1 "emacs" received signal SIGFPE, Arithmetic exception.
> 0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
> 3541	  x1 = x0 - (x0 % dx);
> (gdb) where
> #0  0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
> #1  0x0000000000518877 in x_draw_glyph_string (s=0x7fffffffbe20) at xterm.c:3685
> #2  0x00000000004805d4 in draw_glyphs (w=0x148dc30 <bss_sbrk_buffer+8070032>, x=233, row=0x2eda080, area=TEXT_AREA, start=7, end=26, hl=DRAW_NORMAL_TEXT, overlaps=0) at xdisp.c:26595
> (gdb) p dx
> $1 = 0

Thanks, I tried to correct this with an obvious fix.

Re: HiDPI support for wave style underlines

[Stephen Pegoraro]

It still needs a fair bit of work on my end, the current
implementation is pretty naive.
Certain fonts are also getting clipping issues on HiDPI displays. I've
been pretty short of time this month but will try to get something
better going in the next few days!

On 21 August 2017 at 22:49, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Yuri D'Elia <wavexx@thregr.org>
>> Date: Mon, 21 Aug 2017 14:32:07 +0200
>>
>> On Mon, Aug 21 2017, Yuri D'Elia wrote:
>> > I actually get a floating point exception with this since I rebuilt
>> > emacs on the 19th. I'm rebuilding emacs now with debug to check where
>> > it's located, but I pinpointed it to flyspell so...
>>
>> Thread 1 "emacs" received signal SIGFPE, Arithmetic exception.
>> 0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
>> 3541    x1 = x0 - (x0 % dx);
>> (gdb) where
>> #0  0x00000000005181a3 in x_draw_underwave (s=0x7fffffffbe20) at xterm.c:3541
>> #1  0x0000000000518877 in x_draw_glyph_string (s=0x7fffffffbe20) at xterm.c:3685
>> #2  0x00000000004805d4 in draw_glyphs (w=0x148dc30 <bss_sbrk_buffer+8070032>, x=233, row=0x2eda080, area=TEXT_AREA, start=7, end=26, hl=DRAW_NORMAL_TEXT, overlaps=0) at xdisp.c:26595
>> (gdb) p dx
>> $1 = 0
>
> Thanks, I tried to correct this with an obvious fix.
>