Bug 267770 - D-Bus-activated app widgets unthemed
Summary: D-Bus-activated app widgets unthemed
Status: REOPENED
Alias: None
Product: kdelibs
Classification: Frameworks and Libraries
Component: kdeui (show other bugs)
Version: 4.6
Platform: Fedora RPMs Linux
: NOR normal
Target Milestone: ---
Assignee: David Faure
URL:
Keywords:
: 268193 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-03-06 11:00 UTC by Kevin Kofler
Modified: 2020-01-15 10:32 UTC (History)
8 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.6.2


Attachments
Screenshot of the "ugly" plain old QT style (45.23 KB, image/png)
2011-03-09 18:53 UTC, tnemeth
Details
kdelibs-4.6.1-kde#267770.patch (1.08 KB, patch)
2011-03-11 05:33 UTC, Kevin Kofler
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Kofler 2011-03-06 11:00:48 UTC
Version:           unspecified (using KDE 4.6.1) 
OS:                Linux

As per https://bugzilla.redhat.com/show_bug.cgi?id=682300 :

--- Original report from Rex Dieter on 2011-03-04 14:38:50 EST: ---

Since kde-4.6.1, kpackagekitsmarticon update notifier is using ugly fallback qt (motify) theme, seemingly not using oxygen (via kde platform plugin).

--- Additional comment from Kevin Kofler on 2011-03-06 04:23:53 EST ---

I found the offending commit (to kde-workspace/kcontrol/krdb):
https://projects.kde.org/projects/kde/kdebase/kde-workspace/repository/revisions/9d7eb8f716873d2079039d336c8f9a21b2fae0fa
and followup:
https://projects.kde.org/projects/kde/kdebase/kde-workspace/repository/revisions/da97a77fcd01892a7e2a1a4e8e71ab94b2ece32f

This breaks because KPK is D-Bus-activated, so it gets D-Bus's environment, which doesn't have QT_PLUGIN_PATH set. It was previously getting the plugin path set from that krdb code.

I see 2 possible fixes:
A. Revert these changes, making sure we get the kconf_update stuff right.
B. Changing kpackagekitsmarticon to be a KApplication.

Note that only Qt-only apps which are D-Bus-activated are affected by the change.

--- Additional comment from Kevin Kofler on 2011-03-06 04:25:53 EST ---

On one hand, I feel tempted to do A because this is an incompatible change in a point release, but on the other hand, B is probably the better/upstreamable fix. kpackagekitsmarticon really shouldn't be Qt-only.

--- End quote ---

Reproducible: Always

Steps to Reproduce:
1. Wait for KPackageKit to check for updates.
2. Notice the D-Bus-activated kpackagekitsmarticon showing up.
3. Click on the system tray icon.

Actual Results:  
The GUI pops up in Qt's ugly default theme.

Expected Results:  
The GUI pops up in the theme selected in KDE, i.e. usually Oxygen.

This is a regression when updating the KDE Plasma workspace(s) from 4.6.0 to 4.6.1.
Comment 1 Kevin Kofler 2011-03-06 11:01:54 UTC
CCing dfaure because his krdb commit is triggering the regression.
Comment 2 Kevin Kofler 2011-03-06 21:31:53 UTC
Actually, kpackagekitsmarticon is already a KUniqueApplication (i.e. a KApplication subclass), yet we still end up with the theme not getting found.

KApplication doesn't set the plugin path, it expects it to be already set. It does try to set the default theme, but that doesn't help if Qt doesn't find it.

So there's no easy/good fix for this within KPackageKit, it needs to be fixed inside kdelibs (KApplication) or kde-workspace (krdb, by reverting the offending change).
Comment 3 tnemeth 2011-03-09 18:53:40 UTC
Created attachment 57811 [details]
Screenshot of the "ugly" plain old QT style

I can confirm the behavior. I previously thought it was an ubuntu specific bug. Here
is its link : https://bugs.launchpad.net/ubuntu/+source/kpackagekit/+bug/707916.
In case it's not readable here is what I wrote :

I also have a similar issue with maverick and the latest upgrade to KDE SC 4.6.1 a few
days ago. Here is what I observed :

When there are new upgrades available, the notification icon shows
up and I click on it. Quite usual.

However, when the application windows is displayed, it does not
take my (default) theme into account. It's displayed with the old
QT windows-like style.

   Launching kpackagekit from the menus : everything ok
   Launching it from the command line : ok
   Launching it from the command line with sudo : same bad display
   Launching it from the command line with kdesudo : ok
Comment 4 Kevin Kofler 2011-03-09 22:55:22 UTC
Can we add something like this to KApplicationPrivate::init in kdeui/kernel/kapplication.cpp?

/* Ensure the KDE Qt plugins are found.
   QT_PLUGIN_PATH is not set in all situations. */
QStringList pluginPaths = KGlobal::dirs()->resourceDirs(QLatin1String("qtplugins"));

// reverse plugin paths because QCoreApplication::addLibraryPath prepends
QStringList pluginPathsReversed:
foreach(const QString &pluginPath, pluginPaths)
  pluginPathsReversed.prepend(pluginPath);

foreach(const QString &pluginPath, pluginPathsReversed) {
  // prevent duplicates
  QCoreApplication::removeLibraryPath(pluginPath);

  // add the path (to the front)
  QCoreApplication::addLibraryPath(pluginPath);
}

