Icon patches always need screenshots! :)
This is the wrong approach for thread safety (wrong level of locking). It should come with a multithreaded unittest so it can be checked for safety (with clang+tsan for instance). And yes the concept overall seems wrong anyway as QIcon isn't threadsafe in the first place, AFAIK.
@dfaure Can you commit these changes as I don't have the access to ?
- Remove a fixme.
- Re-write the the file index scheduler.
- Update the balooctl tool with the changed suspend/resume behaviour.
Consolidate file indexer close changes into a single review.
Run a full check for unindexed files when the watches are installed.
sorry for the caused inconvenience
Thanks for sorting that out.
Thanks for the patches! You can add Closes T9050 to one of those to make it automatically close this ticket once it's landed.
patch icon name https://phabricator.kde.org/D13637
patch plasma theme icons: https://phabricator.kde.org/D13636
D13630 should fix the Windows build. Sorry for not noticing this before. I was mostly away from keyboard for the last two weeks after having been sick.
@davidedmundson i don't have much time to test right now, so that's why my reviews looks like *guessing*. I want to know you a sure that loadIcon is called safety, so maybe dbus notifier
connect(s_globalData, &KIconLoaderGlobalData::iconChanged, this, &KIconLoaderPrivate::_k_refreshIcons, Qt::QueuedConnection);
This change broke the build on Windows, which due to the CI notification being ignored has now become a maintainability issue.
The failure log can be found at https://build.kde.org/job/Frameworks%20kfilemetadata%20kf5-qt5%20WindowsMSVCQt5.10/33/consoleText
It looks like to KIconLoader::loadIcon is called from multiple threads, but i'm not sure
Tue, Jun 19
Yep, this was from the old and deprecated modeltest.cpp.