mate-desktop / caja

Caja, the file manager for the MATE desktop
https://mate-desktop.org/
Other
265 stars 143 forks source link

always use same methods to compute desktop area #1730

Closed lukefromdc closed 1 year ago

lukefromdc commented 1 year ago

Use the older gdk_x11 functions in x11 Use the monitor dimensions in wayland for consistancy

lukefromdc commented 1 year ago

Split out from https://github.com/mate-desktop/caja/pull/1729 which uses the same functions to set the size of the usable icon area on the desktop

lukefromdc commented 1 year ago

Strangely, I got a build failure on translations with this on the laptop unless --disable-nls was passed(Ithen the build finished as expected), maybe something specific to that machine. Debian travis build finished, Federa errored out in make distcheck

raveit65 commented 1 year ago

Distcheck did create the tarball, so it is fine. It's a problem with deployment of gh-pages. https://app.travis-ci.com/github/mate-desktop/caja/jobs/605290626#L5040 Failed to push the build to github.com/mate-desktop/caja.git:gh-pages I guess it's a connection problem between travis-CI and github servers. Starting the build again.

raveit65 commented 1 year ago

CI result is good now.

lukefromdc commented 1 year ago

Anyone else want to review this?

raveit65 commented 1 year ago

As far i know we using scale only because of using Width/HeightOfScreen which is a xlib macros and can't be used with wayland. Maybe it is possible to use GDK equivalents because GDK knows about the scaling factor? At one place you're replacing screen_width = gdk_screen_get_width (screen) with screen_width = WidthOfScreen (gdk_x11_screen_get_xscreen (screen)) / scale; And you added the scale variable. I am thinking it might go the other way around and trying to use the same code for X11 and wayland for getting the screen dimensions. But maybe this isn't possible.

lukefromdc commented 1 year ago

I can run a test build using the wayland code in all places, I will lhave to examine it closely for issues with navigation windows called with --geometry oversize and with the desktop edges

L-U-T-i commented 1 year ago

@raveit65

I am thinking it might go the other way around and trying to use the same code for X11 and wayland for getting the screen dimensions. But maybe this isn't possible.

I've indicated in #1728 that it seems it could be enough just to not deduct margins (container->details->top_margin and container->details->bottom_margin in my case) from container_height for X11 (if it has to be done for wayland at all - or, it was just the remaining from the old code, and is already done elsewhere for the new code...), keeping all of the new (wayland) code. With just that patch (and without both #1729 and #1730) everything seems to work just fine for me in X11.

Maybe this way it would be much more simple to test and to maintain the code in the future?

lukefromdc commented 1 year ago

If we can use the same codepath in x11 and wayland, it certainly simplifies testing

lukefromdc commented 1 year ago

OK, I will close this

On 7/6/2023 at 3:39 PM, "raveit65" @.***> wrote:

@raveit65 commented on this pull request.

  • screen_width = gdk_screen_get_width (screen);
  • screen_height = gdk_screen_get_height (screen);
  • display = gdk_screen_get_display (screen);
  • if (GDK_IS_X11_DISPLAY (display))
  • {
  • scale = gtk_widget_get_scale_factor (GTK_WIDGET (window));
  • screen_width = WidthOfScreen (gdk_x11_screen_get_xscreen (screen)) / scale;
  • screen_height = HeightOfScreen (gdk_x11_screen_get_xscreen (screen)) / scale;
  • }
  • else
  • {
  • GdkRectangle geometry = {0};
  • GdkMonitor *monitor;
  • monitor = gdk_display_get_monitor (display, 0);
  • gdk_monitor_get_geometry(monitor, &geometry);
  • screen_width = geometry.width;
  • screen_height = geometry.height;
  • }

Before this change we have 2 lines of (well working) code. I do not get why this should blow up, only because of an idea to use at all places the same old xlib macros. Do we have crashes in x11 at this place? Does anyone have a stacktrace which point to this 2 lines? I recall in 2019 when all this xlib macros were added by a deprecation fix PR, that we get warned by a gnome dev not to do this, because it is only usable for x11. I won't review it because i am thinking the idea of this PR is wrong. Anyway, i don't block it when other devs from @mate-desktop/core- team think this is an excellent idea.

-- Reply to this email directly or view it on GitHub: https://github.com/mate-desktop/caja/pull/1730#pullrequestreview- 1517190279 You are receiving this because you authored the thread.

Message ID: <mate- @.***>

lukefromdc commented 1 year ago

Going forward, the general case gdk code is likely to be better maintained