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

feat: Theme manager #587

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

Conversation

VasigaranAndAngel
Copy link
Collaborator

@VasigaranAndAngel VasigaranAndAngel commented Nov 16, 2024

Theme manager to the application, enabling support for customizable themes.

this theme manager will be easily accessible once the settings window and theme settings in it are implemented. until then, we'll need to manually edit the config.ini file to set theme settings.


to test:

  1. add new group and values as below in your config.ini:

    [Appearance]
    DarkMode=auto
    DarkThemeFile=path_to/dark_theme.ini
    LightThemeFile=path_to/light_theme.ini
    
    • DarkMode=auto will set the dark mode to system. available values: true, false and auto

  2. set up theme files:
    the theme files should be like below (in .ini format):

    [ColorRoleName]
    ColorGroupName = Color
    
    • ColorRoleName is the name of the color role (e.g. Window, Button, etc.)
    • ColorGroupName is the name of the color group (e.g. Active, Inactive or Disabled)
    • Color is the color value in the QColor supported format (e.g. #RRGGBB, blue, etc.)

    sample theme.ini i made this beautiful theme 😎
    [Button]
    Active=green
    Inactive=purple
    Disabled=#101010
    
    [Base]
    Active=cyan
    
    [Text]
    Active=black
    
    [Window]
    Active=orange
    
    available roles
    • WindowText
    • Button
    • Light
    • Midlight
    • Dark
    • Mid
    • Text
    • BrightText
    • ButtonText
    • Base
    • Window
    • Shadow
    • Highlight
    • HighlightedText
    • Link
    • LinkVisited
    • AlternateBase
    • NoRole
    • ToolTipBase
    • ToolTipText
    • PlaceholderText
    • Accent not sure if QPalette and/or widgets gets accent from system, even if this is set
    available groups
    • Active: if focused
    • Inactive: normal
    • Disabled: if disabled

tnx ❤️

@CyanVoxel CyanVoxel added Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience Priority: Low Doesn't require immediate attention Status: Review Needed A review of this is needed labels Nov 16, 2024
Copy link
Collaborator

@seakrueger seakrueger left a comment

Choose a reason for hiding this comment

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

Getting theme management and switching under control will be good to have, thanks. Unfortunately, currently, I'm unsure how to test this? I'd like to see the stock themes moved to the theme file format.
It also seems you are defining your own theming file format. Is there a reason/advantage to doing this rather than using QSS files (Qt Style Sheets, Qt's file format for theming)?

tagstudio/src/qt/main_window.py Show resolved Hide resolved
tagstudio/src/qt/qt_logger.py Outdated Show resolved Hide resolved
tagstudio/src/qt/theme.py Outdated Show resolved Hide resolved
tagstudio/src/qt/main_window.py Outdated Show resolved Hide resolved
Comment on lines +171 to +175
# the following signal emits when system theme (Dark, Light) changes (Not accent color).
QApplication.styleHints().colorSchemeChanged.connect(update_palette)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like to see this file moved into a ThemeManager class, rather than having this executed on import.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or should i just move this line into Ui_MainWindow?

@VasigaranAndAngel
Copy link
Collaborator Author

VasigaranAndAngel commented Nov 17, 2024

Unfortunately, currently, I'm unsure how to test this? I'd like to see the stock themes moved to the theme file format.

this theme manager will be useful once the settings window and theme settings in it is implemented. until then, we'll need to manually edit the config.ini file to set theme files.
explicitly setting a stock theme in Qt will make that theme the default. when saving theme colors to the theme.ini file, all default values will be removed because the QPalette is currently being used as the theme palette and most of the QPalette colors are not yet applied to any widgets.

It also seems you are defining your own theming file format. Is there a reason/advantage to doing this rather than using QSS files (Qt Style Sheets, Qt's file format for theming)?

if we use QSS, it will bring lot of customization options. but we would also need to ensure that every widget respects those customizations. to keep theme management as simple as possible, theme manager uses QSettings to save and load themes from .ini files and relies on QPalette to apply colors to widgets.

thank you so much for the reviews ❤️. i'll update and ping you for rereview.

@VasigaranAndAngel VasigaranAndAngel changed the title Theme manager feat: Theme manager Nov 17, 2024
@Computerdores
Copy link
Collaborator

I have been trying it out and seems to work well, however whenever I open TS the Appearance role disappears from the config and I have to add it again before starting TS for the Theme to work

@VasigaranAndAngel
Copy link
Collaborator Author

I have been trying it out and seems to work well, however whenever I open TS the Appearance role disappears from the config and I have to add it again before starting TS for the Theme to work

it's because opening library clears all settings. #622

@CyanVoxel CyanVoxel added Status: Changes Requested Changes are quested to this Status: Review Needed A review of this is needed and removed Status: Review Needed A review of this is needed labels Dec 7, 2024
@CyanVoxel
Copy link
Member

Could this be updated now that #622 is merged to make is easier to review?

VasigaranAndAngel and others added 5 commits December 7, 2024 23:57
main_window.py: put driver to application property and update theme palette.
qt_logger.py: contains a logger which will be used in ui related codes.
theme.py: New theme management system.
test_theme.py: tests for theme management system.
Co-authored-by: yed <yedpodtrzitko@users.noreply.github.com>
@CyanVoxel CyanVoxel removed the Status: Changes Requested Changes are quested to this label Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Low Doesn't require immediate attention Status: Review Needed A review of this is needed Type: Enhancement New feature or request Type: UI/UX User interface and/or user experience
Projects
Status: 🏓 Ready for Review
Development

Successfully merging this pull request may close these issues.

4 participants