GNU bug report logs - #37770
[PATCH] Expose scale factor through the redisplay interface

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 37770 in the body.
You can then email your comments to 37770 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Expose scale factor through the redisplay interface
Date: Tue, 15 Oct 2019 19:29:58 -0300

[Message part 1 (text/plain, inline)]

I need this in order to move forward #37689.

Besides I did some related changes proposed in #37752, which I'm no
closing in favor of this one.

Here is the commit message:

    * src/dispextern.h: add get_scale_factor API.
    * src/xterm.c:
      - Unify usages of xg_get_scale and x_get_scale_factor into a
        consolidated x_get_scale_factor that is exported by the rif.
      - Simplify scale inferring logic (see note below).
    * src/w32term.c:
      - Simplify w32_get_scale_factor in a similar way (see note).
      - Add it to the rif.
    * src/nsterm.m:
      - Just add NULL to the rif since there are no uses of any scale
        factor here. Clients are expected to interpret a missing API
        as meaning scale factor = 1.

    Note: both x_get_scale_factor and w32_get_scale_factor computed
    distinct scales for x and y by taking the ratio between effective
    resolution in each direction and a standard 96 dpi resolution.  Since
    this ratio is then truncated to an integer (the floor) it seems to me
    that there is no sensible possibility that these two numbers
    diverge. Moreover, modern toolkits report one number as scale factor
    and we need a common interface here. For those reasons I'm arbitrarily
    picking the horizontal scale factor as THE scale factor.

Best regards
--
Carlos

