Fixed mistakes in setting window color

[tsujan]

Three mistakes are corrected in the previous commit (https://github.com/lxqt/lxqt-qtplugin/commit/8a5a1999762c4c020e1cf3939d6eb6acf84b9a71), the first one being a serious bug. The fixes are as follows:

1. LXQtPlatformTheme::loadSettings() may be called multiple times due to changes in ~/.config/lxqt/lxqt.conf. Therefore, LXQtPalette_ should be deleted before being created again; otherwise, a memory leak will exist.

2. The new key window_color should belong to the "Qt" group because it is about a Qt setting.

3. If the value of window_color is changed, the application should be restyled; otherwise, the color change won't have effect immediately and the application should be restarted to reflect it.

[tsujan]

@yan12125 It seems that we should be a little more patient. We focused on fixing a problem so much that we forgot about other things.

Please test this PR with Qt 5.15 in the following way:

1. Add window_color=#c3c3c3 to the "[Qt]" section of ~/.config/lxqt/lxqt.conf manually and save the file. Then, the change should be reflected by the running Qt apps, provided that they're styled by Fusion (or Breeze). In other words, no app restart should be needed.

2. Remove that line and save the file again. Then the color should change to Fusion's default color in the running Qt apps.

Please tell me whether the results are OK. They are OK here, with Qt 5.14.

@luis-pereira Would you please also take a look at the changes? Hidden issues had been found and fixed in this file a few times; apparently, it attracts mistakes ;)

[yan12125]

Hmm, on 5.15 colors do not change until apps are restarted.

[yan12125]

Tried a bit more and found that on 5.15, LXQtPlatformTheme::palette() is not called after changing lxqt.conf. I managed to get dynamic palette working with:

diff --git a/src/lxqtplatformtheme.cpp b/src/lxqtplatformtheme.cpp
index 9c34763..4a163b7 100644
--- a/src/lxqtplatformtheme.cpp
+++ b/src/lxqtplatformtheme.cpp
@@ -194,8 +194,10 @@ void LXQtPlatformTheme::onSettingsChanged() {
     if(style_ != oldStyle || oldWinColor != winColor_) // the widget style or window color is changed
     {
         // ask Qt5 to apply the new style
-        if (qobject_cast<QApplication *>(QCoreApplication::instance()))
+        if (qobject_cast<QApplication *>(QCoreApplication::instance())) {
             QApplication::setStyle(style_);
+            QApplication::setPalette(*LXQtPalette_);
+        }
     }

     if(iconTheme_ != oldIconTheme) { // the icon theme is changed

Another lesson learned: by default, neovim creates a new file (inode) for modifications instead overwriting the existing file. I need to add set backupcopy=yes to my init.vim so that changes to lxqt.conf can be observed.

[tsujan]

@yan12125 Thanks for investigating it.

neovim creates a new file

So, it was a problem in neovim and QApplication::setPalette was not needed with Qt 5.15 (as it isn't with Qt < 5.15)?

I used FeatherPad. When FeatherPad saves something, it really saves it ;)

[yan12125]

Well, with fixed neovim, QApplication::setPalette is still needed.

[tsujan]

Well, with fixed neovim, QApplication::setPalette is still needed.

OK. Then, the changes in Qt 5.15 are more or deeper than I thought. Whether they were all intentional and good or some of them are side-effects of other changes, I don't know yet.

I'm afraid the release of lxqt-qtplugin 0.15.1 needs to be delayed a little. The good thing is that you've found the solution.

[yan12125]

No hurry :)

tsujan notifications@github.com 於 2020年5月30日 週六 下午9:12寫道：

Well, with fixed neovim, QApplication::setPalette is still needed.

OK. Then, the changes in Qt 5.15 are more or deeper than I thought. Whether they were all intentional and good or some of them are side-effects of other changes, I don't know yet.

I'm afraid the release of lxqt-qtplugin 0.15.1 needs to be delayed a little. The good thing is that you've found the solution.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lxqt/lxqt-qtplugin/pull/56#issuecomment-636329198, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAOZCGOZWGDUJXE2KXRWRA3RUEA4HANCNFSM4NOLXTVQ .

[tsujan]

Unfortunately, text colors can be wrong in some widgets after restyling or resetting the palette. I've seen that in KDE too, especially when switching between dark and light themes. An app restart corrects all colors.

Therefore, users need to be informed about that in the future patch for lxqt-config — as a tooltip or in some other way.

EDIT: I found a way to overcome the above-mentioned problem :)

[tsujan]

@yan12125 I applied your change and also enforced palette change on all widgets, so that, now, the palette is updated everywhere when switching between dark and light colors. Everything is fine with Qt<5.15.

Although you had tested your fix with Qt 5.15, would you please test the updated patch again? Who knows what the next magic could be? ;)

[yan12125]

I can confirm with the latest patchset, dynamic switching of palettes also works on Qt 5.15 :tada: :tada: :tada:

[tsujan]

Thanks for the test and, most importantly, for the patch for Qt 5.15!

I'll merge it tomorrow if there's no objection and then, start releasing of 0.15.1.

---

BTW, QApplication::setStyle has changed in this way:

The following lines are removed (I've removed their comments):

    if (QApplicationPrivate::set_pal) {
        QApplication::setPalette(*QApplicationPrivate::set_pal);
    } else if (QApplicationPrivate::sys_pal) {
        clearSystemPalette();
        initSystemPalette();
        QApplicationPrivate::initializeWidgetFontHash();
    } else if (!QApplicationPrivate::sys_pal) {
        QApplicationPrivate::setSystemPalette(QApplicationPrivate::app_style->standardPalette());
    }

and these lines are added instead:

    if (testAttribute(Qt::AA_SetPalette))
        QApplicationPrivate::app_style->polish(*QGuiApplicationPrivate::app_pal);
    else
        QApplicationPrivate::initializeWidgetPalettesFromTheme();
    QApplicationPrivate::initializeWidgetFontHash();

That's why we needed to set the palette ourself, which is safe with Qt < 5.15 too. IMO, it's about a side-effect (= bug) in 5.15.

[tsujan]

@yan12125 Setting the palette had one side-effect: Desktop text color is also changed. With a light theme, the user may have set the desktop text color to white (because of a dark wallpaper) but it will change to black after applying style changes. Restarting pcmanfm-qt fixes the problem, of course.

I'll see if the problem can be solved in pcmanfm-qt itself.

[tsujan]

While the solution for pcmanfm-qt is simple, there are three problems here:

1. On the one hand, if we don't set the widget palettes to the app palette, the text colors of some widgets (like the side-pane of libfm-qt) won't be updated on switching between dark and light colors.

2. On the other hand, if a widget has an intentionally different palette (like pcmanfm-qt's desktop view), forcing the app palette on it will remove its custom colors.

3. In addition, there's a small problem in Qt: When calculating other colors based on the window color, Qt creates a highlight color (very dark blue) different from that of Fusion (blue but not so dark). Only if the app is restarted, the highlight color will change to Fusion's.

[yan12125]

Let's continue the discussion on https://github.com/lxqt/lxqt-config/pull/629 :)