khonsulabs / cushy

An experimental cross-platform graphical user interface (GUI) crate for Rust.
Apache License 2.0
491 stars 25 forks source link

Is `themed_mode` supposed to have an effect on widgets? #196

Closed bluenote10 closed 4 weeks ago

bluenote10 commented 1 month ago

I was wondering if it is on purpose that using .themed_mode(...) doesn't seem to have an effect. The following example

use cushy::widget::MakeWidget;
use cushy::window::ThemeMode;
use cushy::Run;

pub fn main() -> cushy::Result<()> {
    "Click me".into_button().themed_mode(ThemeMode::Dark).run()
}

renders as image

I.e., it looks like the theme mode is ignored.

It only seems to work if I squeeze in an .into_window() before .themed_mode(...). This made me wonder if .themed_mode() should rather be defined on Window instead of Widget?

ecton commented 1 month ago

For me it does work:

image

use cushy::widget::MakeWidget;
use cushy::window::ThemeMode;
use cushy::Run;

pub fn main() -> cushy::Result<()> {
    "Click me"
        .into_button()
        .themed_mode(ThemeMode::Dark)
        .and("Click me".into_button().themed_mode(ThemeMode::Light))
        .into_columns()
        .run()
}

It's always possible for some colors to be the same in both light and dark mode, but in this case the grays are distinct. The theme.rs example also shows how applying a theme and theme mode to an entire window can be done dynamically and all widgets respond to the changes.

bluenote10 commented 1 month ago

Interesting, I should have mentioned that this is with the latest main branch.

On 0.4.0 your example looks like this for me:

image

But trying it on commit 62dded16eff6b6b1b56257b1b9c728825df36673 it becomes:

image

This is not quite what I was expecting, but perhaps it indeed makes sense if the window itself isn't set to dark mode. If I add .into_window().themed_mode(ThemeMode::Dark) at the bottom after .into_columns() I get:

image

Note that the window decoration itself is now dark as well, and apparently this may as well affect the clear color.

What about an example involving some containers:

use cushy::widget::MakeWidget;
use cushy::window::ThemeMode;
use cushy::Run;

pub fn main() -> cushy::Result<()> {
    "Foo"
        .into_button()
        .contain()
        .and("Bar".into_button().contain())
        .into_columns()
        .contain()
        .themed_mode(ThemeMode::Dark)
        .run()
}

On the latest commit this renders as:

image

Isn't the outer .themed_mode(ThemeMode::Dark) call supposed to make the containers use a dark theme? Only if I add the into_window() again, I get cushy's beautiful dark mode :wink::

image

The reason I was reading the window.rs code yesterday was because I was curious to see if the default theme has changed from dark to light on main, but I couldn't find anything obvious. Perhaps it is simply a side effect of also updating e.g. winit, and now the system default actually gets properly respected by cushy as a result or so.

ecton commented 1 month ago

Another user @ModProg wrote a crate that allows detecting dark mode through xdg on Linux now, including when the mode is changed while the app is running. This change was done in appit, and is transparent to Kludgine/Cushy.

That's a good note about how it can be confusing for themed_mode to not change the window's theme unless called directly on a Window. In theory I can allow a widget to report this via RootBehavior so that if themed mode is set at the root of a window, it also affects the window itself.

The use case of setting a theme-mode in a sub-tree is very minimal. But all theming is done through the style system which resolves what the current theme mode is at runtime, and using the ThemedMode widget just overrides whatever the window's setting is.

I'm going to leave this issue open to address the idea of allowing this data to pass through the root behavior system.

bluenote10 commented 4 weeks ago

So are there actually two aspects here?

  1. Whether the "outermost" theme mode (not set on the window) can affect the window theme (outwards propagation).
  2. A possible bug that the theme mode is not properly passed inwards (inward propagation).

Regarding (1) I think it the current behavior may be fine as well: If the general propagation is "inwards", it makes sense that it has to be set on the outermost level, which happens to be the window to affect window decoration and clear color.

But isn't there also something weird about the inward propagation (2)? Another example:

use cushy::widget::MakeWidget;
use cushy::window::ThemeMode;
use cushy::Run;

fn group() -> impl MakeWidget {
    "Foo"
        .into_button()
        .contain()
        .and("Bar".into_button().contain())
        .and("Baz".into_button().contain())
        .into_rows()
        .contain()
}

pub fn main() -> cushy::Result<()> {
    group()
        .and(group().themed_mode(ThemeMode::Dark))
        .into_columns()
        .contain()
        .themed_mode(ThemeMode::Dark)
        .run()
}

which renders as:

image

Why is the inner of the two .themed_mode(ThemeMode::Dark) propagated inwards, but not the outer one? Even if the clear color stays white (and light window decorations), I would have expected all containers to use dark colors if the outermost .themed_mode(ThemeMode::Dark) gets propagated inwards, no?

ecton commented 4 weeks ago

Great catch! That was absolutely a bug. There were situations that contexts were being created using the window's theme. I've pushed a fix to ensure that the widget's theme mode is always used.

I know the "root widget" magic can be a little weird, but it also is really nice when it works. In general, however, I do think that most developers that are building anything more complex than a demo will almost certainly be calling into_window to be able to do things like customize the window's title, so having the current behavior absolutely isn't the end of the world.

bluenote10 commented 4 weeks ago

In general, however, I do think that most developers that are building anything more complex than a demo will almost certainly be calling into_window to be able to do things like customize the window's title, so having the current behavior absolutely isn't the end of the world.

Yes indeed, in my main app I also use an explicit window, and didn't noticed the issue there, only in smaller experiments/snippets.

Coincidentally in all these experiments/snippets I always used the previously broken outer .contain().themed_mode(...).run() pattern, which made me think themed_mode(...) had no effect whatsoever on a widget. I think we can close this issue, because after the fix in 0fe7f789693a12bf61c5a3cba32b69d0e3a8bb9a this aspect should be fully working now.