From ffd76192f35ddc74fcd63fbe77203894b14068ad Mon Sep 17 00:00:00 2001 From: Nate Graham Date: Fri, 19 Aug 2022 11:06:23 -0600 Subject: [PATCH] applets/kicker: fix app icon loading logic to better handle relative paths ba44b69abf82b1236ffab7d8683728c142d30c52 added logic to handle apps that use an absolute path in their .desktop file to define their icon, which works. However in the process it introduced a subtle bug: if the icon is not an absolute path and it's just a normal icon name, when QFileInfo::exists() checks for the existence of that string, it will treat it as a relative file path and therefore look for it in the current working directory, which is typically the user's homedir. If it finds something, it will go down the wrong code path and end up returning a blank QIcon. This can be verified by adding a folder with the name of an app icon into ~ and restarting plasmashell; that app in Kickoff will have a blank icon. To fix this, the icon loading code now first checks whether the icon returned by m_service->icon() is actually an absolute path. If not, it skips the logic to look for it on disk and goes straight to the codepath that looks for an icon with that name in the icon theme. To minimize disk reads, it checks for absolute-file-path-ness by inspecting the string returned by m_service->icon() rather than using QFileInfo::isAbsolute(), because this is a hot code path and most icons will not have relative paths, so checking the disk for every one of them would be a waste of resources. BUG: 457965 FIXED-IN: 5.24.7 (cherry picked from commit 57d55e386a1f66390704c3a166d6c7fcc8120a7c) --- applets/kicker/plugin/appentry.cpp | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/applets/kicker/plugin/appentry.cpp b/applets/kicker/plugin/appentry.cpp index 3190c5f6d..6c5d6442a 100644 --- a/applets/kicker/plugin/appentry.cpp +++ b/applets/kicker/plugin/appentry.cpp @@ -145,10 +145,23 @@ bool AppEntry::isValid() const QIcon AppEntry::icon() const { if (m_icon.isNull()) { - if (QFileInfo::exists(m_service->icon())) { - m_icon = QIcon(m_service->icon()); + const QString serviceIcon = m_service->icon(); + + // Check for absolute-path-ness this way rather than using + // QFileInfo.isAbsolute() because that would perform a ton of unnecessary + // filesystem checks, and most icons are not defined in apps' desktop + // files with absolute paths. + bool isAbsoluteFilePath = serviceIcon.startsWith(QLatin1String("/")); + + // Need to first check for whether the icon has an absolute path, because + // otherwise if the icon is just a name, QFileInfo will treat it as a + // relative path and return true if there randomly happens to be a file + // with the name of an icon in the user's homedir and we'll go down the + // wrong codepath and end up with a blank QIcon; See 457965. + if (isAbsoluteFilePath && QFileInfo::exists(serviceIcon)) { + m_icon = QIcon(serviceIcon); } else { - m_icon = QIcon::fromTheme(m_service->icon(), QIcon::fromTheme(QStringLiteral("unknown"))); + m_icon = QIcon::fromTheme(serviceIcon, QIcon::fromTheme(QStringLiteral("unknown"))); } } return m_icon;