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

UI/Qt: Set dark theme as reported by Qt #2529

Merged
merged 1 commit into from
Nov 23, 2024

Conversation

shlyakpavel
Copy link
Contributor

@shlyakpavel shlyakpavel commented Nov 23, 2024

Previously, we did a bit of guessing.
That approach was hacky and less reliable.

@yyny
Copy link
Contributor

yyny commented Nov 23, 2024

Nit:

That approach was hacky and less reliable.

Copy link
Member

@tcl3 tcl3 left a comment

Choose a reason for hiding this comment

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

This doesn't compile for me. According to the docs this property was added in 6.5. On Ubuntu, I have 6.4.2 installed.

It looks like we do use #if (QT_VERSION > QT_VERSION_CHECK(major, minor, patch)) elsewhere in the code, so you could use that to ensure you only use the built in way of doing things if the QT_VERSION is new enough.

@shlyakpavel
Copy link
Contributor Author

@tcl3 elsewhere, we're using QT_VERSION_CHECK(6, 6, 0) and higher. Are you sure it's a good idea to support ancient Qt versions?

@shlyakpavel shlyakpavel force-pushed the qt_ui_dark_theme branch 2 times, most recently from 5bd2e6c to cac2ff5 Compare November 23, 2024 17:32
@ladybird-bot
Copy link
Collaborator

Hello!

One or more of the commit messages in this PR do not match the Ladybird code submission policy, please check the lint_commits CI job for more details on which commits were flagged and why.
Please do not close this PR and open another, instead modify your commit message(s) with git commit --amend and force push those changes to update this PR.

For Qt >= 6.5, the system theme can be determined reliably, so no
guesswork is needed. A fallback remains for Qt < 6.5, but it is
hacky and less reliable.
Copy link
Member

@tcl3 tcl3 left a comment

Choose a reason for hiding this comment

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

This works very nicely on MacOS (with Qt 6.7), where the page will now update live in response to changes in the system color theme 🎉

@tcl3 tcl3 merged commit 6ad93d2 into LadybirdBrowser:master Nov 23, 2024
6 checks passed
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.

4 participants