mate-desktop / caja

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

caja-desktop-window: Fix the xrandr error #1724

Closed yangxiaojuan-loongson closed 1 year ago

yangxiaojuan-loongson commented 1 year ago

Fix the xrandr error. Here use a gdk_screen_get_width/height func to get the right changed size just like nautilus-desktop do. Also fix https://github.com/mate-desktop/caja/issues/1096

zhuyaliang commented 1 year ago
  1. Functions that have been deprecated should not be used anymore
  2. Scale factor cannot be ignored

the internal scale factor that maps from window coordiantes to the actual device pixels. On traditional systems this is 1, but on very high density outputs this can be a higher value (often 2).

yangxiaojuan-loongson commented 1 year ago
  1. Functions that have been deprecated should not be used anymore
  2. Scale factor cannot be ignored

the internal scale factor that maps from window coordiantes to the actual device pixels. On traditional systems this is 1, but on very high density outputs this can be a higher value (often 2).

Thank you for your reply. I didn’t consider it comprehensively. I refer to the implementation of nautilus-desktop. I will check more implementations of other file managers. But do you have a better way to fix the problem of xrandr? Thanks

lukefromdc commented 1 year ago

Scaling is not needed on wayland but is on x11. What is the xrandr error referenced anyway?

zhuyaliang commented 1 year ago

@yangxiaojuan-loongson Please submit the xrandr error message or provide a replication method

yangxiaojuan-loongson commented 1 year ago

@yangxiaojuan-loongson Please submit the xrandr error message or provide a replication method

Just use the xrandr -o left can reproduce this problem. The lower half of the screen is blurred and the mouse does not respond

zhuyaliang commented 1 year ago

@yangxiaojuan-loongson I will verify this issue later

lukefromdc commented 1 year ago

I've never once had that happen on changing screen resolution. Possibly driver sensitive?

yangxiaojuan-loongson commented 1 year ago

I've never once had that happen on changing screen resolution. Possibly driver sensitive?

There are problems on debian. I have tested on debian10/11/12 mate desktop. I think there will be problems if the resolution is increased. And restart caja can solve the problem.

lukefromdc commented 1 year ago

Anyway, I cannot duplicate it on Debian unstanbe w locally compiled GTK3

raveit65 commented 1 year ago

xrandr -o left and back with xrandr -o normal breaks definitely the caja desktop window. Also,

[rave@mother ~]$ xrandr --listactivemonitors
Monitors: 2
 0: +*DisplayPort-0 3840/607x2160/345+0+0  DisplayPort-0
 1: +HDMI-A-0 3840/608x2160/345+0+2160  HDMI-A-0
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate left --output HDMI-A-0 --rotate left
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate normal --output HDMI-A-0 --rotate normal
[rave@mother ~]$

with 2 monitors doesn't work fine. But sorry, without the scaling for HIDPI i can't test your PR.

raveit65 commented 1 year ago

I refer to the implementation of nautilus-desktop.

Nautilus desktop was gone years ago with gnome 3 :)

lukefromdc commented 1 year ago

GNOME dropped desktop support from Nautilus well after GNOME 3 first came out, so they had it for a while. To keep it with their wayland transition would have required either using gtk-layer-shell, (which might not have existed or matured yet), or implementing similar features themselves in Mutter.

It's not so difficult now, but when they first dropped the desktop they may have been looking at how hard it was then to make any window not only fullscreen but also sticky and always on the bottom even when focused with just the bare wayland protocol not extended by foreign toplevel support and layer shell.

zhuyaliang commented 1 year ago

@yangxiaojuan-loongson I cannot reproduce the problem using ArchLinux Ubuntu Debian, everything is working well

yangxiaojuan-loongson commented 1 year ago

Anyway, I cannot duplicate it on Debian unstanbe w locally compiled GTK3

Are you using mate desktop and use caja file manager? Try rotation the screen. The problem can be reproduced.

yangxiaojuan-loongson commented 1 year ago

