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

Store Themes as JSON files #4471

Merged
merged 4 commits into from
Apr 8, 2023

Conversation

Nerixyz
Copy link
Contributor

@Nerixyz Nerixyz commented Mar 23, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

This PR removes the manual theme creation and adds theme parsing from JSON (as mentioned in #4447 (comment), JSON is probably the best format for this). I converted the built-in themes to JSON files that are included in the final executable. The colors are ported 1:1, so there shouldn't be any visual difference. To keep the parsing code small, I'm using a macro that ensures the JSON keys are the same as the C++ property names.

This is working towards #2237. You can already specify custom themes in the settings, but there's neither UI nor auto-reloading nor a wiki entry for it.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

src/common/QLogging.cpp Show resolved Hide resolved
src/common/QLogging.cpp Show resolved Hide resolved
src/common/QLogging.hpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
src/singletons/Theme.cpp Show resolved Hide resolved
@PsycloneTM PsycloneTM mentioned this pull request Mar 31, 2023
1 task
Copy link
Member

@pajlada pajlada left a comment

Choose a reason for hiding this comment

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

Dark theme differences

old new
old new

This has to do with some ancient ifdefs https://github.com/Chatterino/chatterino2/blame/8064f8a49ebf72e9332c0208e3b5b9d54bf3ec4c/lib/appbase/BaseTheme.cpp#L65-L69

I'm ok to concede the window background color so it's the same on Windows & MacOS, but changed on Linux (for now).

LGTM 👍
Thank you

@pajlada
Copy link
Member

pajlada commented Apr 8, 2023

I'm also not super fond of the macro solution but I understand there would be some string duplication without it, so I'm fine to leave that to some potential cleanup PR in the future.

@pajlada pajlada enabled auto-merge (squash) April 8, 2023 08:37
@pajlada pajlada merged commit 4e3433e into Chatterino:master Apr 8, 2023
@Nerixyz Nerixyz deleted the refactor/theme-handling branch April 8, 2023 09:12
@badoge badoge mentioned this pull request Apr 10, 2023
4 tasks
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.

2 participants