lxqt / libqtxdg

Qt implementation of freedesktop.org xdg specs
https://lxqt.github.io
GNU Lesser General Public License v2.1
72 stars 35 forks source link

Fixed can't get icon when installing wine app #282

Closed Dami-star closed 2 years ago

Dami-star commented 2 years ago

When cache->isValid() returns true, cache->reValid(true) will no longer be executed, resulting in gtkCachesWatcher->addPath no longer adding monitoring files, which cannot be monitored when icon-theme.cache changes.

Even with 3.3.1, this fix still solves the problem

Dami-star commented 2 years ago

I just added my problem reproduction environment in #260

Dami-star commented 2 years ago

I'm afraid this defeats the purpose of having a cache.

Also, see #255 (review) and #226 Hi tsujan, thank you for your reply, I have spent a few days debugging this problem, I also refer to #255, my latest is to repeatedly install and uninstall multiple wine programs, and there will be a failure to get the icon. Especially when repeatedly When uninstalling and then installing.

try adding debug info here to identify the problem:

qDebug() << "QIconCacheGtkReader::reValid, path :" << gtkCachesWatcher->files()<<m_cacheFileInfo.absoluteFilePath());
if (!gtkCachesWatcher->files().contains(m_cacheFileInfo.absoluteFilePath()))
    gtkCachesWatcher->addPath(m_cacheFileInfo.absoluteFilePath());
Dami-star commented 2 years ago

Or try to roll back the commit of #261, test and reproduce the problem, and then just use #282 to solve all problems

tsujan commented 2 years ago

Or try to roll back the commit of https://github.com/lxqt/libqtxdg/pull/261,..

Contrary to some comments, https://github.com/lxqt/libqtxdg/pull/261 had nothing to do with this. It was about consistency in treating dash fallbacks.

Yes, this PR avoids the problem by not using the cache as it's supposed to be used, but in that case, the removal of the cache would be a cleaner "solution". Neither is a real solution (→ https://github.com/lxqt/libqtxdg/issues/226).

Dami-star commented 2 years ago

Or try to roll back the commit of #261,..

Contrary to some comments, #261 had nothing to do with this. It was about consistency in treating dash fallbacks.

Yes, this PR avoids the problem by not using the cache as it's supposed to be used, but in that case, the removal of the cache would be a cleaner "solution". Neither is a real solution (→ #226).

Thank you, do you have any good suggestions for changes to delete the cache

tsujan commented 2 years ago

do you have any good suggestions for changes to delete the cache

I didn't mean deleting the cache but ignoring it. Just remove it from XdgIconLoader::findIconHelper.

But I emphasize again: Without the cache, each icon should be found in the directory hierarchies of icon sets, and that takes more CPU time. It's not disastrous — you might not notice a difference with your computer — but it isn't good for all.

Dami-star commented 2 years ago

do you have any good suggestions for changes to delete the cache

I didn't mean deleting the cache but ignoring it. Just remove it from XdgIconLoader::findIconHelper.

But I emphasize again: Without the cache, each icon should be found in the directory hierarchies of icon sets, and that takes more CPU time. It's not disastrous — you might not notice a difference with your computer — but it isn't good for all.

Whether it is possible to judge whether the cache needs to be updated by the result of cache->lookup instead of ignoring

tsujan commented 2 years ago

Whether it is possible to judge whether the cache needs to be updated....

As I said in https://github.com/lxqt/libqtxdg/issues/226, the current code should do it; it uses QFileSystemWatcher for watching the cache. But, for some reason unknown to us, sometimes, it fails with newly installed apps.

Please read https://github.com/lxqt/libqtxdg/issues/226 and see how I was fooled into thinking that it was fixed (I closed and reopened the report). The problem definitely exists, in spite of watching the cache.

tsujan commented 2 years ago

My last desperate thoughts were https://github.com/lxqt/libqtxdg/issues/226#issuecomment-879683647.

Dami-star commented 2 years ago

Whether it is possible to judge whether the cache needs to be updated....

As I said in #226, the current code should do it; it uses QFileSystemWatcher for watching the cache. But, for some reason unknown to us, sometimes, it fails with newly installed apps.

Please read #226 and see how I was fooled into thinking that it was fixed (I closed and reopened the report). The problem definitely exists, in spite of watching the cache.

Ok, I have a good environment to reproduce the problem here, I am going to continue to find the cause of the problem from the cache, and if you have a good solution, I can help verify it.

tsujan commented 2 years ago

I have a good environment to reproduce the problem here

Very good.

A short explanation:

