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

Colors are messed up with Fusion and Qt 5.15 #54

Closed
yan12125 opened this issue May 27, 2020 · 25 comments · Fixed by #55
Closed

Colors are messed up with Fusion and Qt 5.15 #54

yan12125 opened this issue May 27, 2020 · 25 comments · Fixed by #55

Comments

@yan12125
Copy link
Member

Expected Behavior

Color for general Qt widgets are close to white with Fusion. For example here is a screenshot without lxqt-qtplugin

image

Current Behavior

image

Possible Solution

To be investigated

Steps to Reproduce (for bugs)
  1. Enable testing repos on Arch Linux and upgrade
Context

Texts are harder to read

System Information
  • Distribution & Version: Arch Linux
  • Kernel: shoud not be relevant
  • Qt Version: 5.15.0
  • libqtxdg Version: 3.5.0
  • lxqt-build-tools Version: 0.7.0
  • Package version: 0.15.0

Note that on the machine I'm testing, I use stable versions for LXQt instead of -git ones. If this is already fixed in git, please tell me and I will backport necessary commits to Arch packages.

@tsujan
Copy link
Member

tsujan commented May 27, 2020

Did you recompile the important components after upgrading to Qt 5.15.0?

@yan12125
Copy link
Member Author

Yep, libqtxdg, libfm-qt and lxqt-qtplugin are recompiled.

Also, I found that after applying the following patch, Fusion works again

diff --git a/src/lxqtplatformtheme.cpp b/src/lxqtplatformtheme.cpp
index c3da96b..c41a0ad 100644
--- a/src/lxqtplatformtheme.cpp
+++ b/src/lxqtplatformtheme.cpp
@@ -258,10 +258,6 @@ QPlatformDialogHelper *LXQtPlatformTheme::createPlatformDialogHelper(DialogType
     return nullptr;
 }
 
