lxqt / lxqt-qtplugin

LXQt Qt platform integration plugin
https://lxqt.github.io
GNU Lesser General Public License v2.1
24 stars 14 forks source link

Colors are messed up with Fusion and Qt 5.15 #54

Closed yan12125 closed 4 years ago

yan12125 commented 4 years ago
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

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 commented 4 years ago

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

yan12125 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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

tsujan commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Whoa! It works with 5.15! Nice feature!

tsujan commented 4 years ago

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 commented 4 years ago

I'll add this after https://github.com/lxqt/lxqt-qtplugin/pull/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 commented 4 years ago

@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 commented 4 years ago

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: https://github.com/lxqt/lxqt-config/pull/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 commented 4 years ago

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

jsm222 commented 3 years ago

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 commented 3 years ago

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

jsm222 commented 3 years ago

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

tsujan commented 3 years ago

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 commented 3 years ago

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

tsujan commented 3 years ago

https://github.com/lxqt/lxqt-qtplugin/commit/8cc32d94b4c9de74b5bcf27fae2d10e6b2b11caf is also relevant.

In fact, the latest git makes only these changes: https://github.com/lxqt/lxqt-qtplugin/compare/0.15.1...master