When the GTK icon cache changes (= is updated after app's installation), our watcher makes QIconCacheGtkReader::isValid return false and invalidates the icon (see the c-tor of QIconCacheGtkReader). As a result, QIconCacheGtkReader::reValid is called by XdgIconLoader::findIconHelper, the validity of the new cache is determined, and the icon is searched in it if valid. This sequence seems logical.

Now, the problem seems to be that, in the last stage, the new cache is valid but doesn't contain the icon. My guess is that (1) it's somehow related to how gtk-update-icon-cache does its job, and (2) our watcher acts too soon, when the cache is changed but the icon isn't included in it yet.

Dami-star commented 2 years ago

I have a good environment to reproduce the problem here

Very good.

A short explanation:

When the GTK icon cache changes (= is updated after app's installation), our watcher makes QIconCacheGtkReader::isValid return false and invalidates the icon (see the c-tor of QIconCacheGtkReader). As a result, QIconCacheGtkReader::reValid is called by XdgIconLoader::findIconHelper, the validity of the new cache is determined, and the icon is searched in it if valid. This sequence seems logical.

Now, the problem seems to be that, in the last stage, the new cache is valid but doesn't contain the icon. My guess is that (1) it's somehow related to how gtk-update-icon-cache does its job, and (2) our watcher acts too soon, when the cache is changed but the icon isn't included in it yet.

Yes, I also think there is no problem with the logic of QIconCacheGtkReader::reValid, but what I have encountered is that after all the wine applications are uninstalled and the wine application is installed again, I cannot receive this signal QFileSystemWatcher::fileChanged

At this time there is no output printed here: QObject::connect(gtkCachesWatcher(), &QFileSystemWatcher::fileChanged, &m_file, [this](const QString & path) { qWarning()<<"QFileSystemWatcher::fileChanged++++++++++++1:"<<path<<m_file.fileName(); if (m_file.fileName() == path) { m_isValid = false; // invalidate icons to reload them ... QIconLoader::instance()->invalidateKey(); } });

Dami-star commented 2 years ago

BTW, every time we install and uninstall the wine application, icon-theme.cache is deleted and then recreated. Similar to rm icon-theme.cache && new icon-theme.cache

tsujan commented 2 years ago

BTW, every time we install and uninstall the wine application, icon-theme.cache is deleted and then recreated

That's how gtk-update-icon-cache works (→ https://gitlab.gnome.org/GNOME/gtk/-/blob/main/tools/updateiconcache.c#L1471).

Dami-star commented 2 years ago

BTW, every time we install and uninstall the wine application, icon-theme.cache is deleted and then recreated

That's how gtk-update-icon-cache works (→ https://gitlab.gnome.org/GNOME/gtk/-/blob/main/tools/updateiconcache.c#L1471).

Thanks, after many tests, I found that the problem may still lie in addPath, especially when uninstalling multiple wine applications at one time, but I haven't found the reason yet I'll keep looking for this problem until it's resolved ;-)

AR0x7E7 commented 2 years ago

Hi gents, after installing two identical machines with the one difference of using lubuntu 18.04 and lubuntu 22.04 it is very frustrating to see my wine apps having the correct icons in 18.04, but in 22.04 it appears I need to uninstall and reinstall them again :(

Really hope that you can find a solution soon, thanks.

stefonarch commented 2 years ago

Really hope that you can find a solution soon, thanks.

Lubuntu 18.04 was using LXDE, Lubuntu 22.04 is using LXQt. It's not that there is any solution to be found and surely not by us for a change like that.

Dami-star commented 2 years ago

@tsujan HI tsujan, Is it possible to consider monitoring the folder where icon-theme.cache is located instead of the icon-theme.cache file to solve this problem.

tsujan commented 2 years ago

Is it possible to consider monitoring the folder...

The folder changes twice (at least): after the icon is installed/removed, and after the cache is updated.. Provided that we skip the first change(s) by checking the cache's modification time, we might get the current result with more computation — but it may be worth a try.

If we don't skip the first change(s), it'll be like ignoring the cache and searching in the dir hierarchy — something we could have done from start if it was acceptable.

tsujan commented 2 years ago

I tried it; It didn't work. The result was the same as watching the cache file.

Dami-star commented 2 years ago

This is not good news 😥,I will continue to try other methods to see if I can fix the problem. Feel free to contact me if you have any other fixes or need to verify the problem. Thank you.

tsujan commented 2 years ago

This is not good news

No, it isn't. We're in the dark as to why the problem happens.

tsujan commented 2 years ago

FYI, I also experimented with delaying the icon invalidation. Even with a 1-second delay, some icons weren't found.

Dami-star commented 2 years ago

FYI, I also experimented with delaying the icon invalidation. Even with a 1-second delay, some icons weren't found. I also tested this method, and this delay time basically does not work when installing or uninstalling multiple applications at the same time

tsujan commented 2 years ago

@Dami-star, see https://github.com/lxqt/libqtxdg/issues/226#issuecomment-1191457085

Closing this because it isn't a real solution...

Dami-star commented 2 years ago

@Dami-star, see #226 (comment)

Closing this because it isn't a real solution...

Ok, I will use this scheme as a temporary scheme first, if I have a better scheme, I will open this and resubmit or apply for a pr