-const QPalette *LXQtPlatformTheme::palette(Palette /*type*/) const {
-    return nullptr;
-}
-
 const QFont *LXQtPlatformTheme::font(Font type) const {
     if(type == SystemFont && !fontStr_.isEmpty()) {
         // NOTE: for some reason, this is not called by Qt when program startup.
diff --git a/src/lxqtplatformtheme.h b/src/lxqtplatformtheme.h
index 974e1bf..c5c1e89 100644
--- a/src/lxqtplatformtheme.h
+++ b/src/lxqtplatformtheme.h
@@ -50,8 +50,6 @@ public:
     bool usePlatformNativeDialog(DialogType type) const override;
     QPlatformDialogHelper *createPlatformDialogHelper(DialogType type) const override;
 
-    const QPalette *palette(Palette type = SystemPalette) const override;
-
     const QFont *font(Font type = SystemFont) const override;
 
     QVariant themeHint(ThemeHint hint) const override;

This effectively reverts #22, so Breeze is broken again...

@tsujan
Copy link
Member

tsujan commented May 27, 2020

IMO, we should be cautious here, especially with regard to Qt < 5.15.

I have Manjaro Testing, which will get Qt 5.15 from Arch Testing. Please wait until Qt 5.15 comes here. Qt 5.14.0 and 5.13.0 had serious bugs, which were fixed in their next patch versions; I hope that isn't the case with Qt 5.15.0 — otherwise, it'll give me a headache with Kvantum too.

@Chiitoo
Copy link

Chiitoo commented May 27, 2020

I first noticed this around 2020-01-22 with Qt 5.15... but never got to bisecting it. :\

I have asked about it on a Qt IRC channel, but didn't get a reply, and didn't get to filing a bug report still.

I guess I got too used to it... heh.

@tsujan
Copy link
Member

tsujan commented May 27, 2020

qfusionstyle.cpp has changed a little but the changes aren't related to its background color.

qplatformtheme.cpp hasn't changed, except for 0nullptr.

We might need to search for the needle in the haystack.

@tsujan
Copy link
Member

tsujan commented May 27, 2020

I think I've found the cause in qapplication.cpp:

static void initSystemPalette() is removed from that file and this method is added (I've removed its comments):

QPalette QApplicationPrivate::basePalette() const
{
    QPalette palette = app_style ? app_style->standardPalette() : Qt::gray;
    if (const QPalette *themePalette = platformTheme() ? platformTheme()->palette() : nullptr)
        palette = themePalette->resolve(palette);
    return palette;
}

As a result, Qt::gray ("#a0a0a4) is what you see with Fusion.

Still don't know what we can do....

@tsujan
Copy link
Member

tsujan commented May 27, 2020

@yan12125
A shot in the dark: What about adding a private variable QPalette *LXQtPalette_;, adding it to LXQtPlatformTheme c-tor:

LXQtPlatformTheme::LXQtPlatformTheme():
    iconFollowColorScheme_(true)
    , settingsWatcher_(nullptr)
    , LXQtPalette_(new QPalette)

deleting it in d-tor:

LXQtPlatformTheme::~LXQtPlatformTheme() {
    delete LXQtPalette_;
    if(settingsWatcher_)
        delete settingsWatcher_;
}

and using it as:

const QPalette *LXQtPlatformTheme::palette(Palette type) const {
    if(type == QPlatformTheme::SystemPalette) {
        return LXQtPalette_;
    }
    return nullptr;
}

EDIT: Corrected the code. It works with Qt 5.14.

@Chiitoo
Copy link

Chiitoo commented May 27, 2020

Indeed, looks like it starts with 0a93db4d [1].

I'd report it to Qt, but if you're fixing it here, and issues are not popping up elsewhere, I guess I'll rather not.

  1. https://code.qt.io/cgit/qt/qtbase.git/commit/src/widgets/kernel/qapplication.cpp?h=5.15&id=0a93db4d82c051164923a10e4382b12de9049b45

@yan12125
Copy link
Member Author

Still get gray backgrounds with LXQtPalette_ on 5.15. I guess palletes are broken :/

@tsujan
Copy link
Member

tsujan commented May 28, 2020

@yan12125
It may be a matter of timing. What about having the variable as mutable QPalette *LXQtPalette_; and:

LXQtPlatformTheme::LXQtPlatformTheme():
    iconFollowColorScheme_(true)
    , settingsWatcher_(nullptr)
    , LXQtPalette_(nullptr)
…
…
…
LXQtPlatformTheme::~LXQtPlatformTheme() {
    if(LXQtPalette_)
        delete LXQtPalette_;
    if(settingsWatcher_)
        delete settingsWatcher_;
}
…
…
…
const QPalette *LXQtPlatformTheme::palette(Palette type) const {
    if(type == QPlatformTheme::SystemPalette) {
        if(LXQtPalette_ == nullptr) {
            LXQtPalette_ = new QPalette; // use the default palette
        }
        return LXQtPalette_;
    }
    return nullptr;
}

This was my last shot in the dark until I get Qt 5.15; sorry to bother you with them!

@yan12125
Copy link
Member Author

Still no luck - getting gray backgrounds.

I tried playing with QStyleFactory, too, and get no good results on 5.15. Here is my lastest attempt:

diff --git a/src/lxqtplatformtheme.cpp b/src/lxqtplatformtheme.cpp
index c3da96b..a207c52 100644
--- a/src/lxqtplatformtheme.cpp
+++ b/src/lxqtplatformtheme.cpp
@@ -46,6 +46,7 @@
 #include <QStyle>
 #include <private/xdgiconloader/xdgiconloader_p.h>
 #include <QLibrary>
+#include <QStyleFactory>
 
 
 // Function to create a new Fm::FileDialogHelper object.
@@ -86,6 +87,10 @@ void LXQtPlatformTheme::lazyInit()
     connect(settingsWatcher_, &QFileSystemWatcher::fileChanged, this, &LXQtPlatformTheme::onSettingsChanged);
 
     XdgIconLoader::instance()->setFollowColorScheme(iconFollowColorScheme_);
+
+    // LXQtPalette_ cannot be initialized in the constructor as some style
+    // plugins (e.g., breeze) needs QGuiApplication context
+    initPalette();
 }
 
 void LXQtPlatformTheme::loadSettings() {
@@ -118,6 +123,9 @@ void LXQtPlatformTheme::loadSettings() {
 
     // widget style
     style_ = settings.value(QLatin1String("style"), QLatin1String("fusion")).toString();
+    if (qEnvironmentVariableIsSet("QT_STYLE_OVERRIDE")) {
+        style_ = QString::fromLocal8Bit(qgetenv("QT_STYLE_OVERRIDE"));
+    }
 
     // SystemFont
     fontStr_ = settings.value(QLatin1String("font")).toString();
@@ -175,6 +183,7 @@ void LXQtPlatformTheme::onSettingsChanged() {
 
     if(style_ != oldStyle) // the widget style is changed
     {
+        initPalette();
         // ask Qt5 to apply the new style
         if (qobject_cast<QApplication *>(QCoreApplication::instance()))
             QApplication::setStyle(style_);
@@ -258,7 +267,19 @@ QPlatformDialogHelper *LXQtPlatformTheme::createPlatformDialogHelper(DialogType
     return nullptr;
 }
 
-const QPalette *LXQtPlatformTheme::palette(Palette /*type*/) const {
+
+void LXQtPlatformTheme::initPalette() {
+    QStyle *q_style = QStyleFactory::create(style_);
+    if (q_style) {
+        LXQtPalette_ = q_style->standardPalette();
+    }
+    delete q_style;
+}
+
+const QPalette *LXQtPlatformTheme::palette(Palette type) const {
+    if(type == QPlatformTheme::SystemPalette) {
+        return &LXQtPalette_;
+    }
     return nullptr;
 }
 
diff --git a/src/lxqtplatformtheme.h b/src/lxqtplatformtheme.h
index 974e1bf..a1ed3a2 100644
--- a/src/lxqtplatformtheme.h
+++ b/src/lxqtplatformtheme.h
@@ -83,6 +83,7 @@ public Q_SLOTS:
 
 private:
     void loadSettings();
+    void initPalette();
 
 private Q_SLOTS:
     void onSettingsChanged();
@@ -101,6 +102,7 @@ private:
     QFont font_;
     QString fixedFontStr_;
     QFont fixedFont_;
+    QPalette LXQtPalette_;
     // mouse
     QVariant doubleClickInterval_;
     QVariant wheelScrollLines_;

By the way, I'm happy to test patches :)

@tsujan
Copy link
Member

tsujan commented May 28, 2020

Still no luck - getting gray backgrounds.

Thanks! It seems that we can't get rid of that damn Qt::gray so easily. Once again, Qt5 devs didn't care about breaking people's codes :(

I'll do more tests when Qt 5.15 comes here. If you find a backward-compatible workaround, don't hesitate to make a PR; I can test its backward-compatibility until then.

@tsujan
Copy link
Member

tsujan commented May 28, 2020

@yan12125
With this simple patch, the main color (= window color = button color) of Fusion's palette can be set as the value of window_color in lxqt.conf, with "#efefef" as the fallback (Fusion's default color):

diff -ruNp lxqt-qtplugin-orig/src/lxqtplatformtheme.cpp lxqt-qtplugin/src/lxqtplatformtheme.cpp
--- lxqt-qtplugin-orig/src/lxqtplatformtheme.cpp	2020-03-27 19:21:14.000000000 +0430
+++ lxqt-qtplugin/src/lxqtplatformtheme.cpp	2020-05-29 01:45:11.427981861 +0430
@@ -57,6 +57,7 @@ static CreateFileDialogHelperFunc create
 LXQtPlatformTheme::LXQtPlatformTheme():
     iconFollowColorScheme_(true)
     , settingsWatcher_(nullptr)
+    , LXQtPalette_(nullptr)
 {
     loadSettings();
     // Note: When the plugin is loaded, it seems that the app is not yet running and
@@ -75,6 +76,8 @@ LXQtPlatformTheme::LXQtPlatformTheme():
 }
 
 LXQtPlatformTheme::~LXQtPlatformTheme() {
+    if(LXQtPalette_)
+        delete LXQtPalette_;
     if(settingsWatcher_)
         delete settingsWatcher_;
 }
@@ -96,6 +99,14 @@ void LXQtPlatformTheme::loadSettings() {
     QSettings settings(QSettings::UserScope, QLatin1String("lxqt"), QLatin1String("lxqt"));
     settingsFile_ = settings.fileName();
 
+    // TODO: Qt 5.15 enforces Qt::gray. Later, we could have a completely configurable palette
+    // but, for now, only the window (button) color is set, with Fusion's window color as the fallback.
+    QColor winColor;
+    winColor.setNamedColor(settings.value(QLatin1String("window_color"), QLatin1String("#efefef")).toString());
+    if(!winColor.isValid())
+        winColor.setNamedColor(QStringLiteral("#efefef"));
+    LXQtPalette_ = new QPalette(winColor);
+
     // icon theme
     iconTheme_ = settings.value(QLatin1String("icon_theme"), QLatin1String("oxygen")).toString();
     iconFollowColorScheme_ = settings.value(QLatin1String("icon_follow_color_scheme"), iconFollowColorScheme_).toBool();
@@ -258,7 +269,11 @@ QPlatformDialogHelper *LXQtPlatformTheme
     return nullptr;
 }
 
-const QPalette *LXQtPlatformTheme::palette(Palette /*type*/) const {
+const QPalette *LXQtPlatformTheme::palette(Palette type) const {
+    if(type == QPlatformTheme::SystemPalette) {
+        if(LXQtPalette_)
+            return LXQtPalette_;
+    }
     return nullptr;
 }
 
@@ -356,7 +371,7 @@ QIconEngine *LXQtPlatformTheme::createIc
 {
     return new XdgIconLoaderEngine(iconName);
 }
- 
+
 // Helper to return the icon theme paths from XDG.
 QStringList LXQtPlatformTheme::xdgIconThemePaths() const
 {
diff -ruNp lxqt-qtplugin-orig/src/lxqtplatformtheme.h lxqt-qtplugin/src/lxqtplatformtheme.h
--- lxqt-qtplugin-orig/src/lxqtplatformtheme.h	2020-03-27 19:21:14.000000000 +0430
+++ lxqt-qtplugin/src/lxqtplatformtheme.h	2020-05-28 11:24:15.000000000 +0430
@@ -109,6 +109,8 @@ private:
     QFileSystemWatcher *settingsWatcher_;
     QString settingsFile_;
 
+    QPalette *LXQtPalette_;
+
     QStringList xdgIconThemePaths() const;
 };
 

It works well with Qt < 5.15. For example, here I set it to "#323232" (of course, manually — there's no GUI yet):

fusion

Would you please test it with Qt 5.15? If it works with 5.15 too, LXQt users could change Fusion's main color — a feature that had been requested a few times if I remember correctly. As is mentioned in the code comment, we could support a more complete palette but, IMHO, this is enough for a simple style like Fusion.

EDIT - Good news: Breeze follows it too:

breeze

I'm sure LXQt users have told us several times that they want a way of changing Breeze's colors without KDE. This is a minimal way of doing so.

Kvantum sets its palette independently — under KDE too — because it's SVG-based.

@yan12125
Copy link
Member Author

Whoa! It works with 5.15! Nice feature!

@tsujan
Copy link
Member

tsujan commented May 29, 2020

It works with 5.15

Thanks! That was good news :) I'll make a PR soon. We can have the option in LXQt Appearance Configuration → Widget Style

tsujan added a commit that referenced this issue May 29, 2020
Only the window (= button) color can be configured for now — `QPalette::QPalette(const QColor&)` calculates the other colors automatically — but, if needed, more customization could be added later, although, IMHO, it is better not to confuse users with too many colors.

The default color is that of Fusion's window.

If an option is added to `lxqt-config`, users could change not only Fusion's look-and-feel but also Breeze's, without needing KDE's systemsettings5.

Note 1: This was started to compensate for the ugly color Qt 5.15 gave to Fusion. IMO, that's a Qt issue but we don't need to worry about it.

Note 2: I'll make another PR for `lxqt-config` after the current one is decided.

Closes #54
@tsujan
Copy link
Member

tsujan commented May 29, 2020

I'll add this after #55 is merged:

window_color

The other colors will be calculated (by Qt) based on the window color but, later, we might want to add more colors.

@ghost
Copy link

ghost commented May 31, 2020

@tsujan I'm importing 0.15.0 for NetBSD tomorrow. Are there any planned point releases after this one in the coming days/weeks?

I'lI be maintaining LXQt on NetBSD.

@tsujan
Copy link
Member

tsujan commented May 31, 2020

Are there any planned point releases after this one in the coming days/weeks?

lxqt-qtplugin 0.15.1 was released minutes ago. It makes palette change possible.

The patch for lxqt-config (corresponding to the above screenshot) is here: lxqt/lxqt-config#629. I think it will be in LXQt 0.16.0 (not soon) because point (=patch) releases are for critical bugs/workarounds, as was mentioned by @luis-pereira elsewhere and I agree with him.

However, if LXQt members tell me that an exception should be made for lxqt-config, I'll have no objection.

@ghost
Copy link

ghost commented May 31, 2020

Thx. I'll update lxqt-qtplugin and push the full DE to the repos tomorrow.

@jsm222
Copy link

jsm222 commented Jul 13, 2020

I still get color 0x000080 and not 0x308cc6 as highlight color. Fusion + Openbox.
The background is correct though (0xefefef). I am using lxqt-qtplugin 0.15.1 and qt 5.15 .0 on FreeBSD..

@tsujan
Copy link
Member

tsujan commented Jul 13, 2020

@jsm222
Workarounds for Fusion's window and highlight colors with Qt 5.15 are already in git LXQt (lxqt-qtplugin).

@jsm222
Copy link

jsm222 commented Jul 13, 2020

oh I missed that, is there plans to make another point release?
If not I might downstream the patch to 0.15.1

@tsujan
Copy link
Member

tsujan commented Jul 13, 2020

is there plans to make another point release?

I think all LXQt devs tacitly agreed that there was no need to a point release because it wasn't our bug and it isn't a critical Qt bug either (it can be seen as a new choice in Qt, although maybe a bad one).

@jsm222
Copy link

jsm222 commented Jul 13, 2020

Okay thanks I just took https://github.com/lxqt/lxqt-qtplugin/commit/aa3c3eb521f3d8c70ada308c43ee772f1af8738f.patch into downstream, and it works.

@tsujan
Copy link
Member

tsujan commented Jul 13, 2020

8cc32d9 is also relevant.

In fact, the latest git makes only these changes: 0.15.1...master

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 a pull request may close this issue.

4 participants