Review Request 118561: Allow passing custom theme base path together with custom app themes

[Martin Klapetek]

> On June 5, 2014, 10:22 p.m., Christoph Feck wrote:
> > Thanks Martin for looking at the issue.
> > 
> > Just one question: Is plasmashell the only application which needs to include the additional search path, or does every application wanting a tray icon call the addAppDir() function? I am bit worried about any additional "stat" calls. Those we have already slow down the icon lookup considerably.

Just the application that wants to use those special icons, so that would be just plasmashell. It goes like this: 
 - Application creates new SNI
 - Plasma is notified through a dataengine
 - The dataengine reads all the SNI properties, including IconThemePath property
 - The dataengine sets the the additional path on KIconLoader (where appName is the app the SNI belongs to)
 - Plasma loads the icons using the QML component IconItem (which uses QIcon::fromTheme() which in turn uses KIconEngine aaand that uses KIconLoader)

The additional paths are added at the end of the list and it's usually one directory only in case of the SNIs, so the rise of the number of lookups is not that dramatic because about 99% of all icons are found before the lookup even reaches the normal hicolor theme and so this adds only one lookup per registered application in the worst case only. Plus there is icon cache which also helps things.


> On June 5, 2014, 10:22 p.m., Christoph Feck wrote:
> > src/kiconloader.h, line 209
> > <https://git.reviewboard.kde.org/r/118561/diff/1/?file=279078#file279078line209>
> >
> >     Are we still allowed to break binary compatibility? If not, please create a separate call.

I have to ask.


> On June 5, 2014, 10:22 p.m., Christoph Feck wrote:
> > src/kicontheme.h, line 53
> > <https://git.reviewboard.kde.org/r/118561/diff/1/?file=279080#file279080line53>
> >
> >     The comment does not clarify where the additional hint is included in the search chain (first or last).

Last; will fix.


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118561/#review59358
-----------------------------------------------------------


On June 5, 2014, 12:52 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118561/
> -----------------------------------------------------------
> 
> (Updated June 5, 2014, 12:52 p.m.)
> 
> 
> Review request for KDE Frameworks and Christoph Feck.
> 
> 
> Repository: kiconthemes
> 
> 
> Description
> -------
> 
> KIconTheme in kdelibs4 was searching KGlobal::dirs()->resourceDirs("data") to find all icons; the status notifier dataengine was then setting custom resourceDirs(..) with custom SNI theme paths (which the SNIs can pass) and so the SNI icons always ended up in the theme search paths (the SNI icons are stored as a whole theme).
> 
> With the port to QStandardPaths, we lost the ability to pass custom dirs into the theme search paths and that results in status notifier icons in Plasma Next having "unknown" icons as their icon theme paths are not searched and so icons are not found. This is the case mostly of the Qt4 apps running on Qt with the QSystrayIcon-to-SNI-patches (case of *buntu and I heard opensuse too?)
> 
> KIconLoader however has "addAppDir(..)" method, so I expanded that method to actually take an "app dir" and add it to the theme search paths. Plasma Next now have proper icons in the systray.
> 
> 
> Diffs
> -----
> 
>   src/kiconloader.h 68ccd09 
>   src/kiconloader.cpp 4080a1d 
>   src/kicontheme.h 73011e2 
>   src/kicontheme.cpp 12337e8 
> 
> Diff: https://git.reviewboard.kde.org/r/118561/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Martin Klapetek
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140606/4b84efe9/attachment.html>