bug#37770: [PATCH] Expose scale factor through the redisplay interface

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

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

[-- Attachment #2: 0001-Expose-scale-factor-through-the-redisplay-interface.patch --]
[-- Type: text/x-patch, Size: 7080 bytes --]

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

* 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.
---
 src/dispextern.h |  3 +++
 src/nsterm.m     |  1 +
 src/w32term.c    | 29 +++++++++++------------------
 src/xterm.c      | 41 ++++++++++++++++++-----------------------
 4 files changed, 33 insertions(+), 41 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 0615b16..ec3c555 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+  /* Return the scale factor for the screen containing frame F.  */
+  unsigned (*get_scale_factor) (struct frame *f);
+
   /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
   void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
                               struct draw_fringe_bitmap_params *p);
diff --git a/src/nsterm.m b/src/nsterm.m
index 5583c61..8dddf1e 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -5087,6 +5087,7 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  0, /* get_scale_factor */
   ns_draw_fringe_bitmap,
   0, /* define_fringe_bitmap */ /* FIXME: simplify ns_draw_fringe_bitmap */
   0, /* destroy_fringe_bitmap */
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..c898f17 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -304,20 +304,16 @@ w32_restore_glyph_string_clip (struct glyph_string *s)
     }
 }
 
-static void
-w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_y)
+static unsigned
+w32_get_scale_factor(struct frame *f)
 {
+  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   const int base_res = 96;
 
-  *scale_x = *scale_y = 1;
-
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
 }
 
 /*
@@ -334,12 +330,8 @@ w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_
 static void
 w32_draw_underwave (struct glyph_string *s, COLORREF color)
 {
-  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
-
-  int scale_x, scale_y;
-  w32_get_scale_factor (dpyinfo, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x, thickness = scale_y;
+  int scale = w32_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
   int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
   Emacs_Rectangle wave_clip, string_clip, final_clip;
   RECT w32_final_clip, w32_string_clip;
@@ -348,7 +340,7 @@ w32_draw_underwave (struct glyph_string *s, COLORREF color)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -7192,6 +7184,7 @@ w32_make_rdb (char *xrm_option)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  w32_get_scale_factor,
   w32_draw_fringe_bitmap,
   w32_define_fringe_bitmap,
   w32_destroy_fringe_bitmap,
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..6000860 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3611,21 +3611,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
-static void
-x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
+static unsigned
+x_get_scale_factor(struct frame *f)
 {
+#ifdef USE_GTK
+  return xg_get_scale (f);
+#else
+  Display *disp = FRAME_X_DISPLAY (f);
+  struct x_display_info *dpyinfo = x_display_info_for_display (disp);
   const int base_res = 96;
-  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
-
-  *scale_x = *scale_y = 1;
 
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
+#endif /* USE_GTK */
 }
 
 /*
@@ -3641,27 +3641,21 @@ x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  Display *display = FRAME_X_DISPLAY (s->f);
-
   /* Adjust for scale/HiDPI.  */
-  int scale_x, scale_y;
-
-  x_get_scale_factor (display, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x;
-
+  int scale = x_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * 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);
 #else  /* not USE_CAIRO */
