search

elementary / default-settings

Default settings for elementary OS
GNU General Public License v3.0
39 stars 28 forks source link

Delete gtk-csd.sh #103

Closed davidmhewitt closed 3 years ago

davidmhewitt commented 5 years ago

Fixes #207 Fixes https://github.com/elementary/gala/issues/244

Can anyone remember why this is still here and what issues removing it would cause?

cassidyjames commented 5 years ago

I believe it's because we switched to CSD before it was standard, i.e. back in Freya or Loki. If it's now standard I guess that option could be unnecessary and unsupported?

davidmhewitt commented 5 years ago

@cassidyjames Indeed, that would make sense.

Although, it looks like @danrabbit already had a stab at removing it a couple of years back and then it got restored for some reason, so that makes me suspicious: https://github.com/elementary/default-settings/commit/6d6d3299f457cb4063546b65b58c3faef1cdd123

danirabbit commented 5 years ago

I think it might have been something to do with dialog decorations maybe? We should give it a try again and be on the look out for regressions

cassidyjames commented 5 years ago

@danrabbit could this be why root dialogs don't have the correct decorations? If this isn't set in the root profile or something?

danirabbit commented 5 years ago

@cassidyjames oh that's very possible. Like on the greeter for example?

cassidyjames commented 5 years ago

@danrabbit ah yeah that's another example. But like when running the installer as root, or if you use Files' "Run as Administrator" and then look at a properties dialog.

davidmhewitt commented 5 years ago

Marking this as blocked as it definitely does some weird stuff to dialogs. All dialogs now have a titlebar and it seems it somehow breaks the file/folder chooser completely too.

I'll look into these issues and see if there's a middle ground solution.

tintou commented 5 years ago

So to me it seems like it is an issue with WxWidgets, after investigating, I think that this is cause by https://github.com/wxWidgets/wxWidgets/blob/cc931612eec2e3ea49200ebff45042135f3c3f9c/src/gtk/toplevel.cpp#L350 When for us the realize function set it to: https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/gtk/gtkwindow.c#L7488 (client_decorated is set to TRUE in https://gitlab.gnome.org/GNOME/gtk/blob/gtk-3-24/gtk/gtkwindow.c#L4155 )

davidmhewitt commented 5 years ago

@tintou Yes, the issue wouldn't occur with wxWidgets based apps if they stopped requesting decorations. But I think they possibly have that there as leftover/compatibility with GTK2, so I don't know how open they would be to removing it and it wouldn't fix anything in this cycle anyway.

Also, it's not just wxWidgets, there seem to be a number of other apps that do this (including eclipse and MySQL workbench, which is native GTK).

davidmhewitt commented 5 years ago

The other issue that compounds this is that Gtk.Dialog requests server side decoration instead of CSD if you turn off the actions in the headerbar thing, which we do. This leaves them being server side decorated and a bit weird looking without the shell script, although I wonder if there's something we could do with the stylesheet to make them fit in again?

I would assume that some apps might want to request server side decorations for a reason, so forcing CSD on them too with this shell script is how we've ended up with double decorations in some apps.

Another option is patching GTK to enable CSD on dialogs even when not using the headerbar action buttons, but I'd guess we wouldn't want to do that either.

:woman_shrugging:

danirabbit commented 5 years ago

@davidmhewitt Ah yes that was the issue. Maybe it's time to revisit with Gtk+. I remember us bringing this up before and the response being "But why would you want CSD without moving the action area too?". I think we probably should pursue a patch upstream that let's us keep using CSD without moving the dialog action area and also not having to have this force CSD script thing.

davidmhewitt commented 5 years ago

Ok, I'm now running a patched version of GTK that doesn't request server side decorations for dialogs that don't use the headerbar as the action area, and the file/folder chooser is no longer broken.

However, some of the dialogs are pretty chunky now: image

I suspect that's because dialogs have a headerbar widget (in their .ui file in the GTK source code) ready to contain the action widgets. Patching that out looks like it'd be a bit harder than just patching the bit that disables SSD.

So I guess the next question is, can we fix that with the stylesheet without breaking anything else? Here's the relevant inspector window with the end session dialog selectors from above visible:

image

davidmhewitt commented 5 years ago

It's worth noting that a patched GTK will fix any other unsightly dialog decorations like running files as root and in the greeter session. So this may be worth pursuing for reasons additional to fixing the apps the request and receive double decorations.

cassidyjames commented 5 years ago

We might just be able to target HeaderBars inside of GtkDialogs with CSS, but should really check if that affects other apps and windows as well.

danirabbit commented 5 years ago

@davidmhewitt The headerbar in the dialog should also have the class default-decoration. See this screenshot from what we have currently:

screenshot from 2018-11-21 13 35 50 2x

davidmhewitt commented 5 years ago

Not necessarily... the default-decoration class only gets added when the Gtk.Window class constructs a default headerbar, I think. As soon as that headerbar is overriden, you no longer get that class.

Since the dialog is adding its own "custom" headerbar, which granted is exactly the same as the one that Gtk.Window would add in the case when there aren't action buttons in it, the class doesn't get added in this case.

davidmhewitt commented 5 years ago

I have prepared a patch for GTK that adds a new XSettings override option called Gtk/DialogsAlwaysCSD. When enabled, GTK dialogs are constructed using a CSD headerbar (with the titlebar style constant and the default-decoration class applied). The has-subtitle property is also set to false as this was making the headerbars chunkier.

If we apply this patch, along with dropping the GTK_CSD env var, as per this PR and setting the XSettings overrides to enable this new setting by default, then we resolve the issues around having this shell script still around, with no visible differences to anything.

Is this something we'd want to pursue?

davidmhewitt commented 5 years ago

For reference, here is the GTK patch: https://gist.github.com/davidmhewitt/1cbb7329701ba766051c12959c804621

danirabbit commented 5 years ago

@davidmhewitt That sounds ideal! Have you proposed this patch upstream in the Gtk Gitlab?

davidmhewitt commented 5 years ago

@danrabbit Not yet, I wrote and tested that patch against the version we're running in Juno. I don't understand GTK versioning well enough to know which branch I should propose merging this into in their repository. Anyone have any ideas?

tintou commented 5 years ago

@davidmhewitt https://gitlab.gnome.org/GNOME/gtk/commits/gtk-3-24 is the latest and last GTK+3 branch

davidmhewitt commented 5 years ago

Upstream merge request opened here: https://gitlab.gnome.org/GNOME/gtk/merge_requests/566

cassidyjames commented 3 years ago

In case it's not obvious how to test this:

sudo nano /etc/profile.d/gtk-csd.h

Comment out the export line, save, then reboot. You'll notice it fixes double-decorations on things like wxwidgets apps, but does show the titlebar on dialogs all over the place, which we'll need to address. I'm kind of the opinion that we rip off the Band-aid and then start filing issues and addressing dialogs as we see them.

gniezen commented 3 years ago

FWIW, the double-window border bug was fixed by wxWidgets v3.0.5 (https://forum.kicad.info/t/fix-available-for-double-window-border-on-elementaryos/22257/8) which is not in the repo's yet, but works when installed manually.