unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Stephen Pegoraro <spegoraro@tutive.com>
Cc: emacs-devel@gnu.org
Subject: Re: HiDPI support for wave style underlines
Date: Sat, 29 Jul 2017 10:13:07 +0300	[thread overview]
Message-ID: <83wp6r3ed8.fsf@gnu.org> (raw)
In-Reply-To: <CAPzg8ESH0Sr=jH5=Uj3bV1iDoqWvWu1x1nKGLfkGSkiszgfHKw@mail.gmail.com> (message from Stephen Pegoraro on Sat, 29 Jul 2017 11:29:43 +0800)

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



  reply	other threads:[~2017-07-29  7:13 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-29  3:29 HiDPI support for wave style underlines Stephen Pegoraro
2017-07-29  7:13 ` Eli Zaretskii [this message]
     [not found]   ` <CAPzg8ETxJYr8Gw0Pag5zpWqA7aap_kduwuGvJaxAT5yvPhNd6w@mail.gmail.com>
2017-07-29  9:12     ` Stephen Pegoraro
2017-08-18  8:17       ` Eli Zaretskii
2017-08-21 12:20         ` Yuri D'Elia
2017-08-21 12:32           ` Yuri D'Elia
2017-08-21 14:49             ` Eli Zaretskii
2017-08-27  4:26               ` Stephen Pegoraro

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83wp6r3ed8.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    --cc=spegoraro@tutive.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).