Re: [PATCH 3/9] gtk/ui: Introduce helper gd_update_scale

[Weifeng Liu <weifeng.liu.z@gmail.com>]

On Tue, 2025-05-13 at 20:01 +0000, Kim, Dongwon wrote:
> > Hi Dongwon,
> 
> > On Tue, 2025-05-13 at 01:26 +0000, Kim, Dongwon wrote:
> > > Hi,
> > > 
> > > > Subject: [PATCH 3/9] gtk/ui: Introduce helper gd_update_scale
> > > > 
> > > > The code snippet updating scale_x/scale_y is general and will be
> > > > used in next
> > > > patch. Make it a function.
> > > > 
> > > > Signed-off-by: Weifeng Liu <weifeng.liu.z@gmail.com>
> > > > ---
> > > >  include/ui/gtk.h |  2 ++
> > > >  ui/gtk.c         | 30 +++++++++++++++++++-----------
> > > >  2 files changed, 21 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/include/ui/gtk.h b/include/ui/gtk.h index
> > > > aa3d637029..d3944046db 100644
> > > > --- a/include/ui/gtk.h
> > > > +++ b/include/ui/gtk.h
> > > > @@ -224,4 +224,6 @@ int gd_gl_area_make_current(DisplayGLCtx *dgc,
> > > >  /* gtk-clipboard.c */
> > > >  void gd_clipboard_init(GtkDisplayState *gd);
> > > > 
> > > > +void gd_update_scale(VirtualConsole *vc, int ww, int wh, int fbw,
> > > > int
> > > > +fbh);
> > > > +
> > > >  #endif /* UI_GTK_H */
> > > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > > index 8f5bb4b62e..47af49e387 100644
> > > > --- a/ui/gtk.c
> > > > +++ b/ui/gtk.c
> > > > @@ -801,6 +801,24 @@ void
> > > > gd_update_monitor_refresh_rate(VirtualConsole *vc, GtkWidget
> > > > *widget)
> > > > #endif  }
> > > > 
> > > > +void gd_update_scale(VirtualConsole *vc, int ww, int wh, int fbw,
> > > > int
> > > > +fbh) {
> > > > +    if (!vc) {
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    if (vc->s->full_screen) {
> > > > +        vc->gfx.scale_x = (double)ww / fbw;
> > > > +        vc->gfx.scale_y = (double)wh / fbh;
> > > > +    } else if (vc->s->free_scale) {
> > > > +        double sx, sy;
> > > > +
> > > > +        sx = (double)ww / fbw;
> > > > +        sy = (double)wh / fbh;
> > > > +
> > > > +        vc->gfx.scale_x = vc->gfx.scale_y = MIN(sx, sy);
> > > 
> > > I assume you are trying to keep the w/h ratio same here in case free-
> > > scale == true.
> > > Why would we do that? We can easily stretch the host window to any
> > > direction then the scale-x and scale-y
> > > could be different any time.
> > > 
> > Currently, the code doesn't clarify how we should handle aspect ratios.
> > However, I noticed that in the gd_draw_event function, when free-
> > scale=true, it preserves a fixed aspect ratio. I believe this is a
> > reasonable approach (in my humble opinion, it's unlikely that people
> > want to see distorted images), so I've decided to retain this behavior
> > for now and align other parts to follow the same logic, ensuring a
> > consistent experience for users.
> 
> Yeah, I didn't realize it does keep the ratio in case of gl=off in the original code.
> The reason I brought up this is because as you may Have noticed, scale_x and scale_y
> have been individually configured based on width and height of the current window
> and guest surface in gtk-egl and gtk-gl-area, which is being changed with your patches.
> I am not sure which one is the best either but this would definitely require some discussion.
> 

IMO, neither preserving nor ignoring the aspect ratio is objectively
“better” — both have its own supporters. I suggest we start by defining
a reasonable default and making all implementations conform to it (even
if it means changing the behaviors of a few backends). Once that’s in
place, I’ll add an option like "keep-aspect" or similar (per my
discussion with Balaton in the other thread) so users can select the
behavior they prefer. Does that sound acceptable?

> > Best regards,
> > Weifeng
> 
> > > > +    }
> > > > +}
> > > >  /**
> > > >   * DOC: Coordinate handling.
> > > >   *
> > > > @@ -908,17 +926,7 @@ static gboolean gd_draw_event(GtkWidget
> > > > *widget,
> > > > cairo_t *cr, void *opaque)
> > > >      ww_widget =
> > > > gdk_window_get_width(gtk_widget_get_window(widget));
> > > >      wh_widget =
> > > > gdk_window_get_height(gtk_widget_get_window(widget));
> > > > 
> > > > -    if (s->full_screen) {
> > > > -        vc->gfx.scale_x = (double)ww_widget / fbw;
> > > > -        vc->gfx.scale_y = (double)wh_widget / fbh;
> > > > -    } else if (s->free_scale) {
> > > > -        double sx, sy;
> > > > -
> > > > -        sx = (double)ww_widget / fbw;
> > > > -        sy = (double)wh_widget / fbh;
> > > > -
> > > > -        vc->gfx.scale_x = vc->gfx.scale_y = MIN(sx, sy);
> > > > -    }
> > > > +    gd_update_scale(vc, ww_widget, wh_widget, fbw, fbh);
> > > > 
> > > >      ww_surface = fbw * vc->gfx.scale_x;
> > > >      wh_surface = fbh * vc->gfx.scale_y;
> > > > --
> > > > 2.49.0
> > > >