-  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale_y;;
+  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale;
   bool odd;
   XRectangle wave_clip, string_clip, final_clip;
 
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -10557,7 +10551,7 @@ x_set_offset (struct frame *f, register int xoff, register int yoff, int change_
 {
   int modified_top, modified_left;
 #ifdef USE_GTK
-  int scale = xg_get_scale (f);
+  int scale = x_get_scale_factor (f);
 #endif
 
   if (change_gravity > 0)
@@ -13356,6 +13350,7 @@ x_activate_timeout_atimer (void)
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
     gui_fix_overlapping_area,
+    x_get_scale_factor,
     x_draw_fringe_bitmap,
 #ifdef USE_CAIRO
     x_cr_define_fringe_bitmap,
-- 
2.20.1

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

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.

[-- Attachment #2: 0001-Expose-scale-factor-through-the-redisplay-interface.patch --]
[-- Type: text/x-patch, Size: 7619 bytes --]

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

* 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 a dummy implementation that always return 1 to the rif,
    since there are no uses of any scale factor here.

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.

Note 2: I decided to let get_scale_factor return a double, even tough
factors currently in use are all integers AFAIK. This is in
anticipation of fractional scaling. I believe it's prudent to keep
the interface general in this regard.
---
 src/dispextern.h |  3 +++
 src/nsterm.m     |  6 ++++++
 src/w32term.c    | 29 +++++++++++------------------
 src/xterm.c      | 41 ++++++++++++++++++-----------------------
 4 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 0615b16..b93e25f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+  /* Return the scale factor for the screen containing frame F.  */
+  double (*get_scale_factor) (struct frame *f);
+
   /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
   void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
                               struct draw_fringe_bitmap_params *p);
diff --git a/src/nsterm.m b/src/nsterm.m
index 5583c61..6e1b751 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2957,6 +2957,11 @@ so some key presses (TAB) are swallowed by the system.  */
 
    ========================================================================== */
 
+static double
+ns_get_scale_factor (struct frame *f)
+{
+  return 1;  // TODO do we need to do something else here?
+}
 
 extern int max_used_fringe_bitmap;
 static void
@@ -5087,6 +5092,7 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  ns_get_scale_factor,
   ns_draw_fringe_bitmap,
   0, /* define_fringe_bitmap */ /* FIXME: simplify ns_draw_fringe_bitmap */
   0, /* destroy_fringe_bitmap */
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..6d430c6 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -304,20 +304,16 @@ w32_restore_glyph_string_clip (struct glyph_string *s)
     }
 }
 
-static void
-w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_y)
+static double
+w32_get_scale_factor(struct frame *f)
 {
+  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   const int base_res = 96;
 
-  *scale_x = *scale_y = 1;
-
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
 }
 
 /*
@@ -334,12 +330,8 @@ w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_
 static void
 w32_draw_underwave (struct glyph_string *s, COLORREF color)
 {
-  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
-
-  int scale_x, scale_y;
-  w32_get_scale_factor (dpyinfo, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x, thickness = scale_y;
+  double scale = w32_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
   int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
   Emacs_Rectangle wave_clip, string_clip, final_clip;
   RECT w32_final_clip, w32_string_clip;
@@ -348,7 +340,7 @@ w32_draw_underwave (struct glyph_string *s, COLORREF color)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -7192,6 +7184,7 @@ w32_make_rdb (char *xrm_option)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  w32_get_scale_factor,
   w32_draw_fringe_bitmap,
   w32_define_fringe_bitmap,
   w32_destroy_fringe_bitmap,
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..67253a6 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3611,21 +3611,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
-static void
-x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
+static double
+x_get_scale_factor(struct frame *f)
 {
+#ifdef USE_GTK
+  return xg_get_scale (f);
+#else
+  Display *disp = FRAME_X_DISPLAY (f);
+  struct x_display_info *dpyinfo = x_display_info_for_display (disp);
   const int base_res = 96;
-  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
-
-  *scale_x = *scale_y = 1;
 
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
+#endif /* USE_GTK */
 }
 
 /*
@@ -3641,27 +3641,21 @@ x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  Display *display = FRAME_X_DISPLAY (s->f);
-
   /* Adjust for scale/HiDPI.  */
-  int scale_x, scale_y;
-
-  x_get_scale_factor (display, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x;
-
+  double scale = x_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * 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);
 #else  /* not USE_CAIRO */
