Bug 457965 - Creating a folder ~/[icon name] causes that the app using that icon to become blank in Kickoff after restarting plasmashell
Summary: Creating a folder ~/[icon name] causes that the app using that icon to become...
Status: RESOLVED FIXED
Alias: None
Product: plasmashell
Classification: Plasma
Component: Application Launcher (Kickoff) (show other bugs)
Version: master
Platform: Neon Linux
: NOR normal
Target Milestone: 1.0
Assignee: Plasma Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-08-16 15:36 UTC by Patrick Silva
Modified: 2022-08-22 16:20 UTC (History)
3 users (show)

See Also:
Latest Commit:
Version Fixed In: 5.24.7


Attachments
favorites list (122.89 KB, image/png)
2022-08-16 15:36 UTC, Patrick Silva
Details
internet submenu (140.50 KB, image/png)
2022-08-16 15:37 UTC, Patrick Silva
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick Silva 2022-08-16 15:36:41 UTC
Created attachment 151358 [details]
favorites list

STEPS TO REPRODUCE
1. install Firefox from distro repositories
2. add Firefox to Favorites list of kickoff
3. go to https://www.mozilla.org/en-US/firefox/new/ and download .tar.bz2 archive of firefox
4. extract the downloaded archive to Home
5. restart Plasma with 'plasmashell --replace', or logout and login
6. open kickoff and

OBSERVED RESULT
Icon of firefox disappeared from Favorites list and from Internet submenu.
Please see the attached screenshots.

EXPECTED RESULT
no icon should disappear after the provided steps

SOFTWARE/OS VERSIONS
Operating System: KDE neon Unstable Edition
KDE Plasma Version: 5.25.80
KDE Frameworks Version: 5.97.0
Qt Version: 5.15.5
Graphics Platform: Wayland
Comment 1 Patrick Silva 2022-08-16 15:37:17 UTC
Created attachment 151359 [details]
internet submenu
Comment 2 Nate Graham 2022-08-17 19:47:12 UTC
Did it install icons to ~/.local/share/icons?
Comment 3 Patrick Silva 2022-08-17 20:17:29 UTC
Possibly Firefox icons are installed to ~/.local/share/icons when I install Firefox from neon/ubuntu repos.
The icons reappear in my kickoff after deleting 'firefox' folder (created after extracting firefox-<version>.tar.bz2 archive) from Home and re-login.
Comment 4 Nate Graham 2022-08-17 20:26:55 UTC
That all makes sense. The XDG icon inheritance logic looks for icons in  your home folder before looking in /usr/share/icons. So it seems like the icons in your home folder got picked first, and for some reason they weren't able to be displayed. Can you attach the contents of ~/.local/share/icons/firefox?
Comment 5 Patrick Silva 2022-08-17 21:31:46 UTC
the directory does not exist

$ ls ~/.local/share/icons/firefox
ls: cannot access '/home/stalker/.local/share/icons/firefox': No such file or directory
Comment 6 Patrick Silva 2022-08-18 11:34:05 UTC
can also reproduce with smb4k

1. install smb4k and make sure its icon is present under 'Utilities' submenu of kickoff
2. create a folder called 'smb4k' in Home
3. restart Plasma by running 'plasmashell --replace' with krunner

Result: icon of smb4k disappeared from kickoff
Comment 7 Patrick Silva 2022-08-18 11:42:06 UTC
same with okular

1. install Okular and make sure its icon is present in kickoff
2. create a folder called 'okular' in Home
3. restart Plasma by running 'plasmashell --replace' with krunner

Result: icon of Okular disappeared from kickoff
Comment 8 Nate Graham 2022-08-18 15:51:53 UTC
Wow, I can reproduce for all apps.
Comment 9 Nate Graham 2022-08-18 16:00:37 UTC
Wow, crazy. Can reproduce 100% in Kickoff for all app names I try. For example creating ~/system-file-manager makes Dolphin's icon in Kickoff become blank.
Comment 10 Nate Graham 2022-08-19 16:34:56 UTC
Found it. In the Kicker model, appentry.cpp has this:

QIcon AppEntry::icon() const
{
    if (m_icon.isNull()) {
        if (QFileInfo::exists(m_service->icon())) {
            m_icon = QIcon(m_service->icon());
        } else {
            m_icon = QIcon::fromTheme(m_service->icon(), QIcon::fromTheme(QStringLiteral("unknown")));
        }
    }
    return m_icon;
}

if (QFileInfo::exists(m_service->icon())) { is returning true here. If I comment all of that out, the problem stops happening. This code seems weird. I hven't figured out its purpose yet.
Comment 11 Bug Janitor Service 2022-08-19 17:23:47 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/plasma-workspace/-/merge_requests/2034
Comment 12 Nate Graham 2022-08-22 16:15:52 UTC
Git commit 57d55e386a1f66390704c3a166d6c7fcc8120a7c by Nate Graham.
Committed on 22/08/2022 at 16:06.
Pushed by ngraham into branch 'master'.

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.
FIXED-IN: 5.24.7

M  +16   -3    applets/kicker/plugin/appentry.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/57d55e386a1f66390704c3a166d6c7fcc8120a7c
Comment 13 Nate Graham 2022-08-22 16:20:08 UTC
Git commit 1a74e6237062e5c6d583f0afcd53d21bef3ffd98 by Nate Graham.
Committed on 22/08/2022 at 16:20.
Pushed by ngraham into branch 'cherry-pick-57d55e38'.

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.
FIXED-IN: 5.24.7


(cherry picked from commit 57d55e386a1f66390704c3a166d6c7fcc8120a7c)

M  +16   -3    applets/kicker/plugin/appentry.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/1a74e6237062e5c6d583f0afcd53d21bef3ffd98
Comment 14 Nate Graham 2022-08-22 16:20:36 UTC
Git commit ffd76192f35ddc74fcd63fbe77203894b14068ad by Nate Graham.
Committed on 22/08/2022 at 16:20.
Pushed by ngraham into branch 'Plasma/5.25'.

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.
FIXED-IN: 5.24.7


(cherry picked from commit 57d55e386a1f66390704c3a166d6c7fcc8120a7c)

M  +16   -3    applets/kicker/plugin/appentry.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/ffd76192f35ddc74fcd63fbe77203894b14068ad
Comment 15 Nate Graham 2022-08-22 16:20:51 UTC
Git commit f0a3a400d63939e70004e6df76536ce538b428c3 by Nate Graham.
Committed on 22/08/2022 at 16:20.
Pushed by ngraham into branch 'Plasma/5.24'.

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.
FIXED-IN: 5.24.7


(cherry picked from commit 57d55e386a1f66390704c3a166d6c7fcc8120a7c)

M  +16   -3    applets/kicker/plugin/appentry.cpp

https://invent.kde.org/plasma/plasma-workspace/commit/f0a3a400d63939e70004e6df76536ce538b428c3