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

lxqtplatformtheme: Initialize "folowColorScheme" once #33

Closed palinek closed 6 years ago

palinek commented 6 years ago

follow up of #32

tsujan commented 6 years ago

@palinek I had tried such methods. All of them will purge all icons, perhaps, because it's too soon to call setFollowColorScheme.

tsujan commented 6 years ago

Oh, yes! To be certain, I tried this and it removed all my panel icons after logging in ;) pcmanfm-qt used the icon set I'd given to gtk apps.

palinek commented 6 years ago

Oh, yes! To be certain, I tried this and it removed all my panel icons after logging in ;) pcmanfm-qt used the icon set I'd given to gtk apps.

...that may be because of the QIconLoader logic:

  void QIconLoader::ensureInitialized()
  {
      if (!m_initialized) {
          m_initialized = true;

          Q_ASSERT(qApp);

          m_systemTheme = systemThemeName();

          if (m_systemTheme.isEmpty())
              m_systemTheme = fallbackTheme();
          if (qt_iconEngineFactoryLoader()->keyMap().key(QLatin1String("svg"), -1) != -1)
              m_supportsSvg = true;
      }
  }

...so we need to wait for the QApplication to be created first.

palinek commented 6 years ago

ignore the d8e2932... it's plain wrong because of the QThread etc. initialization and the QTimer...

tsujan commented 6 years ago

First of all, when I had different ideas, I really tested them. I forgot to tell you about this: QObject::startTimer: Timers can only be used with threads started with QThread.

Second, without testing, I'm 99% sure that this won't work.

tsujan commented 6 years ago

Oh, sorry, I didn't see your last comment! You realized that.

palinek commented 6 years ago

now it should work...

tsujan commented 6 years ago

I didn't test this but, out of curiosity, tested https://github.com/lxde/lxqt-qtplugin/commit/2283f5ee3c344fc62a74e41a127e3eee53fb2c37 (didn't change anything) with this:

void XdgIconLoader::setFollowColorScheme(bool enable)
{
    if (m_followColorScheme != enable)
    { qDebug("*******setFollowColorScheme********");
        QIconLoader::instance()->invalidateKey();
        m_followColorScheme = enable;
    }
}

For each app, the message *******setFollowColorScheme******** appeared just once.

So, the reasoning behind this can't be correct and so, this is an unnecessary complication in the code.

tsujan commented 6 years ago

In other words, IMHO, this a micro-optimization not worthy of a complication. It shouldn't make any difference in an app with 10000 icons or even more. (Tell me if you find such an app.)

EDIT: I had tested with adding/removing of a boolean check in one of my apps, where the check was done hundreds of thousands of times. No difference.

tsujan commented 6 years ago

I looked at this from the wrong side, having been misled by @palinek's wrong reason. Apart from that, It can also be seen as a logical replacement for the old QTimer::singleShot(0, this, SLOT(initWatch()));, which worked only "by chance".

So, GTM.

EDIT: The title is very misleading too.