-  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale_y;;
+  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale;
   bool odd;
   XRectangle wave_clip, string_clip, final_clip;
 
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -10557,7 +10551,7 @@ x_set_offset (struct frame *f, register int xoff, register int yoff, int change_
 {
   int modified_top, modified_left;
 #ifdef USE_GTK
-  int scale = xg_get_scale (f);
+  double scale = x_get_scale_factor (f);
 #endif
 
   if (change_gravity > 0)
@@ -13356,6 +13350,7 @@ x_activate_timeout_atimer (void)
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
     gui_fix_overlapping_area,
+    x_get_scale_factor,
     x_draw_fringe_bitmap,
 #ifdef USE_CAIRO
     x_cr_define_fringe_bitmap,
-- 
2.20.1

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Robert Pluim]

>>>>> On Tue, 15 Oct 2019 21:32:25 -0300, Carlos Pita <carlosjosepita@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@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

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

> 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

[-- Attachment #2: 0001-Expose-scale-factor-through-the-redisplay-interface.patch --]
[-- Type: text/x-patch, Size: 7599 bytes --]

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

* src/dispextern.h (redisplay_interface): add get_scale_factor API.

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

* src/w32term.c (w32_get_scale_factor): likewise simplify logic and
add to the rif.

* src/nsterm.m (ns_get_scale_factor): add a dummy implementation that
always return 1 to the rif, since there are no uses of any scale
factor here.

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.

Note 2: I decided to let get_scale_factor return a double, even tough
factors currently in use are all integers AFAIK. This is in
anticipation of fractional scaling. I believe it's prudent to keep
the interface general in this regard.
---
 src/dispextern.h |  3 +++
 src/nsterm.m     |  6 ++++++
 src/w32term.c    | 29 +++++++++++------------------
 src/xterm.c      | 41 ++++++++++++++++++-----------------------
 4 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 0615b16..b93e25f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+  /* Return the scale factor for the screen containing frame F.  */
+  double (*get_scale_factor) (struct frame *f);
+
   /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
   void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
                               struct draw_fringe_bitmap_params *p);
diff --git a/src/nsterm.m b/src/nsterm.m
index 5583c61..6e1b751 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2957,6 +2957,11 @@ so some key presses (TAB) are swallowed by the system.  */
 
    ========================================================================== */
 
+static double
+ns_get_scale_factor (struct frame *f)
+{
+  return 1;  // TODO do we need to do something else here?
+}
 
 extern int max_used_fringe_bitmap;
 static void
@@ -5087,6 +5092,7 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  ns_get_scale_factor,
   ns_draw_fringe_bitmap,
   0, /* define_fringe_bitmap */ /* FIXME: simplify ns_draw_fringe_bitmap */
   0, /* destroy_fringe_bitmap */
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..6d430c6 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -304,20 +304,16 @@ w32_restore_glyph_string_clip (struct glyph_string *s)
     }
 }
 
-static void
-w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_y)
+static double
+w32_get_scale_factor(struct frame *f)
 {
+  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   const int base_res = 96;
 
-  *scale_x = *scale_y = 1;
-
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
 }
 
 /*
@@ -334,12 +330,8 @@ w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_
 static void
 w32_draw_underwave (struct glyph_string *s, COLORREF color)
 {
-  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
-
-  int scale_x, scale_y;
-  w32_get_scale_factor (dpyinfo, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x, thickness = scale_y;
+  double scale = w32_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
   int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
   Emacs_Rectangle wave_clip, string_clip, final_clip;
   RECT w32_final_clip, w32_string_clip;
@@ -348,7 +340,7 @@ w32_draw_underwave (struct glyph_string *s, COLORREF color)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -7192,6 +7184,7 @@ w32_make_rdb (char *xrm_option)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  w32_get_scale_factor,
   w32_draw_fringe_bitmap,
   w32_define_fringe_bitmap,
   w32_destroy_fringe_bitmap,
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..67253a6 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3611,21 +3611,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
-static void
-x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
+static double
+x_get_scale_factor(struct frame *f)
 {
+#ifdef USE_GTK
+  return xg_get_scale (f);
+#else
+  Display *disp = FRAME_X_DISPLAY (f);
+  struct x_display_info *dpyinfo = x_display_info_for_display (disp);
   const int base_res = 96;
-  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
-
-  *scale_x = *scale_y = 1;
 
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
+#endif /* USE_GTK */
 }
 
 /*
@@ -3641,27 +3641,21 @@ x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  Display *display = FRAME_X_DISPLAY (s->f);
-
   /* Adjust for scale/HiDPI.  */
-  int scale_x, scale_y;
-
-  x_get_scale_factor (display, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x;
-
+  double scale = x_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * 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);
 #else  /* not USE_CAIRO */