[0001-Expose-scale-factor-through-the-redisplay-interface.patch (text/x-patch, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Re: [PATCH] Expose scale factor through the redisplay interface
Date: Tue, 15 Oct 2019 21:32:25 -0300

[Message part 1 (text/plain, inline)]

I have updated my patch a bit:

1. Instead of setting the new API to NULL when the scale factor is
ignored or unsupported by the backend, I'm setting it to a dummy
function returning 1. This keeps backwards compatibility and is more
convenient for the user of the interface.

2. get_scale_factor now returns a double. Even though current scale
factors are integers, this is an interface that will better have to
make it through the forthcoming era of fractional scaling.

[0001-Expose-scale-factor-through-the-redisplay-interface.patch (text/x-patch, attachment)]

Message #11 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Carlos Pita <carlosjosepita <at> gmail.com>
Cc: 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Wed, 16 Oct 2019 10:51:35 +0200

>>>>> On Tue, 15 Oct 2019 21:32:25 -0300, Carlos Pita <carlosjosepita <at> gmail.com> said:

    Carlos> I have updated my patch a bit:
    Carlos> 1. Instead of setting the new API to NULL when the scale factor is
    Carlos> ignored or unsupported by the backend, I'm setting it to a dummy
    Carlos> function returning 1. This keeps backwards compatibility and is more
    Carlos> convenient for the user of the interface.

    Carlos> 2. get_scale_factor now returns a double. Even though current scale
    Carlos> factors are integers, this is an interface that will better have to
    Carlos> make it through the forthcoming era of fractional scaling.

    Carlos> From 5b6e276e3dd9b4cb06bcf0e21e99227b2134a8bf Mon Sep 17 00:00:00 2001
    Carlos> From: memeplex <carlosjosepita <at> gmail.com>
    Carlos> Date: Tue, 15 Oct 2019 19:14:03 -0300
    Carlos> Subject: [PATCH] Expose scale factor through the redisplay interface

    Carlos> * src/dispextern.h: add get_scale_factor API.
    Carlos> * src/xterm.c:
    Carlos>   - Unify usages of xg_get_scale and x_get_scale_factor into a
    Carlos>     consolidated x_get_scale_factor that is exported by the rif.
    Carlos>   - Simplify scale inferring logic (see note below).
    Carlos> * src/w32term.c:
    Carlos>   - Simplify w32_get_scale_factor in a similar way (see note).
    Carlos>   - Add it to the rif.
    Carlos> * src/nsterm.m:
    Carlos>   - Just add a dummy implementation that always return 1 to the rif,
    Carlos>     since there are no uses of any scale factor here.

This commit message doesnʼt use the ChangeLog format. There are
various help functions in vc for creating that in the correct
format. Either select some files, run 'vc-diff', and then
'diff-add-change-log-entries-other-window' (bound to C-x 4 A) to
prepare it, or, when youʼre committing, you can run C-c C-w, which
does the same.

    Carlos> Note 1: both x_get_scale_factor and w32_get_scale_factor computed
    Carlos> distinct scales for x and y by taking the ratio between effective
    Carlos> resolution in each direction and a standard 96 dpi resolution.  Since
    Carlos> this ratio is then truncated to an integer (the floor) it seems to me
    Carlos> that there is no sensible possibility that these two numbers
    Carlos> diverge. Moreover, modern toolkits report one number as scale factor
    Carlos> and we need a common interface here. For those reasons I'm arbitrarily
    Carlos> picking the horizontal scale factor as THE scale factor.

Sure.

    Carlos> Note 2: I decided to let get_scale_factor return a double, even tough
    Carlos> factors currently in use are all integers AFAIK. This is in
    Carlos> anticipation of fractional scaling. I believe it's prudent to keep
    Carlos> the interface general in this regard.

Yes. I believe at least KDE already supports fractional scaling, Gnome
canʼt be far behind.

Carlos> diff --git a/src/dispextern.h b/src/dispextern.h
    Carlos> index 0615b16..b93e25f 100644
    Carlos> --- a/src/dispextern.h
    Carlos> +++ b/src/dispextern.h
    Carlos> @@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
    Carlos>  #ifdef HAVE_WINDOW_SYSTEM
 
    Carlos> +  /* Return the scale factor for the screen containing frame F.  */
    Carlos> +  double (*get_scale_factor) (struct frame *f);
    Carlos> +
    Carlos>    /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
    Carlos>    void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
    Carlos>                                struct draw_fringe_bitmap_params *p);
    Carlos> diff --git a/src/nsterm.m b/src/nsterm.m
    Carlos> index 5583c61..6e1b751 100644
    Carlos> --- a/src/nsterm.m
    Carlos> +++ b/src/nsterm.m
    Carlos> @@ -2957,6 +2957,11 @@ so some key presses (TAB) are swallowed by the system.  */
 
    Carlos>     ========================================================================== */
 
    Carlos> +static double
    Carlos> +ns_get_scale_factor (struct frame *f)
    Carlos> +{
    Carlos> +  return 1;  // TODO do we need to do something else here?
    Carlos> +}

As far as I know, macOS *can* scale displays, but generally doesnʼt. I
think using 1 here doesnʼt change anything from the status quo. There
is an API to get the scale factor if it turns out to be needed.

For the rest, it looks ok. Do you plan to make the changes to actually
use the rif interface as part of the same patch?

Robert

Message #14 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Wed, 16 Oct 2019 13:12:22 -0300

[Message part 1 (text/plain, inline)]

> This commit message doesnʼt use the ChangeLog format.

Ahh, I was just informally pattern matching other commit messages I
had seen in your repo. Now I've read [1] (I really like those rules,
will adopt them for other projects too). Nevertheless I'm still unsure
about how to format the notes that go below the actual changes.

Please, tell me what do you think about the new commit message.

> For the rest, it looks ok. Do you plan to make the changes to actually
> use the rif interface as part of the same patch?

No, those are part of bug#37689 [2].

---

[1] https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs
[2] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=37689

[0001-Expose-scale-factor-through-the-redisplay-interface.patch (text/x-patch, attachment)]

Message #17 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Wed, 16 Oct 2019 16:08:47 -0300

[Message part 1 (text/plain, inline)]

I've capitalized changelog entries in the message and also added a
reference to the bug number.

Eli told me that you prefer code comments to long commit messages but
in this case I think the rationale would be lost in fragmentary
comments here and there. It's true that there is still the reference
to this discussion in the commit message, but I believe it's
convenient to quickly get a description of the change using git blame
when browsing the code. If you disagree I will remove the notes from
the commit message and copy them here.

Best regards
--
Carlos

[0001-Expose-scale-factor-through-the-redisplay-interface-.patch (text/x-patch, attachment)]

