Re: HiDPI support for wave style underlines

[Stephen Pegoraro]

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 <address@hidden> 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 <address@hidden> wrote:
>>> From: Stephen Pegoraro <address@hidden>
>>> 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.

0001-Implement-HiDPI-support-for-wave-style-underlines (1).patch
Description: Text Data