I think that should fix the issue for KPackageKit, and any other kdelibs-using stuff getting activated through D-Bus activation. (It won't help for Qt-only stuff getting activated through D-Bus activation though, if there's any of that.) And yes, looking at the Qt code, it should be safe to call those 2 static QCoreApplication methods before the QCoreApplication is constructed.
Comment 5 Kevin Kofler 2011-03-11 05:33:08 UTC
Created attachment 57852 [details]
kdelibs-4.6.1-kde#267770.patch

This is the proposed patch we are now testing in Fedora. Compared to my previous proof-of-concept snippet, I fixed:
* a typo (':' instead of ';') which made the code fail to compile,
* incorrect use of QLatin1String for a method which expects a const char *,
* minor coding style nitpicks (but the coding style might still need tweaks, especially because the surrounding code has very inconsistent styles).
Comment 6 Kevin Kofler 2011-03-11 17:54:47 UTC
So we tested the patch, it doesn't work. :-( It has no visible effect.
Either more fixes or a different fix are needed.
Comment 7 Kevin Kofler 2011-03-11 18:20:07 UTC
So the reason this idea doesn't work is that KApplicationPrivate::init runs too late, only after QApplication is already fully initialized. (Grrr, I should have thought of that.)

One possible solution, though a bit hackish, would be to stuff the plugin code into KCmdLineArgs::init. That runs before QApplication::QApplication. But is it really where initialization code is supposed to sit?
Comment 8 David Faure 2011-03-11 23:14:47 UTC
Git commit 979a19afe93d1e4a5be684c1139b6ab242e6b9b6 by David Faure.
Committed on 11/03/2011 at 21:01.
Pushed by dfaure into branch 'KDE/4.6'.

Add paths to qt plugins (from kstandarddirs) before qapp is created.

Necessary for apps started through d-bus activation, so that the kde widget
style is loaded. The QCoreApp::instance test was "so that qt.conf is read" (db852684)
but the whole idea of 9d7eb8f7 is that this doesn't rely on the qt config files anymore.

FIXED-IN: 4.6.2
BUG: 267770

M  +1    -1    kdecore/kernel/kcomponentdata.cpp     
M  +2    -0    kdeui/kernel/kapplication.cpp     

http://commits.kde.org/kdelibs/979a19afe93d1e4a5be684c1139b6ab242e6b9b6
Comment 9 David Faure 2011-03-11 23:19:37 UTC
Git commit f1a81feb7851ceaf91d5bfe3ebdafb5f1052260a by David Faure.
Committed on 11/03/2011 at 21:01.
Pushed by dfaure into branch 'master'.

Add paths to qt plugins (from kstandarddirs) before qapp is created.

Necessary for apps started through d-bus activation, so that the kde widget
style is loaded. The QCoreApp::instance test was "so that qt.conf is read" (db852684)
but the whole idea of 9d7eb8f7 is that this doesn't rely on the qt config files anymore.

FIXED-IN: 4.6.2
BUG: 267770
(cherry picked from commit 979a19afe93d1e4a5be684c1139b6ab242e6b9b6)

M  +1    -1    kdecore/kernel/kcomponentdata.cpp     
M  +2    -0    kdeui/kernel/kapplication.cpp     

http://commits.kde.org/kdelibs/f1a81feb7851ceaf91d5bfe3ebdafb5f1052260a
Comment 10 Arno Rehn 2011-03-16 11:55:55 UTC
This doesn't fix it for me. I'm using kdelibs ata8cf52fa332293455a4a90080931e986ffc8c588 (which includes David's fix) but the telepathy-kde chat handler started is still using the wrong theme.

I've noticed that a bunch of env. variables isn't set for the dbus-launched apps, among them KDE_FULL_SESSION.
Unsetting this variable in a shell session will result in equally unthemed KDE apps.
Comment 11 Kevin Kofler 2011-03-16 11:57:53 UTC
The fix will only work for a KApplication. If your chat handler is a QApplication, you'll have to fix it to be a KApplication.
Comment 12 Arno Rehn 2011-03-16 12:01:26 UTC
It's a subclass of KApplication, so I think this should work.
Comment 13 Tom Kijas 2011-04-19 13:12:20 UTC
*** Bug 268193 has been marked as a duplicate of this bug. ***
Comment 14 Arno Rehn 2011-07-31 21:38:34 UTC
The issue doesn't seem to be fixed yet. We are experiencing it with the telepathy chat handler again (see Bug #269861 ).

I noticed that the problem only occurs when the environment of the process doesn't contain KDE_FULL_SESSION="true".
In startkde, that environment variable is only set *after* dbus is started, so consequently any app that is started by dbus will inherit it's environment and thus *not* have that env variable set.
I don't think we can rely on that env var being set before dbus is started, so any modification is startkde would only be a hack.
Comment 15 George Kiagiadakis 2011-07-31 23:10:44 UTC
I think Arno's claim is true. The only way for KDE applications to know that they run in a KDE environment is the KDE_FULL_SESSION environment variable, and since this is not set, it is reasonable (and correct) for them to be themed differently. If we hack KApplication so that it always loads the KDE style even if KDE_FULL_SESSION is not set, then we will get KDE themed apps on gnome or other environments (windows, osX...), which is not desirable.

I think the correct solution is to change startkde so that KDE_FULL_SESSION is set before dbus-daemon is started.