Review Request 110833: Remove KIconLoader dependency to KWidgets

[Kevin Ottens]

> On June 11, 2013, 9:07 a.m., Kevin Ottens wrote:
> > This change go in. Note that I disagree with the aim though: KWidgets should *not* depend on KIconThemes. Now having KIconThemes not use KGlobalSettings is completely welcome (aim being to have KGlobalSettings in kde4support at some point).
> 
> Aleix Pol Gonzalez wrote:
>     Well, then the KWidgets not depending on KIconItems contradicts this review request, right?
> 
> Kevin Ottens wrote:
>     Well, not really. Here it's more about removing the dependency on KGlobalSettings than anything, and that is definitely welcome. Your commit log should probably reflect that, and likely you should remove the include kglobalsettings.h line.
> 
> Aleix Pol Gonzalez wrote:
>     I can't do it because of:
>             KIconLoader::emitChange(KIconLoader::Group(arg));
>     
>     If I removed that, we'd lose backwards compatibility. I could copy it over as:
>             QDBusMessage message = QDBusMessage::createSignal("/KIconLoader", "org.kde.KIconLoader", "iconChanged" );
>             message.setArguments(QList<QVariant>() << int(group));
>             QDBusConnection::sessionBus().send(message);
>     
>     But then I'd worry about maintainability...

You lost me here... I meant that KIconLoader shall not use KGlobalSettings (so should not include kglobalsettings.h), not the other way around (so it's fine for KGlobalSettings to call KIconLoader::emitChange).


- Kevin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110833/#review34115
-----------------------------------------------------------


On June 10, 2013, 11:07 a.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/110833/
> -----------------------------------------------------------
> 
> (Updated June 10, 2013, 11:07 a.m.)
> 
> 
> Review request for KDE Frameworks and David Faure.
> 
> 
> Description
> -------
> 
> As we discussed in the IRC meeting, I removed the KIconLoader dependency to KWidgets (so that we can make KWidgets depend on KIconLoader).
> 
> I'm unsure if it's the best approach, so I tried to do the simplest implementation I could. Don't hesitate to suggest changes.
> 
> 
> Diffs
> -----
> 
>   staging/kwidgets/src/utils/kglobalsettings.cpp 53b648e 
>   staging/kwidgets/src/utils/kglobalsettings.h 5fb1d9b 
>   staging/kiconthemes/src/kiconloader.cpp 795ec93 
>   staging/kiconthemes/src/CMakeLists.txt 82828dc 
>   staging/kiconthemes/src/kiconloader.h e4423eb 
> 
> Diff: http://git.reviewboard.kde.org/r/110833/diff/
> 
> 
> Testing
> -------
> 
> it builds
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130612/6505ce5b/attachment-0001.html>