mate-desktop / mate-settings-daemon

MATE settings daemon
https://mate-desktop.org
GNU General Public License v2.0
43 stars 48 forks source link

Scale OSD size correctly on HiDPI displays #212

Closed vkareh closed 6 years ago

vkareh commented 6 years ago

Related: https://github.com/mate-desktop/mate-power-manager/pull/246

vkareh commented 6 years ago

@lukefromdc - second attempt! 😊

I had to change the size calculation to happen each time prior to showing the window, as the original code calculated the size once and it locked it for the session

vkareh commented 6 years ago

@lukefromdc - okay now I feel dumb... All I needed to do was to mark the OSD window as invalid if the scale factor changed, then the key manager will re-instantiate it with the current screen dimensions. Sigh.

I've updated the PR with this. I've also fixed a few build warnings for the process re-spawn in xsettings (and now re-scaling doesn't enable compositing if you're not using it)

lukefromdc commented 6 years ago

OK, that just plain works, will test the m-p-m commits when I get home, have to head out right now

vkareh commented 6 years ago

awesome, thanks @lukefromdc!

flexiondotorg commented 6 years ago

Tested on my Dell XPS 15 in HiDPI and unscaled mode with and without a compositor. Works great.

raveit65 commented 6 years ago

All tests are made in a f26 Quem-kvm VM with 1920x1080 px resolution (non hidpi). I tested switching from windows-scaling=0 to windows-scaling=2 gsettings key and back, without restarting the session. a) It seems to be working in non-compositor case. b) With compositor=ON i see the quarter-rendering bug after switching to scale=2 This issue is gone if i restart the session. And i think the proportions between the 'size' and the 'corner radius' of the audio-OSD are wrong with scale=2 in compositor case.

I don't have mate-1.20 installed on a notebook so i can't test brightness OSD from m-p-m.

lukefromdc commented 6 years ago

@raveit65 , did you run your tests with https://github.com/mate-desktop/mate-settings-daemon/pull/212/commits/7cad241450f7ff4cae3492f477454792f0f261e1 applied? Your results were consistant with my original results prior to that commit, where mate-settings-daemon had to be restarted after setting window-scaling=2. That last commit is supposed to preventy that, and @flexiondotorg reports a successful test of the mate-power-manager work which should be essentially the same code on a real hidpi display https://github.com/mate-desktop/mate-power-manager/pull/246#issuecomment-364633192

vkareh commented 6 years ago

@raveit65 - The border-radius is pixel-fixed in the theme, but the size of the OSD is determined by measuring the size of the screen, not the number of pixels. This means that the ratio of size to corner radius will be different for different scales/resolutions.

Relevant code: https://github.com/mate-desktop/mate-themes/blob/master/desktop-themes/Menta/gtk-3.0/mate-applications.css#L845-L850

MsdOsdWindow.background.osd {
    border-radius: 20px;
    border-style: solid;
    border-width: 1px;
    border-color: rgba(0,0,0,0.8);
}

Does CSS in GTK+ themes support using em units, instead of px? That might solve the problem, but I haven't really played with the theme to test this hypothesis...

Try as I might, I could not reproduce the quarter-rendering issue 😕. The latest commit essentially re-instantiates the entire widget whenever you re-scale, so it should behave as though it's a fresh session.

raveit65 commented 6 years ago

Ok, my fault. For some reasons i didn't applied the last 2 commits from this PR. The quarter-rendering issue is gone now in compositor case.

But maybe i found another problem, not sure if this is reproducible and it is related to PR. If i disable compositor and change scaling from 0 to 2 and back, than i can't enable the compositor again. At this point i need a restart of marco to get the compositor back.

II used this test sequence:

