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(client): Automatically sync theme with OS color scheme #6619

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

Conversation

jakub2682-tuke
Copy link

@jakub2682-tuke jakub2682-tuke commented Oct 28, 2024

This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades.

Implements #6515

Checks

This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades.

Implements mumble-voip#6515
This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades. -fix

Implements mumble-voip#6515
Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

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

I think this is on a good track, but you should reuse code that are already present in Mumble (Themes.cpp and LookConfig.cpp). See below:

@@ -9,3 +9,7 @@ qss_MAC=OSX Dark.qss
name=Lite
qss=Lite.qss
qss_MAC=OSX Lite.qss
[auto]
name=Auto
Copy link
Member

Choose a reason for hiding this comment

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

I guess you added this "theme" as a placeholder dummy, right? Take a look at this:

qcbTheme->clear();
qcbTheme->addItem(tr("None"));

There we add the "None" theme. We could just as easily add the "Auto" theme there, so we do not need to have a dummy theme in theme.ini

Copy link
Author

Choose a reason for hiding this comment

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

Originily i tried to add the option with "qcbTheme->addItem(tr("Auto"));" , but it didnt work right as i think it didnt save/load properly. I tried to fix it by changing LookConfig::reloadThemes ,LookConfig::load and LookConfig::save, but to no avail. So I had to resort to doing with putting fake theme in theme.ini .

Copy link
Member

Choose a reason for hiding this comment

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

You are right just adding qcbTheme->addItem(tr("Auto")); does not seem to be enough. However, from a maintainability point of view and general structure of this sort of this, I would insist that we solve this programmatically. I think that means modification to Themes::getConfiguredStyle and possibly a custom ThemeMap object.

(Please note that I do not have the time currently to properly 100% think this through, so that's also why I am very glad that someone else is taking this feature request 😅)

Copy link
Member

Choose a reason for hiding this comment

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

We should figure this out programmatically. If you are stuck here, let us know what is failing and we can figure out a way together.

src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
src/mumble/MainWindow.cpp Show resolved Hide resolved
src/mumble/MainWindow.cpp Outdated Show resolved Hide resolved
This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades.

Implements mumble-voip#6515
This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades.

Implements mumble-voip#6515
Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

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

P.S.: Feel free to request a new review from me directly using the Github UI, that way I will get notified when there is something new to review.

src/mumble/Themes.cpp Show resolved Hide resolved
src/mumble/Themes.cpp Outdated Show resolved Hide resolved
src/mumble/Themes.cpp Outdated Show resolved Hide resolved
src/mumble/Themes.cpp Outdated Show resolved Hide resolved
src/mumble/Themes.cpp Outdated Show resolved Hide resolved
@@ -9,3 +9,7 @@ qss_MAC=OSX Dark.qss
name=Lite
qss=Lite.qss
qss_MAC=OSX Lite.qss
[auto]
name=Auto
Copy link
Member

Choose a reason for hiding this comment

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

You are right just adding qcbTheme->addItem(tr("Auto")); does not seem to be enough. However, from a maintainability point of view and general structure of this sort of this, I would insist that we solve this programmatically. I think that means modification to Themes::getConfiguredStyle and possibly a custom ThemeMap object.

(Please note that I do not have the time currently to properly 100% think this through, so that's also why I am very glad that someone else is taking this feature request 😅)

@@ -8,6 +8,14 @@
#include "MumbleApplication.h"
#include "Global.h"

#include <QFile>
#include <QGuiApplication>
Copy link
Member

Choose a reason for hiding this comment

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

Please also double check (at least when we are done here), if all of those includes are actually needed.

Copy link
Author

Choose a reason for hiding this comment

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

I think only these includes are needed "QPalette, QSettings, QStyleHints", but not 100% sure.

This update introduces an automatic theme switch that changes based on the OS color scheme at startup and during runtime. This feature is implemented for Qt 6.2 and includes compatibility for Qt 6.5 once Mumble upgrades.

Implements mumble-voip#6515
boost::optional< ThemeInfo::StyleInfo > style = Themes::getConfiguredStyle(Global::get().s);
if (!style) {
return false;
}

const QFileInfo qssFile(style->getPlatformQss());
QString themePath = style->getPlatformQss().absoluteFilePath();
if (style->themeName == "Auto" || style->name == "Auto") {
Copy link
Member

Choose a reason for hiding this comment

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

Idea: If you move this to Themes::getConfiguredStyle, you will have access to the user setting strings. If you do this check with something like settings.themeName == "Auto", you could probably simply add the "Auto" to the ComboBox (further investigation required)

This would have the additional benefit of not having to hard-code the path, but rather - in the case of "Auto" - just selecting the appropriate ThemeMap with the name "Dark" or "Lite".

But if you run into any issues with this approach, be sure to let me know.

Copy link
Author

Choose a reason for hiding this comment

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

I tried that today, to no avail. I don't know if I implemented it wrong, in the wrong section, or it's something else. Furthermore, I have other tasks related to this git, but those only need to stay on my local machine, as well as other tasks from other subjects, so I don't know when I will be able to look at this specific issue again.

@Hartmnt Hartmnt added client feature-request This issue or PR deals with a new feature ui labels Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants