Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed a regression where Linux theme is not properly detected #1382

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

veler
Copy link
Collaborator

@veler veler commented Aug 17, 2024

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • New feature or enhancement
  • UI change (please include screenshot!)
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes
  • Internationalization and localization
  • Other (please describe):

What is the current behavior?

Issue Number: #1381

What is the new behavior?

Use DBus to retrieve the org.freedesktop.appearance parameter to detect the theme on Linux Mint, Ubuntu, Fedora.

Other information

Quality check

Before creating this PR:

  • Did you follow the code style guideline as described in CONTRIBUTING.md
  • Did you build the app and test your changes?
  • Did you check for accessibility? On Windows, you can use Accessibility Insights for this.
  • Did you verify that the change work in Release build configuration
  • Did you verify that all unit tests pass
  • If necessary and if possible, did you verify your changes on:
    • Windows
    • macOS
    • Linux

@CodedOre
Copy link

Looks good to me. On Fedora, the app now follows dark mode again, but the titlebar remains light:

Bildschirmfoto vom 2024-08-17 06-01-01

Which I guess is to expected since GTK itself still doesn't know about the dark mode preference...

Copy link
Contributor

@badcel badcel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good at a first glance. You should be able to remove the Gio initialization though.

I will try to test your change on my hardware and report back.

@@ -31,6 +31,7 @@ internal partial class LinuxProgram

internal LinuxProgram()
{
Gio.Module.Initialize();
Copy link
Contributor

@badcel badcel Aug 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can remove this line. This is part of the sample as the Gio.Module is not initialized automatically.

For Libadwaita and GTK applications this is done automatically via the Gtk.Application class for all GTK dependencies including Gio.

@badcel
Copy link
Contributor

badcel commented Aug 17, 2024

@CodedOre Thanks for testing then I will skip the testing on my side.

Making the title bar dark should be possible, too.

@CodedOre
Copy link

Looking through the docs, I think setting the gtk-application-prefer-dark-theme property should work?

Though I haven't tried that yet.

@badcel
Copy link
Contributor

badcel commented Aug 17, 2024

@CodedOre Yes this works, tested on fedora 40 with:

var application = Gtk.Application.New("org.gir.core", Gio.ApplicationFlags.FlagsNone);
application.OnActivate += (sender, args) =>
{
    var window = Gtk.ApplicationWindow.New((Gtk.Application) sender);
    window.Title = "Gtk4 Window";
    window.SetDefaultSize(300, 300);
    window.GetSettings().GtkApplicationPreferDarkTheme = true;
    window.Show();
};

return application.RunWithSynchronizationContext(null);

So setting GtkApplicationPreferDarkTheme to true seems to be the right thing to do.

@veler It should work if you use the Gtk.Settings you already have around in your code. No need to get the Window object involved.

@badcel
Copy link
Contributor

badcel commented Aug 17, 2024

I think Gtk.Settings.GtkApplicationPreferDarkTheme should not be used as a fallback decider for the preferred theming mode. This is implied by the linked docs of @CodedOre, too.

I think it is more meant as an "application level setting" defining the developers preference of either a dark or light theme. E.g. in GNOME there are apps, which use a dark theme, even if the user chooses to have a light theme. This is the case for media related apps like video players. I think those apps set this setting to indicate that they prefer a dark theme over the user choise.

And If your code detects, the user wants to use a dark theme you set this property to true to adhere to the users wish resulting in a dark titlebar.

@badcel
Copy link
Contributor

badcel commented Aug 17, 2024

@veler Are you aware that the dbus call might fail regularly in case of old distributions (API not available). In those cases you would not check the GTK theme currently.

@veler
Copy link
Collaborator Author

veler commented Aug 18, 2024

Hey everyone! Thank you all for your feedback, it's super helpful :)
I think I updated the PR according to your suggestions. I'm struggling at setting up a Fedora somehow. I wouldn't mind if one of you could try it. On my side, I tested on Mint and Ubuntu.

@veler
Copy link
Collaborator Author

veler commented Aug 18, 2024

Alright, I finally got Fedora to work!
The title bar now follows the theme of DevToys. However, DevToys does not detect the system's theme. userThemePreference on line 179 is 0, so it fallback on checking the theme name, which is just "adwaita", whether it's dark or not.

I'm happy to take suggestions, otherwise I kind of happy with the current state too. I know it's not perfect but it's not a blocking issue.

@CodedOre
Copy link

Which edition of Fedora did you install?

Because the Settings portal, like the other XDG desktop portals, depend on a implementation in the desktop to provide the information.

Gnome and Elementary have implemented the new portal, as well as KDE (partially at least).
But some like Cinnamon or Xfce might not provide the preference yet, so there a fallback to the theme name would be used.

@CodedOre
Copy link

I did just check with my Fedora desktop (to be precise, Fedora Silverblue with the Gnome desktop), and it does work fine and is indeed using the portal.

@badcel
Copy link
Contributor

badcel commented Aug 18, 2024

@veler you could double check if you use "D-Spy" to execute the call to the portal interface manually like I showed in the screenshot.

Perhaps you really have some variant / version which does not have this feature available?

@veler
Copy link
Collaborator Author

veler commented Aug 19, 2024

Hey there,
Thanks for your comments :)

I made a virtual machine of Fedora Workstation 40 using the ISO from the Fedora website.
If I'm not wrong, @badcel , I'm getting the same result than you in D-Bus. This is in my Fedora, currently set to Dark mode.

image

Am I understanding correctly it finds "1" ? Also, when I switch the theme to Default, then the value is "0".

image

@CodedOre
Copy link

The values from the settings portal looks right to me. If you look at the portal documentation, the value 1 stands for dark mode, while 0 for "no preference". The reason Gnome sets "no preference" instead of "prefer light" on the default setting is that some apps on default would prefer to be dark (like image viewers), while most apps use light as default.

@CodedOre
Copy link

So, I so notice that as of now, on the default setting the app uses the fallback. So, this could be improved by calling the fallback if the DBus call returns an error (like when it did not find the portal).

Comment on lines +175 to +177
);

uint userThemePreference = ret.GetChildValue(0).GetVariant().GetVariant().GetUint32();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhere here you should, if I read the documentation right, check for an G_IO_ERROR_CLOSED error, in which case the fallback should be called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks ! (and sorry for the late reply)
I will try to do this soon. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If an error occurs, the bus.CallSync() call will throw an exception as gir.core interpretes the error argument by itself and raises an exception. As there is already a try/catch in place this should be fine.

So you probably only need to explicitly define in the switch expression which Theme DevToys should use if the user has no preference instead of calling the fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Linux: Use desktop portals to get dark theme information
3 participants