[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
true
[rave@f26 ~]$ gsettings get org.mate.interface window-scaling-factor
0
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager false
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
false
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager true
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
true
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 2
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 0
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager false
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
false
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager true
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
true

**ALL GOOD AT THIS POINT.**

[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager false
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
false
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 2
[rave@f26 ~]$ gsettings get org.mate.interface window-scaling-factor
2
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 0
[rave@f26 ~]$ gsettings get org.mate.interface window-scaling-factor
0
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager true
[rave@f26 ~]$ gsettings get org.mate.Marco.general compositing-manager
true

Now, compostitor isn't enabled but 'gsettings get' tells me it is. This is very weird. But as i said, i hope this is reproducible and i am not sure if this problem still exits before.

raveit65 commented 6 years ago

Does CSS in GTK+ themes support using em units, instead of px? That might solve the problem, but I haven't really played with the theme to test this hypothesis...

Using 'em units' is possible but it doesn't solve the proportions issue. But i don't care much of this problem now, shouldn't be a blocker ;-)

raveit65 commented 6 years ago

I tested m-s-d and m-p-m from master again without using both related PRs. Here the problem with enable the compositor again doesn't exists. Tested with this short sequence:

[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager false
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 2
[rave@f26 ~]$ gsettings set org.mate.interface window-scaling-factor 0
[rave@f26 ~]$ gsettings set org.mate.Marco.general compositing-manager true

So, it looks like the issue comes from this PR... .... or maybe i did a error again......

Please test it.

lukefromdc commented 6 years ago

One differemce in my testing: The composited case was tested in compiz-reloaded (my normal WM) and the noncomposited case in march with compositing kept off. I used fusion-icon to toggle between them

vkareh commented 6 years ago

@muktupavels - yeah, my bad - thanks for pointing that out that I need to free the allocated memory 😊

However, I think it won't be necessary to have that code anymore - I think I know what's happening:

I've been changing marco between composite and non-composite mode by using mate-tweak, which switches between calling marco --replace and marco --replace --no-composite. So I finally started doing that myself in the command line for testing. Then I started noticing that my m-s-d replace enabled compositing every single time, even if I had restarted with --no-composite, which is why I added that new flag to it. Turns out, my composite flag was on all the time in gsettings, but by restarting marco with the --no-composite flag, it essentially overrides the gsettings flag.

In short, the m-s-d code that adds --no-composite is unnecessary as long as you don't use mate-tweak to set the non-composited marco. So, I will remove that new chunk of code and submit a fix for mate-tweak so that it just toggles the flag.

vkareh commented 6 years ago

@raveit65 - okay, I fixed the composite toggle issue now (but still left the changes to fix the build warnings)

raveit65 commented 6 years ago

@vkareh Thx, enabling/disabling the compositor from marco is working now again after scale was toogled. Can you rebase/squashing both PRs after all is done, please?

raveit65 commented 6 years ago

The border-radius is pixel-fixed in the theme, but the size of the OSD is determined by measuring the size of the screen, not the number of pixels.

Are you sure? For me it looks like that the pixel size of the radius is increased too? Otherwise the radius should be looking smaller with scale=2, or not? normal osd with 1920x1080, scale=0 osd-with-compositor

osd with 1920x1080, scale=2 osd-withcompositor-and-scale-2

Or should be the whole OSD size not be larger with scale=2 ? I mean the terminal window is larger too. Maybe this looks different with a HIDPI Monitor..... Correct me if my logic is wrong please?

raveit65 commented 6 years ago

Btw. Submarine themes are using 'border-radius: 30px;' https://github.com/mate-desktop/mate-themes/blob/master/desktop-themes/Blue-Submarine/gtk-3.0/mate-applications.css#L1383 So it looks more dramatical weird than Menta themes.

vkareh commented 6 years ago

@raveit65 - thinking about this hurt my brain! The size of the OSD is calculated based on screen size, so that it looks the same physical size (for example, 10cm) regardless of the scale:

/* assume 130x130 on a 640x480 display and scale from there */
scalew = WidthOfScreen (gdk_x11_screen_get_xscreen (screen)) / 640.0;
scaleh = HeightOfScreen (gdk_x11_screen_get_xscreen (screen)) / 480.0;
scale = MIN (scalew, scaleh);
size = 130 * MAX (1, scale)

However, when you scale=2, the circle that forms the borders is doubled in size (since it's pixel based), while the OSD stays the same size (since it's cm based). If you draw 4 circles in a square pattern, and connect the outside tangents into a rectangle, as the circles grow, the corner radii become much more pronounced if you keep the square the same size. For me, intuitively, it should be the other way around (exactly what you say), but after trying to make sense of it in paper, I think this is the explanation...

muktupavels commented 6 years ago

It should be bigger (2x) like any other window.

vkareh commented 6 years ago

@muktupavels - I think the idea of using an On-Screen Display is that it should look as though it came from the screen itself, rather than as a window. That way it looks consistently-sized across any resolution. The OSD window is technically 2x bigger (pixel-wise), but at a 2x resolution it looks the same size. It just happens to be calculated differently so that the scale factor doesn't affect its physical size.

muktupavels commented 6 years ago

Maybe you are right, seems that it works like this also in GNOME.

raveit65 commented 6 years ago

Yes, in gnome-shell the size of the OSD and the radius are the same if scaling

vkareh commented 6 years ago

yeah, I'm looking at the g-s-d and gnome-shell codes and it seems like OSD windows are rendered on the shell at the UI layer. They use similar calculations to us, which makes sense, since both are ported straight from g-s-d: https://gitlab.gnome.org/GNOME/gnome-shell/blob/master/js/ui/osdWindow.js#L194-209

    _relayout: function() {
        /* assume 110x110 on a 640x480 display and scale from there */
        let monitor = Main.layoutManager.monitors[this._monitorIndex];
        if (!monitor)
            return; // we are about to be removed

        let scalew = monitor.width / 640.0;
        let scaleh = monitor.height / 480.0;
        let scale = Math.min(scalew, scaleh);
        let popupSize = 110 * Math.max(1, scale);

        let scaleFactor = St.ThemeContext.get_for_stage(global.stage).scale_factor;
        this._icon.icon_size = popupSize / (2 * scaleFactor);
        this._box.translation_y = monitor.height / 4;
        this._boxConstraint.minSize = popupSize;
    }

However, they don't scale the OSD window itself, they just scale the icon inside it. This is because possibly the monitor object already contains its own scaling information, so the theme just scales along with it, whereas we're using X11 to determine screen size, which doesn't know anything about scaling...? Anyway, I'm going to stop digging into gnome's javascript layer - I'm going down a rabbit hole 😝

If we don't consider the border-radius issue a blocker, I'll go ahead and merge this.

raveit65 commented 6 years ago

If we don't consider the border-radius issue a blocker, I'll go ahead and merge this.

Not for me, i will reduce the border-radius in themes if someone blame us for this.