Message #20 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Carlos Pita <carlosjosepita <at> gmail.com>
Cc: 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Thu, 17 Oct 2019 10:01:40 +0200

>>>>> On Wed, 16 Oct 2019 16:08:47 -0300, Carlos Pita <carlosjosepita <at> gmail.com> said:

    Carlos> I've capitalized changelog entries in the message and also added a
    Carlos> reference to the bug number.

    Carlos> Eli told me that you prefer code comments to long commit messages but
    Carlos> in this case I think the rationale would be lost in fragmentary
    Carlos> comments here and there. It's true that there is still the reference
    Carlos> to this discussion in the commit message, but I believe it's
    Carlos> convenient to quickly get a description of the change using git blame
    Carlos> when browsing the code. If you disagree I will remove the notes from
    Carlos> the commit message and copy them here.

If by 'you' you mean 'Emacs', then yes, putting stuff in the code
is preferred, I think. Of course nothing stops us from doing both.

Carlos> From 01e52de9ce49bc0b3490492891f066d2f9306cf4 Mon Sep 17 00:00:00 2001
    Carlos> From: memeplex <carlosjosepita <at> gmail.com>
    Carlos> Date: Tue, 15 Oct 2019 19:14:03 -0300
    Carlos> Subject: [PATCH] Expose scale factor through the redisplay interface
    Carlos>  (Bug#37770)

    Carlos> * src/dispextern.h (redisplay_interface): Add get_scale_factor API.

    Carlos> * src/xterm.c (x_get_scale_factor): Consolidate with xg_get_scale (see
    Carlos> bug#37752) and export through the rif. Simplify scale inferring
    Carlos> logic (see note 1 below).

2 spaces after '.'

    Carlos> Note 1: both x_get_scale_factor and w32_get_scale_factor computed
    Carlos> distinct scales for x and y by taking the ratio between effective
    Carlos> resolution in each direction and a standard 96 dpi resolution.  Since
    Carlos> this ratio is then truncated to an integer (the floor) it seems to me
    Carlos> that there is no sensible possibility that these two numbers
    Carlos> diverge. Moreover, modern toolkits report one number as scale factor
    Carlos> and we need a common interface here. For those reasons I'm arbitrarily
    Carlos> picking the horizontal scale factor as THE scale factor.

    Carlos> Note 2: I decided to let get_scale_factor return a double, even tough
    Carlos> factors currently in use are all integers AFAIK. This is in
    Carlos> anticipation of fractional scaling. I believe it's prudent to keep
    Carlos> the interface general in this regard.

I tend to lean towards putting this sort of rationale at the beginning
of the commit message, and let the ChangeLog message explain the
mechanics of the change.

Robert

Message #23 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Carlos Pita <carlosjosepita <at> gmail.com>
Cc: rpluim <at> gmail.com, 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Sun, 20 Oct 2019 15:34:31 +0300

> From: Carlos Pita <carlosjosepita <at> gmail.com>
> Date: Wed, 16 Oct 2019 16:08:47 -0300
> Cc: 37770 <at> debbugs.gnu.org
> 
> Eli told me that you prefer code comments to long commit messages but
> in this case I think the rationale would be lost in fragmentary
> comments here and there. It's true that there is still the reference
> to this discussion in the commit message, but I believe it's
> convenient to quickly get a description of the change using git blame
> when browsing the code. If you disagree I will remove the notes from
> the commit message and copy them here.

The explanations should precede the file/function entries part.

> Note 1: both x_get_scale_factor and w32_get_scale_factor computed
> distinct scales for x and y by taking the ratio between effective
> resolution in each direction and a standard 96 dpi resolution.  Since
> this ratio is then truncated to an integer (the floor) it seems to me
> that there is no sensible possibility that these two numbers
> diverge. Moreover, modern toolkits report one number as scale factor
> and we need a common interface here. For those reasons I'm arbitrarily
> picking the horizontal scale factor as THE scale factor.

I'm not sure about this.  Admittedly, I'm not an expert on screen
scales on GUI systems, but why do we need to drop the two scale
factors as part of this change, which is basically just refactoring?
I'd suggest to keep the 2 scale factors for now.  We can always
replace 2 with one later.

> +static double
> +ns_get_scale_factor (struct frame *f)
> +{
> +  return 1;  // TODO do we need to do something else here?

Please use C-style comments, /* Like this.  */, not C++ style.

Thanks.

Message #26 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Robert Pluim <rpluim <at> gmail.com>, 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Sun, 20 Oct 2019 14:22:10 -0300

[Message part 1 (text/plain, inline)]

> I'm not sure about this.  Admittedly, I'm not an expert on screen
> scales on GUI systems, but why do we need to drop the two scale
> factors as part of this change, which is basically just refactoring?
> I'd suggest to keep the 2 scale factors for now.  We can always
> replace 2 with one later.

I might do that, I might rename x/w32_get_scale_factor to
x/w32_get_scale_factor_xy and then define the rif exposed
x/w32_get_scale_factor to call these ones and simply drop the y scale
factor. But I would prefer not to do that because:

1. It adds complexity (more code, two different apis).

2. The only users of the (static) _xy variants would be x/w32_draw_underwave.

3. For me, it's hard to figure out why a screen would offer, say, x
resolution = 2 * 96dpi and y resolution = 3 * 96dpi (and surely we
could accommodate a humble underwave drawing routing if such a screen
happens to exist).

4. The nsterm backend also draws underwaves and doesn't use two
different scale factors (indeed, it doesn't use any scale factor at
all, nor the rest of this backend does).

5. AFAIK, the "toolkit industry" has long been using one number as THE
scale factor. Moreover, most of the time this number is just 1 or 2.

Regarding point 3, it's easier to imagine a screen with 1.99 and 2.01
x and y ratios that get respectively truncated down 1 and 2 x and y
scale factors. Now, the problem is not the different scale factors but
the way they were computed, and this is not solved at all by using 2
scale factors. Maybe we should use round instead of floor there.

I've modified the code comment and the message comment according to
your comments, Eli. I've also changed floor to round.

Best regards
--
Carlos

[0001-Expose-scale-factor-through-the-redisplay-interface-.patch (text/x-patch, attachment)]

Message #29 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Robert Pluim <rpluim <at> gmail.com>, 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Tue, 22 Oct 2019 15:06:49 -0300

I've improved the code comment for the redisplay interface entry in:

https://github.com/memeplex/emacs/commit/d3c66e6eea8a3b6f1a269bd968597a3bd8a3e811

(to generate the patch, add a .patch suffix to that url)

The new comment states:

/* Return the scale factor for the screen containing frame F. All
geometries are reported by the backend using a scale that is
approximately 96dpi x scale_factor. This scale may match
physical resolution or not. */

Some thoughts:

One possibility for the (maybe distant) future, is that this scale
factor api won't be needed any more. Like nsterm does (I believe), all
backends might expose a 1 x 96dpi interface so that the upper layers
can work mostly or fully unaware of the device complexities. But at
this moment the xterm backend goes to lengths in order to revert gtk
auto-scaling and provide a "physical dpi" (well, not necessarily
physical, since there is still randr in the middle)  interface to the
upper layers, thus losing the benefits of gtk auto-scaling, although
with good reason since nowadays gtk is more of a hack to the x11
backend, sniffing the underlying x event loop as it is, than a proper
backend on its own.

Anyway, even if that's not the trend, exposing a higher scale factor
to the upper layers still has the potential benefit of letting those
layers decide how to better use the extra available resolution,
instead of pretending they are drawing to a vintage screen. Whether
this is worthwhile or not I don't know, given that font and image
rendering are the parts most profited from this extra resolution, and
that fact every modern toolkit already exploits. In any case, at this
moment we still need the api because of the differences between
backends.

Message #32 received at 37770 <at> debbugs.gnu.org (full text, mbox):

From: Robert Pluim <rpluim <at> gmail.com>
To: Carlos Pita <carlosjosepita <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 37770 <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Thu, 24 Oct 2019 11:19:24 +0200

>>>>> On Tue, 22 Oct 2019 15:06:49 -0300, Carlos Pita <carlosjosepita <at> gmail.com> said:

    Carlos> I've improved the code comment for the redisplay interface entry in:
    Carlos> https://github.com/memeplex/emacs/commit/d3c66e6eea8a3b6f1a269bd968597a3bd8a3e811

    Carlos> (to generate the patch, add a .patch suffix to that url)

    Carlos> The new comment states:

    Carlos> /* Return the scale factor for the screen containing frame F. All
    Carlos> geometries are reported by the backend using a scale that is
    Carlos> approximately 96dpi x scale_factor. This scale may match
    Carlos> physical resolution or not. */

Two spaces after '.'. Also: "This scale may not match the physical resolution."

    Carlos> Some thoughts:

    Carlos> One possibility for the (maybe distant) future, is that this scale
    Carlos> factor api won't be needed any more. Like nsterm does (I believe), all
    Carlos> backends might expose a 1 x 96dpi interface so that the upper layers
    Carlos> can work mostly or fully unaware of the device complexities. But at
    Carlos> this moment the xterm backend goes to lengths in order to revert gtk
    Carlos> auto-scaling and provide a "physical dpi" (well, not necessarily
    Carlos> physical, since there is still randr in the middle)  interface to the
    Carlos> upper layers, thus losing the benefits of gtk auto-scaling, although
    Carlos> with good reason since nowadays gtk is more of a hack to the x11
    Carlos> backend, sniffing the underlying x event loop as it is, than a proper
    Carlos> backend on its own.

If emacs used only GTK to draw things to the screen, then indeed there
would be no need for those conversions, as GTK would handle them.

<https://github.com/masm11/emacs> is attempting to implement a 'pure'
GTK backend. I have no idea how close it is to being ready to merge.

    Carlos> Anyway, even if that's not the trend, exposing a higher scale factor
    Carlos> to the upper layers still has the potential benefit of letting those
    Carlos> layers decide how to better use the extra available resolution,
    Carlos> instead of pretending they are drawing to a vintage screen. Whether
    Carlos> this is worthwhile or not I don't know, given that font and image
    Carlos> rendering are the parts most profited from this extra resolution, and
    Carlos> that fact every modern toolkit already exploits. In any case, at this
    Carlos> moment we still need the api because of the differences between
    Carlos> backends.

Youʼre right about that (although on macOS we seem to get by OK
without it).

Robert

Message #37 received at 37770-close <at> debbugs.gnu.org (full text, mbox):

From: Carlos Pita <carlosjosepita <at> gmail.com>
To: Robert Pluim <rpluim <at> gmail.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 37770-close <at> debbugs.gnu.org
Subject: Re: bug#37770: [PATCH] Expose scale factor through the redisplay
 interface
Date: Thu, 24 Oct 2019 11:34:01 -0300

[Message part 1 (text/plain, inline)]

>
>
> If emacs used only GTK to draw things to the screen, then indeed there
> would be no need for those conversions, as GTK would handle them.
>

Exactly.

>
> <https://github.com/masm11/emacs> is attempting to implement a 'pure'
> GTK backend. I have no idea how close it is to being ready to merge.
>

That's great. The way towards Wayland also. Sounds like a LOT of work,
though.

I've been working in an alternative to this patch and the one that fixes
the cairo fringe. My goal is to provide a gtk+cairo solution that listen to
gtk scale changes and scales everything (including fonts) accordingly. This
would enable an adaptive multi-monitor multi-dpi emacs using the old
scale-up-gtk then scale-down-xrandr trick, a nice implementation of which,
including GUI and everything,  is going to be part of the next Ubuntu LTS
with high probability.

I'm having a really hard time making the font engine abandon it's idea of
pixel-sized fonts that is very entrenched in its caching mechanism once
everything was first rendered at some initial dpi. The only way I found to
circumvent this is to report a fixed 96dpi resolution and then scale all
cairo rendering (fonts, images, bitmaps) using the platform scaling factor.
This mostly works but I'm still unable to get rid of all previously opened
fonts, even after clearing all ftcrfont caches, so I get a mix of different
sized fonts. It will probably take me some time to figure out what's
happening.

So I'm closing this issue and will open a new one once I have an integral
cairo+gtk solution working in a way that mostly resembles the nsterm
backend.

Best regards
--
Carlos



>

[Message part 2 (text/html, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.