-  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale_y;;
+  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale;
   bool odd;
   XRectangle wave_clip, string_clip, final_clip;
 
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -10557,7 +10551,7 @@ x_set_offset (struct frame *f, register int xoff, register int yoff, int change_
 {
   int modified_top, modified_left;
 #ifdef USE_GTK
-  int scale = xg_get_scale (f);
+  double scale = x_get_scale_factor (f);
 #endif
 
   if (change_gravity > 0)
@@ -13356,6 +13350,7 @@ x_activate_timeout_atimer (void)
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
     gui_fix_overlapping_area,
+    x_get_scale_factor,
     x_draw_fringe_bitmap,
 #ifdef USE_CAIRO
     x_cr_define_fringe_bitmap,
-- 
2.20.1

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

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

[-- Attachment #2: 0001-Expose-scale-factor-through-the-redisplay-interface-.patch --]
[-- Type: text/x-patch, Size: 7612 bytes --]

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

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

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

* src/w32term.c (w32_get_scale_factor): Likewise simplify logic and
add to the rif.

* src/nsterm.m (ns_get_scale_factor): Add a dummy implementation that
always return 1 to the rif, since there are no uses of any scale
factor here.

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.

Note 2: I decided to let get_scale_factor return a double, even tough
factors currently in use are all integers AFAIK. This is in
anticipation of fractional scaling. I believe it's prudent to keep
the interface general in this regard.
---
 src/dispextern.h |  3 +++
 src/nsterm.m     |  6 ++++++
 src/w32term.c    | 29 +++++++++++------------------
 src/xterm.c      | 41 ++++++++++++++++++-----------------------
 4 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 0615b16..b93e25f 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+  /* Return the scale factor for the screen containing frame F.  */
+  double (*get_scale_factor) (struct frame *f);
+
   /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
   void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
                               struct draw_fringe_bitmap_params *p);
diff --git a/src/nsterm.m b/src/nsterm.m
index 5583c61..6e1b751 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2957,6 +2957,11 @@ so some key presses (TAB) are swallowed by the system.  */
 
    ========================================================================== */
 
+static double
+ns_get_scale_factor (struct frame *f)
+{
+  return 1;  // TODO do we need to do something else here?
+}
 
 extern int max_used_fringe_bitmap;
 static void
@@ -5087,6 +5092,7 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  ns_get_scale_factor,
   ns_draw_fringe_bitmap,
   0, /* define_fringe_bitmap */ /* FIXME: simplify ns_draw_fringe_bitmap */
   0, /* destroy_fringe_bitmap */
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845..6d430c6 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -304,20 +304,16 @@ w32_restore_glyph_string_clip (struct glyph_string *s)
     }
 }
 
-static void
-w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_y)
+static double
+w32_get_scale_factor(struct frame *f)
 {
+  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   const int base_res = 96;
 
-  *scale_x = *scale_y = 1;
-
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
 }
 
 /*
@@ -334,12 +330,8 @@ w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_
 static void
 w32_draw_underwave (struct glyph_string *s, COLORREF color)
 {
-  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
-
-  int scale_x, scale_y;
-  w32_get_scale_factor (dpyinfo, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x, thickness = scale_y;
+  double scale = w32_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
   int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
   Emacs_Rectangle wave_clip, string_clip, final_clip;
   RECT w32_final_clip, w32_string_clip;
@@ -348,7 +340,7 @@ w32_draw_underwave (struct glyph_string *s, COLORREF color)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -7192,6 +7184,7 @@ w32_make_rdb (char *xrm_option)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  w32_get_scale_factor,
   w32_draw_fringe_bitmap,
   w32_define_fringe_bitmap,
   w32_destroy_fringe_bitmap,
diff --git a/src/xterm.c b/src/xterm.c
index 5d8b148..67253a6 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3611,21 +3611,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
-static void
-x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
+static double
+x_get_scale_factor(struct frame *f)
 {
+#ifdef USE_GTK
+  return xg_get_scale (f);
+#else
+  Display *disp = FRAME_X_DISPLAY (f);
+  struct x_display_info *dpyinfo = x_display_info_for_display (disp);
   const int base_res = 96;
-  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
-
-  *scale_x = *scale_y = 1;
 
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return floor (dpyinfo->resx / base_res);
+  else
+    return 1;
+#endif /* USE_GTK */
 }
 
 /*
@@ -3641,27 +3641,21 @@ x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  Display *display = FRAME_X_DISPLAY (s->f);
-
   /* Adjust for scale/HiDPI.  */
-  int scale_x, scale_y;
-
-  x_get_scale_factor (display, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x;
-
+  double scale = x_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * 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);
 #else  /* not USE_CAIRO */
