GNOME Bugzilla – Bug 731792
Fix the current desktop preview for HiDpi displays
Last modified: 2021-06-09 16:34:25 UTC
Apart from the issue mentioned in bug 731738 , we should generate higher resolution images and consider the scale factor in our calculations when doing so.
Created attachment 278613 [details] [review] background: Remove unnecessary calculations
Created attachment 278614 [details] [review] background: Fix the current desktop preview for HiDpi displays
Created attachment 278615 [details] Screenshot of bug Notice how the wallpaper is overlapping half the height of the top panel, which has itself lost half its width.
Review of attachment 278613 [details] [review]: Because you want to capture the primary screen, not screen 0.
Review of attachment 278614 [details] [review]: This will need testing with a dual screen setup, with the primary screen being the second one from the left. ::: panels/background/cc-background-panel.c @@ +227,3 @@ + scale_factor = gtk_widget_get_scale_factor (widget); + preview_width *= 2; Huh, * scale_factor.
Created attachment 278664 [details] [review] background: Fix the current desktop preview for HiDpi displays
(In reply to comment #4) > Review of attachment 278613 [details] [review]: > > Because you want to capture the primary screen, not screen 0. Umm... how is that relevant? :) This is not about capturing the primary monitor. We have already done that. It is about trying to remove the work area from the screenshot of the primary monitor. As mentioned in the commit message, capture_rect's (x, y) is always the same as monitor_rect's (x, y), and I can't think of a logical reason for it not to be. What can be different is the height component of capture_rect and monitor_rect.
(In reply to comment #5) > Review of attachment 278614 [details] [review]: > > This will need testing with a dual screen setup, with the primary screen being > the second one from the left. That works. Irrespective of these patches, the code is broken when one of the monitors is rotated or if the secondary is above the primary. I look into those in a separate bug. > ::: panels/background/cc-background-panel.c > @@ +227,3 @@ > > + scale_factor = gtk_widget_get_scale_factor (widget); > + preview_width *= 2; > > Huh, * scale_factor. Oops, sorry about that. Fixed.
(In reply to comment #8) > (In reply to comment #5) > > Review of attachment 278614 [details] [review] [details]: > > > > This will need testing with a dual screen setup, with the primary screen being > > the second one from the left. > > That works. > > Irrespective of these patches, the code is broken when one of the monitors is > rotated or if the secondary is above the primary. I look into those in a > separate bug. Forgot to mention that I tested the multi-monitor setup on LoDpi. I have to find a connector to hook this Pixel up to my monitor.
(In reply to comment #8) > (In reply to comment #5) > > Review of attachment 278614 [details] [review] [details]: > > > > This will need testing with a dual screen setup, with the primary screen being > > the second one from the left. > > That works. > > Irrespective of these patches, the code is broken when one of the monitors is > rotated or if the secondary is above the primary. I look into those in a > separate bug. That's actually something that works right now and that you're regressing on.(In reply to comment #7) > (In reply to comment #4) > > Review of attachment 278613 [details] [review] [details]: > > > > Because you want to capture the primary screen, not screen 0. > > Umm... how is that relevant? :) > > This is not about capturing the primary monitor. We have already done that. It > is about trying to remove the work area from the screenshot of the primary > monitor. > > As mentioned in the commit message, capture_rect's (x, y) is always the same as > monitor_rect's (x, y), and I can't think of a logical reason for it not to be. > What can be different is the height component of capture_rect and monitor_rect. The capture_rect won't be at the same if there's a panel bar at the bottom (as in classic) or a dock on either side of the screen (see calculate_contiguous_workarea())
> > > This will need testing with a dual screen setup, with the primary screen being > > > the second one from the left. > > > > That works. > > > > Irrespective of these patches, the code is broken when one of the monitors is > > rotated or if the secondary is above the primary. I look into those in a > > separate bug. > > That's actually something that works right now and that you're regressing > on. What makes you say so? As someone who uses an L-shaped setup everyday, I can see that it does not work right now for these setups. > The capture_rect won't be at the same if there's a panel bar at the bottom (as > in classic) or a dock on either side of the screen Ok, and how does that affect the x and y of capture_rect? > (see > calculate_contiguous_workarea()) How is the x and y of capture_rect going to be different from monitor_rect's in this code: if (data->workarea_rect.x != data->monitor_rect.x) return FALSE; if ((data->workarea_rect.y + data->workarea_rect.height) != (data->monitor_rect.y + data->monitor_rect.height)) return FALSE; data->capture_rect.x = data->monitor_rect.x; data->capture_rect.width = data->monitor_rect.width; data->capture_rect.y = data->monitor_rect.y; data->capture_rect.height = data->monitor_rect.height - data->workarea_rect.height;
Created attachment 278672 [details] [review] background: Explain why the optimization might be skipped on HiDpi I don't know if it is worth trying to be clever and second guess the rounding error. It does not look too slow without the optimization.
Review of attachment 278613 [details] [review]: > I can't think of a reason why we would place the screenshot anywhere > other than (0, 0). Moreover, capture_rect.[xy] is always the same as > monitor_rect.[xy]. This needs a better explanation. Explain what values are assigned to each of the 2 sets of coordinates, and in which functions.
Review of attachment 278664 [details] [review]: ::: panels/background/cc-background-panel.c @@ +284,3 @@ + + widget = WID ("background-desktop-drawingarea"); + scale_factor = gtk_widget_get_scale_factor (widget); That seems wrong, at least for Wayland which can have scale factors be different values on different monitors. So if the Settings window is on a different screen from the primary screen (on which the top bar is) then the scale factors would be different. The gnome-shell screenshot API should work in absolute pixels, and we shouldn't need to scale anything other than the default thumbnail size (which you're doing above). Or did I misread this code?
Created attachment 278706 [details] [review] background: Remove unnecessary calculations Improve the commit message.
Created attachment 278709 [details] [review] background: Fix the current desktop preview for HiDpi displays
(In reply to comment #14) > Review of attachment 278664 [details] [review]: > > ::: panels/background/cc-background-panel.c > @@ +284,3 @@ > + > + widget = WID ("background-desktop-drawingarea"); > + scale_factor = gtk_widget_get_scale_factor (widget); > > That seems wrong, at least for Wayland which can have scale factors be > different values on different monitors. So if the Settings window is on a > different screen from the primary screen (on which the top bar is) then the > scale factors would be different. Good point. I have fixed this now using gdk_screen_get_monitor_scale_factor with the primary monitor. I have not tested it yet on a actual multi-monitor setup because I don't have the cables at the moment. > The gnome-shell screenshot API should work in absolute pixels, and we shouldn't > need to scale anything other than the default thumbnail size (which you're > doing above). There are a couple of things here: 1. In on_screenshot_finished we have a pixbuf that is a screenshot of the primary monitor. We operate on it with cairo to remove the workarea. Since we are dealing with a GdkPixbuf which is unaware of the scale factor and has raw pixels, we can not use monitor_rect and workarea_rect on it because those values came from GDK and have the unscaled values. So we have to take care of the scale factor of the primary monitor ourselves. 2. Then there is the issue of what values the gnome-shell screenshot API takes. Although related, I don't think this affects (1) because we need the scaled values there anyway. In bug 731738 I changed gnome-shell to take unscaled values. I think that it is better than asking every user (eg., gnome-screenshot) of this API to deal with scaling the GDK values themselves. As you pointed out, this scaling can be tricky on LoDpi + HiDpi setups.
(In reply to comment #18) <snip> > There are a couple of things here: > > 1. In on_screenshot_finished we have a pixbuf that is a screenshot of the > primary monitor. We operate on it with cairo to remove the workarea. Since we > are dealing with a GdkPixbuf which is unaware of the scale factor and has raw > pixels, we can not use monitor_rect and workarea_rect on it because those > values came from GDK and have the unscaled values. So we have to take care of > the scale factor of the primary monitor ourselves. The problem isn't that GDK doesn't scale those values. That's not what it should be doing. The problem is that gnome-shell doesn't export the right values (it exports the unscaled values instead of the scaled ones). > 2. Then there is the issue of what values the gnome-shell screenshot API takes. > Although related, I don't think this affects (1) because we need the scaled > values there anyway. In bug 731738 I changed gnome-shell to take unscaled > values. I think that it is better than asking every user (eg., > gnome-screenshot) of this API to deal with scaling the GDK values themselves. > As you pointed out, this scaling can be tricky on LoDpi + HiDpi setups. GDK's workarea is in pixels, everything should be in pixels. Whether or not there is a scaling factor, if the top bar take 30 pixels instead of 15, we should export 30 pixels as being its height. How gnome-shell scales text and widgets is irrelevant here.
(In reply to comment #19) > GDK's workarea is in pixels GDK's workarea is not in pixels.
(In reply to comment #19) > (In reply to comment #18) > <snip> > > There are a couple of things here: > > > > 1. In on_screenshot_finished we have a pixbuf that is a screenshot of the > > primary monitor. We operate on it with cairo to remove the workarea. Since we > > are dealing with a GdkPixbuf which is unaware of the scale factor and has raw > > pixels, we can not use monitor_rect and workarea_rect on it because those > > values came from GDK and have the unscaled values. So we have to take care of > > the scale factor of the primary monitor ourselves. > > The problem isn't that GDK doesn't scale those values. That's not what it > should be doing. The problem is that gnome-shell doesn't export the right > values (it exports the unscaled values instead of the scaled ones). Sorry, this makes no sense to me. Why would gnome-shell export anything? It gives us a screenshot as a GdkPixbuf, and we have some values obtained from GDK which are in app pixels, not device pixels. If we are to operate on this GdkPixbuf to extract the top panel then we have to work in device pixels because GdkPixbuf has no clue about the scaling factor. So somehow we have to convert the app pixel values from GDK to device pixel values.
(In reply to comment #21) > (In reply to comment #19) > > (In reply to comment #18) > > <snip> > > > There are a couple of things here: > > > > > > 1. In on_screenshot_finished we have a pixbuf that is a screenshot of the > > > primary monitor. We operate on it with cairo to remove the workarea. Since we > > > are dealing with a GdkPixbuf which is unaware of the scale factor and has raw > > > pixels, we can not use monitor_rect and workarea_rect on it because those > > > values came from GDK and have the unscaled values. So we have to take care of > > > the scale factor of the primary monitor ourselves. > > > > The problem isn't that GDK doesn't scale those values. That's not what it > > should be doing. The problem is that gnome-shell doesn't export the right > > values (it exports the unscaled values instead of the scaled ones). > > Sorry, this makes no sense to me. Why would gnome-shell export anything? Because EWMH's "work-area" hint is exported by the window manager. GDK picks it up from the window manager: http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#idm140130317674416 The values exported by gnome-shell are incorrect. > It > gives us a screenshot as a GdkPixbuf, and we have some values obtained from GDK > which are in app pixels, not device pixels. It's not in app pixels, it's in "gnome-shell" pixels. If the app isn't scaled by gnome-shell is, then the value will be incorrect. It kind of works because your application happens to have the same scale factor as gnome-shell. You can verify this by setting the GDK scaling factor to a value that's different from gnome-shell's value, the workarea values come from the window manager and aren't scaled by the scale factor. > If we are to operate on this GdkPixbuf to extract the top panel then we have to > work in device pixels because GdkPixbuf has no clue about the scaling factor. > So somehow we have to convert the app pixel values from GDK to device pixel > values.
(In reply to comment #22) > Because EWMH's "work-area" hint is exported by the window manager. GDK picks it > up from the window manager: > http://standards.freedesktop.org/wm-spec/wm-spec-1.3.html#idm140130317674416 > > The values exported by gnome-shell are incorrect. See gdk_x11_screen_get_monitor_geometry: https://git.gnome.org/browse/gtk+/tree/gdk/x11/gdkscreen-x11.c#n251 commit 525e5cff04f66b87314e91ef408525f02923c14b Author: Alexander Larsson <alexl@redhat.com> Date: Thu Jun 20 11:40:07 2013 +0200 x11: Initial cut at supporting window scaling for X11 If you set GDK_SCALE=2 in the environment then all windows will be scaled by 2. Its not an ideal solution as it doesn't handle multi-monitors at different scales, and only affects gtk apps. But it is a good starting points and will help a lot on HiDPI laptops.
None of the three patches could be applied against master.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new bug report at https://gitlab.gnome.org/GNOME/gnome-control-center/-/issues/ Thank you for your understanding and your help.