wayland: Replace fallback CSD with proper one

[PolyMeilex]

Hi!
I wrote a simple CSD implementation for Smithay client toolkit, and I think it could be a good default for winit.
Here is winit running with those decorations:

Let me also answer questions that will most likely come up:

• Why not libdecor?
  • Using libdecor from Rust Wayland ecosystem is nowhere near completion, and in my opinion it will never be. And besides some initial tests with calling it from Rust, no one is actively working on it atm.
  • If I'm wrong and libdecor-rs will be a thing, then we can just drop my impl easily, so it's a win-win scenario.
• Why does it look like GTK/Adwaita?
  • Mutter/Gnome is probably the only big compositor that does not support SSD, so I think it's a good idea to make it fit there, other DEs will render their own SSD anyway, so users of those will be happy anyway.
  • I think it just looks neutral, it's simple and minimal, so I doubt anyone will find this kind of design offensive.
• Why does winit need a good default?
  • Not all developers that use winit are well versed in Linux world, let alone know what Wayland is. So I believe that we have to have a good default, so we get a decent look, even if the developer of an app haven't spent time understanding what Wayland is and why it needs CSD.

closes: #1967

[kchibisov]

I'm not that against the idea behind providing some good client side decorations, however as war as I can tell they're always white? Would be nice to have a dark ones, but that's not really a blocker.

Also, do they support text rendering or something like that? Since that should be supported if we're going to outsource it for some other crate.

[PolyMeilex]

Would be nice to have a dark ones

Dark mode is definitely planned, but my problem is that I don't know how to expose it, if this was just my app I would simply get a dark mode preference from dbus desktop portal, but that would mean bringing a full dbus library to winit, I don't think that would be good for compile times.
So should we maybe just expose dark mode get/set methods to users of winit? (that would be probably Wayland only API)

Also, do they support text rendering or something like that?

I intentionally omitted that, because I was concerned that compile times may discourage winit from merging this, but if we decide that a few more text related dependencies is fine, I will happily implement it.

[kchibisov]

I intentionally omitted that, because I was concerned that compile times may discourage winit from merging this, but if we decide that a few more text related dependencies is fine, I will happily implement it.

I'd probably put that under the feature flag. You may try crossfont or something more rusty.

[PolyMeilex]

Title and Dark/Light/Custom themes were added.
Adding text rendering did not change compile times in any significant way, so it's always enabled. I guess I was needlessly worried about that.

[kchibisov]

That looks good, however I'd guess that you're using some hard coded font? Honestly I'd suggest to use https://github.com/alacritty/crossfont, since it basically a thin wrapper around system fontconfig and freetype. And then request sans-serif font.

Though, its API could be not ready, but you can ping wrt it in #alacritty.

The problem is that if we want similar text look, we should use freetype for like everything...

I'm not suggesting other font crates, since I haven't used them, they lack automatic fallback, and they are quite heavy compared to crossfont...

[PolyMeilex]

It is now ported to crossfont, glyph positioning is kinda hacky because crossfont heavily assumes monospace everywhere, I will redo it once alacritty/crossfont#40 is merged and released. Nonetheless it looks good enough even now.

Any ideas why CI is unhappy with crossfont/FreeType? Both libfreetype-dev and libfontconfig-dev are available.

[kchibisov]

@PolyMeilex try adding pkg-config?

[kchibisov]

The code looks good, just some minor notes.

[kchibisov]

@chrisduerr how you'd prefer handling of theme coloring downstream? Via winit env or exposing some sort of config/env variable for alacritty?

The only way I'm aware of picking the right theme variant is via dbus api, however it's not something I want/we should add.

With the current API it's more like downstream option, since there's a with_theme name.

Might remove the method entirely and handle everything via env variable?

[kchibisov]

@PolyMeilex have you considered adding different button for maximized when the window is already maximized? I think that's what everyone is doing and could better for ux.

Should probably bump crossfont as well, since proportional font rendering isn't ideal right now.

[PolyMeilex]

@PolyMeilex have you considered adding different button for maximized when the window is already maximized? I think that's what everyone is doing and could better for ux.

Done

Should probably bump crossfont as well, since proportional font rendering isn't ideal right now.

Done, hacky positioning replaced with proper one that makes use of new crossfont, looks a lot better.

Might remove the method entirely and handle everything via env variable?

Env variable does not allow for proper dynamic changing of the theme based on dbus, as client can connect to dbus and listen for theme changes and pass those down to winit.

[chrisduerr]

how you'd prefer handling of theme coloring downstream? Via winit env or exposing some sort of config/env variable for alacritty?

Good question. If we were talking about GTK, I'd strongly encourage using an environment variable so at least you know it's guaranteed to work. But with winit being a little more hands-on, I wouldn't be surprised if there was no way to do this for all winit apps (since users are usually unaware of their apps using winit anyway).

I feel like the best choice might be to just load based on variable by default and allow overriding on demand via winit's API?

[kchibisov]

Just some minor comments. Should be good to go after that.

[kchibisov]

The CHANGELOG bits are optional, but we're getting close. Thanks for your work so far.

[kchibisov]

Could you also apply this

diff --git a/src/platform_impl/linux/wayland/window/mod.rs b/src/platform_impl/linux/wayland/window/mod.rs
index 7bd98854..601d9c8a 100644
--- a/src/platform_impl/linux/wayland/window/mod.rs
+++ b/src/platform_impl/linux/wayland/window/mod.rs
@@ -158,12 +158,7 @@ impl Window {
                 }
             });
 
-            let config = match theme {
-                Theme::Light => sctk_adwaita::FrameConfig::light(),
-                Theme::Dark => sctk_adwaita::FrameConfig::dark(),
-            };
-
-            window.set_frame_config(config);
+            window.set_frame_config(theme.into());
         }
 
         // Set decorations.
@@ -577,3 +572,13 @@ impl Drop for Window {
         self.send_request(WindowRequest::Close);
     }
 }
+
+#[cfg(feature = "sctk-adwaita")]
+impl From<Theme> for sctk_adwaita::FrameConfig {
+    fn from(theme: Theme) -> Self {
+        match theme {
+            Theme::Light => sctk_adwaita::FrameConfig::light(),
+            Theme::Dark => sctk_adwaita::FrameConfig::dark(),
+        }
+    }
+}
diff --git a/src/platform_impl/linux/wayland/window/shim.rs b/src/platform_impl/linux/wayland/window/shim.rs
index b9defe0e..03d64e63 100644
--- a/src/platform_impl/linux/wayland/window/shim.rs
+++ b/src/platform_impl/linux/wayland/window/shim.rs
@@ -425,11 +425,7 @@ pub fn handle_window_requests(winit_state: &mut WinitState) {
                 }
                 #[cfg(feature = "sctk-adwaita")]
                 WindowRequest::CsdThemeVariant(theme) => {
-                    let config = match theme {
-                        Theme::Light => sctk_adwaita::FrameConfig::light(),
-                        Theme::Dark => sctk_adwaita::FrameConfig::dark(),
-                    };
-                    window_handle.window.set_frame_config(config);
+                    window_handle.window.set_frame_config(theme.into());
 
                     let window_update = window_updates.get_mut(window_id).unwrap();
                     window_update.refresh_frame = true;

[PolyMeilex]

Could you also apply this

Done

[kchibisov]

Thanks!