-  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale_y;;
+  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale;
   bool odd;
   XRectangle wave_clip, string_clip, final_clip;
 
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -10557,7 +10551,7 @@ x_set_offset (struct frame *f, register int xoff, register int yoff, int change_
 {
   int modified_top, modified_left;
 #ifdef USE_GTK
-  int scale = xg_get_scale (f);
+  double scale = x_get_scale_factor (f);
 #endif
 
   if (change_gravity > 0)
@@ -13356,6 +13350,7 @@ x_activate_timeout_atimer (void)
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
     gui_fix_overlapping_area,
+    x_get_scale_factor,
     x_draw_fringe_bitmap,
 #ifdef USE_CAIRO
     x_cr_define_fringe_bitmap,
-- 
2.20.1

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Robert Pluim]

>>>>> On Wed, 16 Oct 2019 16:08:47 -0300, Carlos Pita <carlosjosepita@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@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

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Eli Zaretskii]

> From: Carlos Pita <carlosjosepita@gmail.com>
> Date: Wed, 16 Oct 2019 16:08:47 -0300
> Cc: 37770@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.

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

> 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

[-- Attachment #2: 0001-Expose-scale-factor-through-the-redisplay-interface-.patch --]
[-- Type: text/x-patch, Size: 7640 bytes --]

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

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.

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

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

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

* src/w32term.c (w32_get_scale_factor): Likewise simplify logic and
add to the rif.

* src/nsterm.m (ns_get_scale_factor): Add a dummy implementation that
always return 1 to the rif, since there are no uses of any scale
factor here.
---
 src/dispextern.h |  3 +++
 src/nsterm.m     |  6 ++++++
 src/w32term.c    | 29 +++++++++++------------------
 src/xterm.c      | 41 ++++++++++++++++++-----------------------
 4 files changed, 38 insertions(+), 41 deletions(-)

diff --git a/src/dispextern.h b/src/dispextern.h
index 0615b16d71..b93e25f216 100644
--- a/src/dispextern.h
+++ b/src/dispextern.h
@@ -2942,6 +2942,9 @@ reset_mouse_highlight (Mouse_HLInfo *hlinfo)
 
 #ifdef HAVE_WINDOW_SYSTEM
 
+  /* Return the scale factor for the screen containing frame F.  */
+  double (*get_scale_factor) (struct frame *f);
+
   /* Draw a fringe bitmap in window W of row ROW using parameters P.  */
   void (*draw_fringe_bitmap) (struct window *w, struct glyph_row *row,
                               struct draw_fringe_bitmap_params *p);
diff --git a/src/nsterm.m b/src/nsterm.m
index 5583c6105c..7b95b9e20b 100644
--- a/src/nsterm.m
+++ b/src/nsterm.m
@@ -2957,6 +2957,11 @@ so some key presses (TAB) are swallowed by the system.  */
 
    ========================================================================== */
 
+static double
+ns_get_scale_factor (struct frame *f)
+{
+  return 1;  /* TODO do we need to do something else here?  */
+}
 
 extern int max_used_fringe_bitmap;
 static void
@@ -5087,6 +5092,7 @@ static Lisp_Object ns_string_to_lispmod (const char *s)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  ns_get_scale_factor,
   ns_draw_fringe_bitmap,
   0, /* define_fringe_bitmap */ /* FIXME: simplify ns_draw_fringe_bitmap */
   0, /* destroy_fringe_bitmap */
diff --git a/src/w32term.c b/src/w32term.c
index 9da0845836..fc2893f254 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -304,20 +304,16 @@ w32_restore_glyph_string_clip (struct glyph_string *s)
     }
 }
 