@yangxiaojuan-loongson I cannot reproduce the problem using ArchLinux Ubuntu Debian, everything is working well

Are you using mate desktop and use caja file manager? The problem is exists only with caja file manager. Nautilus or other file manager is ok

yangxiaojuan-loongson commented 1 year ago

xrandr -o left and back with xrandr -o normal breaks definitely the caja desktop window. Also,

[rave@mother ~]$ xrandr --listactivemonitors
Monitors: 2
 0: +*DisplayPort-0 3840/607x2160/345+0+0  DisplayPort-0
 1: +HDMI-A-0 3840/608x2160/345+0+2160  HDMI-A-0
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate left --output HDMI-A-0 --rotate left
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate normal --output HDMI-A-0 --rotate normal
[rave@mother ~]$

with 2 monitors doesn't work fine. But sorry, without the scaling for HIDPI i can't test your PR.

loong

xrandr -o left and back with xrandr -o normal breaks definitely the caja desktop window. Also,

[rave@mother ~]$ xrandr --listactivemonitors
Monitors: 2
 0: +*DisplayPort-0 3840/607x2160/345+0+0  DisplayPort-0
 1: +HDMI-A-0 3840/608x2160/345+0+2160  HDMI-A-0
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate left --output HDMI-A-0 --rotate left
[rave@mother ~]$ xrandr --output DisplayPort-0 --rotate normal --output HDMI-A-0 --rotate normal
[rave@mother ~]$

with 2 monitors doesn't work fine. But sorry, without the scaling for HIDPI i can't test your PR.

Yes, now caja doesn't work fine even with one monitors. Yeah, I did not take into account the influence of the scaler factor. I need to learn more about caja.

lukefromdc commented 1 year ago

I finally found the ONE thing that could trigger this bug on my system: rotating the screen and rotating it back. Note that is is very difficult for me to rotate is back as I am almost unable to use the mouse on a rotated screen not physically turned as well, but xrandr's timeouts can take care of that.

We still have the question of fixing this without adding new deprecations however

lukefromdc commented 1 year ago

On more complete testing, I found this code successfully handles rotate left and back, change screen resolutions and back, change scaling from 2 to 1 and back to 2 again. The older code handles the scale and resolution changes will but leaves part of the desktop unused (and apparently undefined) after rotate left and back.

Yes, we add new deprecations but if this was what GNOME did before dumping the desktop we probably need this too. I tried my own code from the wayland side but ran into scaling issues no matter how I tried to define it as wayfire (my wayland compositor) handles scaling itself and x11 does not.

lukefromdc commented 1 year ago

