GNOME Bugzilla – Bug 781534
xsettings: Calculate window scale from configuration state
Last modified: 2017-07-14 13:05:35 UTC
This patch is needed to have gsd-xsettings respect the configured scale. What the patch does is, when the experimental API is enabled (a state exposed via a property on the DisplayConfig D-Bus node), instead of gsd-xsettings doing its own hidpi calculation, it looks up the primary logical monitor and assumes the scale of that; unless the layout mode is "logical" in which case it assumes the scale is always 1. It's all done using synchronous D-Bus calls, because it seems to be common way in here, but it could just as well be changed to asynchronous if that is preferred. This means that when the "scale-monitor-framebuffer" experimental feature is enabled in mutter, GTK+-on-X11 clients will *not* be drawn in high resolution, as if they did, they would be oversized. When that feature is not enabled, scaling worked as before, except for the fact that the configuration is respected.
Created attachment 350126 [details] [review] xsettings: Calculate window scale from configuration state If there is a configuration state, i.e. a valid state as returned by org.gnome.Mutter.DisplayConfig.GetCurrentState(), use that state to derive a window scale. The way this is done depends on the active layout mode used by the compositor. If the active layout mode is 'logical', the window scaling factor 1 is always used. This will avoid GTK+ X11 clients from being doubly upscaled. If the active layout mode is 'physical', the window scaling is determined by taking the scale of the primary logical monitor.
Review of attachment 350126 [details] [review]: With those comments addressed, looks fine ::: plugins/xsettings/gsd-xsettings-manager.c @@ +633,3 @@ + is_experimental_api_enabled = g_variant_get_boolean (property_value_variant); + + g_variant_unref (property_variant); you also need to unref property_value_variant @@ +635,3 @@ + g_variant_unref (property_variant); + + return is_experimental_api_enabled; we could cache this to avoid the IPC call for something that doesn't change but not really needed for something that runs so seldomly @@ +704,3 @@ + if (!current_state) { + g_warning ("Failed to get current display configuration state: %s", + error->message); need to free the error here @@ +742,3 @@ +out: + g_variant_iter_free (properties); + g_variant_iter_free (logical_monitors); need to free current_state too
Review of attachment 350126 [details] [review]: This looks seriously overcomplicated to look up one or two variables on a D-Bus interface. And the hard-coding of bits of the new D-Bus interface in the sources makes me wonder why we'd want to obsolete the gnome-rr code in gnome-desktop. It was made to avoid (or at least segregate in one place) just this sort of code.
(In reply to Bastien Nocera from comment #3) > Review of attachment 350126 [details] [review] [review]: > > This looks seriously overcomplicated to look up one or two variables on a > D-Bus interface. And the hard-coding of bits of the new D-Bus interface in > the sources makes me wonder why we'd want to obsolete the gnome-rr code in > gnome-desktop. It was made to avoid (or at least segregate in one place) > just this sort of code. gnome-rr in gnome desktop doesn't have anything to represent what this patch uses, and would need to add new API for everything, while at the same time making most if not all of the old API broken; if anything we should add a completely new API, replacing gnome-rr. When dealing with outputs, crtcs etc I can see the benefit, but I don't think it is needed to add another external API for ~150 loc.
(In reply to Jonas Ådahl from comment #4) > gnome-rr in gnome desktop doesn't have anything to represent what this patch > uses, and would need to add new API for everything, while at the same time > making most if not all of the old API broken; if anything we should add a > completely new API, replacing gnome-rr. When dealing with outputs, crtcs etc > I can see the benefit, but I don't think it is needed to add another > external API for ~150 loc. I agree, this is verbose only because checking properties in dbus interfaces using GDBus is verbose. Do you have an amended version of this that addresses comment #2 ?
Created attachment 352634 [details] [review] xsettings: Calculate window scale from configuration state If there is a configuration state, i.e. a valid state as returned by org.gnome.Mutter.DisplayConfig.GetCurrentState(), use that state to derive a window scale. The way this is done depends on the active layout mode used by the compositor. If the active layout mode is 'logical', the window scaling factor 1 is always used. This will avoid GTK+ X11 clients from being doubly upscaled. If the active layout mode is 'physical', the window scaling is determined by taking the scale of the primary logical monitor.
Review of attachment 352634 [details] [review]: looks good ::: plugins/xsettings/gsd-xsettings-manager.c @@ +735,3 @@ + + if (is_primary) { + scale = (int) logical_monitor_scale; so, in physical layout mode, scales are never going to be fractional, right?
(In reply to Rui Matos from comment #7) > Review of attachment 352634 [details] [review] [review]: > > looks good > > ::: plugins/xsettings/gsd-xsettings-manager.c > @@ +735,3 @@ > + > + if (is_primary) { > + scale = (int) logical_monitor_scale; > > so, in physical layout mode, scales are never going to be fractional, right? I doubt it, but even if so, in Xsettings terms, it must be rounded in some direction, as we can't describe the scaling factor as a float.
(In reply to Jonas Ådahl from comment #8) > > so, in physical layout mode, scales are never going to be fractional, right? > > I doubt it, but even if so, in Xsettings terms, it must be rounded in some > direction, as we can't describe the scaling factor as a float. Right, I was just wondering if we might want to do some kind of rounding other than truncation.
(In reply to Rui Matos from comment #9) > (In reply to Jonas Ådahl from comment #8) > > > so, in physical layout mode, scales are never going to be fractional, right? > > > > I doubt it, but even if so, in Xsettings terms, it must be rounded in some > > direction, as we can't describe the scaling factor as a float. > > Right, I was just wondering if we might want to do some kind of rounding > other than truncation. We could potentially first truncate, and be clever about font sizes, but it's not something I've looked into further.
Attachment 352634 [details] pushed as 2f553ba - xsettings: Calculate window scale from configuration state