-static void
-w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_y)
+static double
+w32_get_scale_factor(struct frame *f)
 {
+  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (f);
   const int base_res = 96;
 
-  *scale_x = *scale_y = 1;
-
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return round (dpyinfo->resx / base_res);
+  else
+    return 1;
 }
 
 /*
@@ -334,12 +330,8 @@ w32_get_scale_factor(struct w32_display_info *dpyinfo, int *scale_x, int *scale_
 static void
 w32_draw_underwave (struct glyph_string *s, COLORREF color)
 {
-  struct w32_display_info *dpyinfo = FRAME_DISPLAY_INFO (s->f);
-
-  int scale_x, scale_y;
-  w32_get_scale_factor (dpyinfo, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x, thickness = scale_y;
+  double scale = w32_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * scale, thickness = scale;
   int dx, dy, x0, y0, width, x1, y1, x2, y2, odd, xmax;
   Emacs_Rectangle wave_clip, string_clip, final_clip;
   RECT w32_final_clip, w32_string_clip;
@@ -348,7 +340,7 @@ w32_draw_underwave (struct glyph_string *s, COLORREF color)
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -7192,6 +7184,7 @@ w32_make_rdb (char *xrm_option)
   gui_clear_window_mouse_face,
   gui_get_glyph_overhangs,
   gui_fix_overlapping_area,
+  w32_get_scale_factor,
   w32_draw_fringe_bitmap,
   w32_define_fringe_bitmap,
   w32_destroy_fringe_bitmap,
diff --git a/src/xterm.c b/src/xterm.c
index 045589534f..1a5c339fa7 100644
--- a/src/xterm.c
+++ b/src/xterm.c
@@ -3611,21 +3611,21 @@ x_draw_stretch_glyph_string (struct glyph_string *s)
   s->background_filled_p = true;
 }
 
-static void
-x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
+static double
+x_get_scale_factor(struct frame *f)
 {
+#ifdef USE_GTK
+  return xg_get_scale (f);
+#else
+  Display *disp = FRAME_X_DISPLAY (f);
+  struct x_display_info *dpyinfo = x_display_info_for_display (disp);
   const int base_res = 96;
-  struct x_display_info * dpyinfo = x_display_info_for_display (disp);
-
-  *scale_x = *scale_y = 1;
 
-  if (dpyinfo)
-    {
-      if (dpyinfo->resx > base_res)
-	*scale_x = floor (dpyinfo->resx / base_res);
-      if (dpyinfo->resy > base_res)
-	*scale_y = floor (dpyinfo->resy / base_res);
-    }
+  if (dpyinfo && dpyinfo->resx > base_res)
+    return round (dpyinfo->resx / base_res);
+  else
+    return 1;
+#endif /* USE_GTK */
 }
 
 /*
@@ -3641,27 +3641,21 @@ x_get_scale_factor(Display *disp, int *scale_x, int *scale_y)
 static void
 x_draw_underwave (struct glyph_string *s)
 {
-  Display *display = FRAME_X_DISPLAY (s->f);
-
   /* Adjust for scale/HiDPI.  */
-  int scale_x, scale_y;
-
-  x_get_scale_factor (display, &scale_x, &scale_y);
-
-  int wave_height = 3 * scale_y, wave_length = 2 * scale_x;
-
+  double scale = x_get_scale_factor (s->f);
+  int wave_height = 3 * scale, wave_length = 2 * 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);
 #else  /* not USE_CAIRO */
-  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale_y;;
+  int dx, dy, x0, y0, width, x1, y1, x2, y2, xmax, thickness = scale;
   bool odd;
   XRectangle wave_clip, string_clip, final_clip;
 
   dx = wave_length;
   dy = wave_height - 1;
   x0 = s->x;
-  y0 = s->ybase + wave_height / 2 - scale_y;
+  y0 = s->ybase + wave_height / 2 - scale;
   width = s->width;
   xmax = x0 + width;
 
@@ -10557,7 +10551,7 @@ x_set_offset (struct frame *f, register int xoff, register int yoff, int change_
 {
   int modified_top, modified_left;
 #ifdef USE_GTK
-  int scale = xg_get_scale (f);
+  double scale = x_get_scale_factor (f);
 #endif
 
   if (change_gravity > 0)
@@ -13357,6 +13351,7 @@ x_activate_timeout_atimer (void)
     gui_clear_window_mouse_face,
     gui_get_glyph_overhangs,
     gui_fix_overlapping_area,
+    x_get_scale_factor,
     x_draw_fringe_bitmap,
 #ifdef USE_CAIRO
     x_cr_define_fringe_bitmap,
-- 
2.20.1

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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.

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Robert Pluim]

>>>>> On Tue, 22 Oct 2019 15:06:49 -0300, Carlos Pita <carlosjosepita@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

bug#37770: [PATCH] Expose scale factor through the redisplay interface

[Carlos Pita]

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

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



>

[-- Attachment #2: Type: text/html, Size: 2680 bytes --]