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