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.

Sign up for GitHub

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

Jump to bottom

feat: enable dark mode on GTK UIs #38977

Merged
merged 2 commits into from Sep 27, 2023
Merged

feat: enable dark mode on GTK UIs #38977

merged 2 commits into from Sep 27, 2023

Conversation

robertgzr
Copy link
Contributor

@robertgzr robertgzr commented Jul 3, 2023 •
edited by ckerr

Description of Change

This is needed after https://bugs.chromium.org/p/chromium/issues/detail?id=998903
and replaces the previous workaround to detect dark mode on GTK.
Detect system dark theme preference via xdg settings portal:
https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Settings

Closes: #38961
Closes #28838

cc @codebytere @ckerr

Checklist

Release Notes

Notes: Honor XDG dark theme preferences on Linux.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jul 3, 2023
@robertgzr
Copy link
Contributor Author

robertgzr commented Jul 3, 2023 •
edited

this also fixes #28838, and #28887 (maybe, haven't tested on Ubuntu 20), tested using https://github.com/electron/electron/tree/main/docs/fiddles/features/dark-mode

@robertgzr
Copy link
Contributor Author

robertgzr commented Jul 3, 2023 •
edited

Since Electron/Chromium use GTK3 (EDIT: or at least not libadwaita), the system needs to have org.gnome.desktop.interface gtk-theme set to the correct variant for native UI elements to actually be styled.

So when using Gnome, Adwaita (for light mode) and Adwaita-dark (for dark mode)

@robertgzr robertgzr changed the title Port DarkModeManagerLinux feat: port DarkModeManagerLinux Jul 3, 2023
@codebytere
Copy link
Member

I don't feel strongly either way but we can likely call this a fix and mark it semver/patch cc @electron/wg-api

@codebytere codebytere added the semver/patch backwards-compatible bug fixes label Jul 4, 2023
@codebytere codebytere changed the title feat: port DarkModeManagerLinux feat: enable dark mode on GTK UIs Jul 4, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jul 4, 2023
@dsanders11
Copy link
Member

#28887 (maybe, haven't tested on Ubuntu 20)

A quick test on Ubuntu 22 determines that issue is not resolved by these changes.

this also fixes #28838

That one doesn't repro for me on Ubuntu 22 (haven't tested on Ubuntu 20).

I haven't reviewed the changes in this PR, just chiming in after testing it on Ubuntu 22.

dsanders11
dsanders11 reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@dsanders11 dsanders11 left a comment

Choose a reason for hiding this comment

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

#include "ui/gfx/color_utils.h" becomes unused with these changes and should be removed in shell/browser/electron_browser_main_parts.cc.

@dsanders11 dsanders11 added target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Jul 5, 2023
This was referenced Jul 7, 2023
Closed
Closed
@robertgzr
Copy link
Contributor Author

tested for gnome 42.9 on ubuntu 22.01:

2023-07-14_electron_darkmode_gnome.mp4

behavior is the same on fedora 38 with KDE, menubar style follows system UI elements

@robertgzr
Copy link
Contributor Author

i tested this out on ubuntu 20 (gnome 3.36.8) and it seems that there is some inconsistency when switching, even with native apps:

2023-07-14_electron_darkmode_ubuntu20.mp4

@robertgzr robertgzr mentioned this pull request Jul 14, 2023
Open
3 tasks
@robertgzr
feat: port DarkModeManagerLinux
f035068 
This is needed after https://bugs.chromium.org/p/chromium/issues/detail?id=998903
and replaces the previous workaround to detect dark mode on GTK.
Detect system dark theme preference via xdg settings portal:
https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Settings

Closes: electron#38961
Closes: electron#28838
Signed-off-by: Robert Günzler <r@gnzler.io>
@robertgzr
Copy link
Contributor Author

that test failure looks like a flake? https://app.circleci.com/pipelines/github/electron/electron/72486/workflows/a9b3d28b-bbf6-42fc-9f50-ee789af823c7/jobs/1567694/tests

@deepak1556 deepak1556 mentioned this pull request Aug 16, 2023
Closed
@github-actions github-actions bot added the target/27-x-y PR should also be added to the "27-x-y" branch. label Aug 16, 2023
@nekohayo nekohayo mentioned this pull request Sep 8, 2023
Open
1 task
@q234rty
Copy link

q234rty commented Sep 15, 2023

Gentle ping on this :)

@Igetin Igetin mentioned this pull request Sep 19, 2023
Open
@jkleinsc jkleinsc merged commit 480f48b into electron:main Sep 27, 2023
17 of 18 checks passed
@release-clerk
Copy link

release-clerk bot commented Sep 27, 2023 •
edited by ckerr

Release Notes Persisted

Honor XDG dark theme preferences on Linux.

This was referenced Sep 27, 2023
Closed
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Sep 27, 2023

I have automatically backported this PR to "24-x-y", please check out #40008

@trop trop bot added in-flight/24-x-y and removed target/24-x-y PR should also be added to the "24-x-y" branch. labels Sep 27, 2023
@trop
Copy link
Contributor

trop bot commented Sep 27, 2023

I have automatically backported this PR to "25-x-y", please check out #40009

@trop
Copy link
Contributor

trop bot commented Sep 27, 2023

I have automatically backported this PR to "27-x-y", please check out #40010

@trop
Copy link
Contributor

trop bot commented Sep 27, 2023

I have automatically backported this PR to "26-x-y", please check out #40011

@trop trop bot added in-flight/27-x-y in-flight/26-x-y merged/27-x-y PR was merged to the "27-x-y" branch. and removed target/25-x-y PR should also be added to the "25-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. in-flight/24-x-y in-flight/27-x-y labels Sep 27, 2023
@Danik1601
Copy link

Danik1601 commented Sep 27, 2023 •
edited

I have automatically backported this PR to "24-x-y", please check out #40008

AFAIK the upstream fix has landed in Chromium 114 (Electron 25), so PR for Electron 24 should be rejected. Other PRs, however, should be good to go.

Edited: Forget everything I just said because of #40008 (comment)

@trop trop bot added merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. and removed in-flight/25-x-y in-flight/26-x-y labels Sep 27, 2023
@robertgzr robertgzr deleted the dark-mode branch September 28, 2023 15:55
@codebytere codebytere mentioned this pull request Nov 20, 2023
Closed
3 tasks
@shiftkey shiftkey mentioned this pull request Nov 26, 2023
Closed
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@robertgzr @jkleinsc
feat: enable dark mode on GTK UIs (electron#38977)
877174c 
feat: port DarkModeManagerLinux

This is needed after https://bugs.chromium.org/p/chromium/issues/detail?id=998903
and replaces the previous workaround to detect dark mode on GTK.
Detect system dark theme preference via xdg settings portal:
https://flatpak.github.io/xdg-desktop-portal/#gdbus-org.freedesktop.portal.Settings

Closes: electron#38961
Closes: electron#28838

Signed-off-by: Robert Günzler <r@gnzler.io>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsanders11 dsanders11 dsanders11 left review comments

@jkleinsc jkleinsc jkleinsc approved these changes

@ckerr ckerr Awaiting requested review from ckerr

@codebytere codebytere Awaiting requested review from codebytere

Assignees
No one assigned
Labels
merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
6 participants
@robertgzr @codebytere @dsanders11 @q234rty @Danik1601 @jkleinsc