dfaure requested changes to this revision.
dfaure added a comment.
This revision now requires changes to proceed.
You fixed the small details I found, but the big picture is still very wrong.
Using the same KIconLoader in multiple threads cannot work and doesn't make
sense: one thread could addAppDir(), or reinit, messing up results for another
thread.
Use a different KIconLoader per thread, and then the only thing you have to
make threadsafe is any use of a global object, but not the kiconloader itself,
which CANNOT be made threadsafe given its API (theme() is a perfect example).
INLINE COMMENTS
> kiconeffect.cpp:74
>
> +KIconEffect& KIconEffect::operator=(const KIconEffect &other)
> +{
unrelated to this patch, right?
> kiconeffect.cpp:76
> +{
> + memcpy(d, other.d, sizeof(*d));
> + return *this;
warning, very wrong if there's a 'q' pointer...
> kiconloader.cpp:1327
> +{
> + QPixmap pixmap = S_D(d)->loadMimeTypeIcon(_iconName, group, size, state,
> overlays, path_store);
> + return pixmap;
And now such things could be written in a much more standard and readable way
with a QMutexLocker at the beginning of the method, rather then the S_D macro
and proxy hack.
> kiconloader.cpp:1655
> {
> - d->initIconThemes();
> - if (d->mpThemeRoot) {
> - return d->mpThemeRoot->theme;
> + KIconTheme *theme = S_D(d)->theme();
> + return theme;
This is not threadsafe, it's wishful-thinking-threadsafe.
Sure, the call to theme() is protected from interference from other threads,
but if another thread can recreate the themes, then the pointer you got out of
this method call is now invalid.
So I'll say it again: use a different KIconLoader instance in every thread,
MUCH simpler, MUCH safer.
> kiconloader.h:482
> +#ifndef KICONTHEMES_NO_DEPRECATED
> + KICONTHEMES_DEPRECATED KIconEffect *iconEffect() const;
> +#endif
unrelated to this patch, please split this out
REPOSITORY
R302 KIconThemes
REVISION DETAIL
https://phabricator.kde.org/D13627
To: anthonyfieroni, davidedmundson, dfaure, #frameworks
Cc: kde-frameworks-devel, michaelh, ngraham, bruns
