Skip to content

fix: properly handle light/dark theme switch #18324

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

Merged
merged 1 commit into from
Jul 9, 2025

Conversation

caybro
Copy link
Member

@caybro caybro commented Jul 8, 2025

What does the PR do

  • under Qt6, we can now listen to the styleHints color scheme property changes and react accordingly
  • remove the hack of detecting the current dark theme by checking the hsl lightness of the window; also removes propagating the systemPalette object thru the hierarchy
  • as a result, we can get rid of the dedicated C++ OSThemeEvent

Fixes #17764
Fixes #14856

Needs status-im/dotherside#95
Needs status-im/nimqml#67

Affected areas

App/Settings

Architecture compliance

Screencapture of the functionality

Zaznam.obrazovky.z.2025-07-08.20-04-35.mp4

Storybook:
image

Impact on end user

Settings/Appearance/System should follow system light/dark theme changes

@caybro caybro linked an issue Jul 8, 2025 that may be closed by this pull request
@status-im-auto
Copy link
Member

status-im-auto commented Jul 8, 2025

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 814575d #1 2025-07-08 18:04:13 ~7 min tests/nim 📄log
✔️ 814575d #1 2025-07-08 18:07:50 ~10 min macos/aarch64 🍎dmg
✔️ 814575d #1 2025-07-08 18:09:46 ~12 min tests/ui 📄log
✔️ 814575d #1 2025-07-08 18:12:59 ~15 min linux/x86_64 📦tgz
✔️ 814575d #1 2025-07-08 18:29:53 ~32 min windows/x86_64 💿exe
✔️ e6b2d68 #2 2025-07-09 15:19:28 ~7 min tests/nim 📄log
✔️ e6b2d68 #2 2025-07-09 15:19:36 ~7 min macos/aarch64 🍎dmg
✔️ e6b2d68 #2 2025-07-09 15:24:58 ~12 min tests/ui 📄log
✔️ e6b2d68 #2 2025-07-09 15:25:09 ~12 min linux/x86_64 📦tgz
✔️ e6b2d68 #2 2025-07-09 15:46:49 ~34 min windows/x86_64 💿exe

@caybro caybro marked this pull request as ready for review July 8, 2025 18:05
@caybro caybro requested review from micieslak, alexjba, noeliaSD and a team as code owners July 8, 2025 18:05
Copy link
Member

@micieslak micieslak left a comment

Choose a reason for hiding this comment

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

Cool! I tested on Linux and Mac and it works really nice.

The thing that is questionable to me is using system theme on login page even if dark/light is explicitly specified in settings:

Screencast.from.09.07.2025.11.49.50.webm

I think that we should respect user's choice also on the login page.

The other thing is why we need to keep this on the nim side. I think that property should not be synced - it's specific to given device, therefore regular qt's Settings should be suitable here.

@caybro
Copy link
Member Author

caybro commented Jul 9, 2025

Cool! I tested on Linux and Mac and it works really nice.

The thing that is questionable to me is using system theme on login page even if dark/light is explicitly specified in settings:

Screencast.from.09.07.2025.11.49.50.webm
I think that we should respect user's choice also on the login page.

The other thing is why we need to keep this on the nim side. I think that property should not be synced - it's specific to given device, therefore regular qt's Settings should be suitable here.

It's done that since before the login, there are no user preferences available (no user/profile logged in yet -> no settings), so we opt to always follow the system theme. Nothing we could do/fix here really

@caybro
Copy link
Member Author

caybro commented Jul 9, 2025

Cool! I tested on Linux and Mac and it works really nice.
The thing that is questionable to me is using system theme on login page even if dark/light is explicitly specified in settings:
Screencast.from.09.07.2025.11.49.50.webm
I think that we should respect user's choice also on the login page.
The other thing is why we need to keep this on the nim side. I think that property should not be synced - it's specific to given device, therefore regular qt's Settings should be suitable here.

It's done that since before the login, there are no user preferences available (no user/profile logged in yet -> no settings), so we opt to always follow the system theme. Nothing we could do/fix here really

    Component.onCompleted: {
        console.info(">>> %1 %2 started, using Qt version %3".arg(Qt.application.name).arg(Qt.application.version).arg(SystemUtils.qtRuntimeVersion()))

        Theme.changeTheme(Theme.Style.System);

^^ this is main.qml, during Onboarding but the settings are stored in localAppSettings which is not around at this point

@caybro caybro requested a review from micieslak July 9, 2025 11:16
Copy link
Member

@jrainville jrainville left a comment

Choose a reason for hiding this comment

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

Awesome!

It's funny how the color switch when using the System theme is super smooth and when switching manually in the app is very sudden, but it's not a real issue

@caybro
Copy link
Member Author

caybro commented Jul 9, 2025

Awesome!

It's funny how the color switch when using the System theme is super smooth and when switching manually in the app is very sudden, but it's not a real issue

Yeah because it's the system/OS compositor doing the color transition, not us 😆

- under Qt6, we can now listen to the styleHints color scheme property
changes and react accordingly
- remove the hack of detecting the current dark theme by checking the
hsl lightness of the window; also removes propagating the systemPalette
object theu the hierarchy
- as a result, we can get rid of the dedicated C++ OSThemeEvent

Fixes #17764
@caybro caybro force-pushed the 17764-improper-lightdark-mode branch from 814575d to e6b2d68 Compare July 9, 2025 15:11
@caybro caybro merged commit dc509ca into master Jul 9, 2025
7 checks passed
@caybro caybro deleted the 17764-improper-lightdark-mode branch July 9, 2025 16:10
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.

[Qt 5->6]: Improper light/dark mode of the onboarding [Linux/Windows] The System theme doesn't work on windows and linux
4 participants