Apparently gdk_screen_get_width(screen); and gdk_screen_get_height(screen); return the correct dimensions including scaling, while WidthOfScreen (gdk_x11_screen_get_xscreen (screen) and HeightOfScreen (gdk_x11_screen_get_xscreen (screen) do not.

if I cannot find a better solution myself I will merge this, as a deprecation that may bitrot later is to be preferred to a bug affecting users now.

lukefromdc commented 1 year ago

My final set of tests was multimonitor support, three ways of doing this tried. First note that any recent version of caja CAN render the desktop background onto multiple monitors. At least on my test with two dissimilar monitors however, no version of caja I tested was able to render icons onto a secondary monitor, just the background. That sets up the "size-changed" test for adding and removing monitors.

The current git master: works for all cases EXCEPT rotate-and rotate fails ONLY if there is NOT a second monitor connected

Your PR based on old Nautilus: Works for ALL cases.

My Wayland code in x11: works for all cases EXCEPT removing and replacing a monitor. Then we get the unused background with (in compiz) the wierd no-redraw mess. This is expected as that code is on a per-monitor basis, to fix it would require iterating over all monitors, which is much more complex.

The GdkScreen stuff is deprecated only because GdkScreen was removed from GTK4, which we have no plans to support anytime soon as we would have to add a substantial extension library to it. A GdkScreen however can include multiple monitors, so this lets the background be drawn over multiple monitors and follow turning monitors on and off. The latter is all that is affected because this code runs only when the screen dimensions change.

As for multimonitor support in wayland, that may have to be written from scratch and is a problem for another day. Merging this and rebasing my wayland work

raveit65 commented 1 year ago

Why merging this breaks scaling? Please revert! Please do not merge so fast. I didn't approve my review.

raveit65 commented 1 year ago

Also i do not agree with putting deprecation's back in code. There must be another code solution. Take a look at nemo for example.

lukefromdc commented 1 year ago

I tested scaling and it worked with this PR, switching between gtk-window-scaling = 2 and gtk-window-scaling = 1

lukefromdc commented 1 year ago

Just checked Nemo from current master. the string "window_size_changed" desktop or otherwise does not appear at all in the code. Note that Nemo uses a transparent desktop and does not draw the background however. Will try removing this entirely and see what happens

raveit65 commented 1 year ago

I found time testing it for myself, and it seems scaling isn't broken. So decide for yourself to revert this or not. Several people complains about the deprecation's here in PR. But i really don't like such fast mergeing with our main application without proper testind from several people. And i don't like this situation now with weird emotions in the team. This was exactly one reason why i stepped from developing Mate 1,5 years ago. Looks like i'm too close again.

lukefromdc commented 1 year ago

For the past year or so it's been difficult to get enough reviews, which has set up a balancing act between getting enough reviews and dealing with issues. Bugfixes are the sort of thing that people need done the fastest, new features the slowest, and style fixes and code cleanups in between.

For me, the lack of formal guidelines about when something should be merged is an issue: no required number of reviews-and it we did have one some important work would rot on the vine along with the less important stuff. In a larger team, a requirement for X number of reviews or Y amount of wait time after one less than that many reviews and no disapproving reviews might make sense.

To really get enough testing, we need a significant user base of people running development releases, like when GNOME 2's development versions were included in Ubuntu alphas. That made it much easier to catch problems before they ever got to a release.

Finally, I am autistic and some parts of team dynamics and implied things will be invisible to me

lukefromdc commented 1 year ago

The current GDK docs imply that the way to cover the multimonitor case is to set the window dimensions for each window separately, which would be a lot of new code. I will play with this a bit and see if a reasonable solution exists that works in all cases and gets rid of the deprecated code

Using the code I wrote for wayland worked in all the other cases, multimonitor is OK if caja is restarted and probably OK when not using compiz. In compiz, when neither Caja nor mate-settings daemon is managing a part of the background (this includes an entire secondary monitor getting ignored), instead of a sane default such as black (general noncompositing default),we have a background that can be black in some cases, transparent to the rest of the cube when a compiz cube is moving, and which has the redraw issue.

zhuyaliang commented 1 year ago

@lukefromdc I am not ready to merge this PR yet. There should be a better solution, and I will fix it in the next PR

lukefromdc commented 1 year ago

I just tested removing caja_desktop_window_screen_size_changed entirely, and everything worked EXCEPT the multimonitor case. It's possible this would also mean replacing the monitor with a larger one would also mean unmanaged space.

I will try root window dimensions and see if that can be made to work in all cases.

We can do a new PR to update this one when we get something that always worked. I just set my second screen back up specfically to test this stuff

lukefromdc commented 1 year ago

This three line change worked, for x11 we can simply key to the root window's side and bypass iterating over monitors or using deprecated screen functions.

         GdkWindow *root_window;
         root_window = gdk_screen_get_root_window (screen);
         gdk_window_get_geometry (root_window, NULL, NULL, &width_request, &height_request);

I tested this with changes of monitor resolution, monitor orientation, window scaling, and number of desktops and it worked in all cases. Behavior is the same as in master in terms of icon positions (lower resolution, some go over the edge, higher resolution/lower scaling all icons are rendered in part of the screen but can be moved elsewhere).

lukefromdc commented 1 year ago

See #1727