[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: kiconloader.cpp, Help needed
From:       "Aaron J. Seigo" <aseigo () kde ! org>
Date:       2007-07-28 22:15:28
Message-ID: 200707281615.32320.aseigo () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Saturday 28 July 2007, James Richard Tyrer wrote:
> I am having some difficulty understanding this code.  Specifically, the
> attached which is from KDE4 trunk.
>
> The documentation for "iconPath" says:
>
> QString KIconLoader::iconPath   (  const QString &   name,
>    	int   group_or_size,
>    	bool   canReturnNull = false
>    	)   const
>
>   Returns the path of an icon.
>
> Parameters:
>
> name   The name of the icon, without extension. If an absolute path is
>      supplied for this parameter, iconPath will return it directly.
>
> group_or_size   If positive, search icons whose size is specified by the
>      icon group group_or_size. If negative, search icons whose size is -
> group_or_size. See K3Icon::Group and K3Icon::StdSizes
>      canReturnNull   Can return a null string? If not, a path to the
>      "unknown" icon will be returned.
>
> Returns:
>
>      the path of an icon, can be null or the "unknown" icon when not
>      found, depending on canReturnNull.
>
> -------------------------------------------------------------------------
>
> Since I can't figure out the code and after reading this, it appears to
> me that the code is wrong.
>
> Specifically, the documentation says that the icon name is to be
> supplied _without_ extension.  However, KIconLoader::findMatchingIcon
> adds a path before calling it.
>
> Obviously, KIconLoader::findMatchingIcon is called with an icon name
> without an extension so it would seem that it could simply pass it on
> to: KIconLoader::iconPath.
>
> I, therefore, suggest the attached patch which would greatly speed up
> the code.  Or do I have it wrong?

in the case where group_or_size is K3Icon::user (god i hate K3Icon in kde's 
api =) then yes you are right. however, for any -system- icon (e.g. installed 
with kde itself or an app) then it will break. the checks in iconPath for the 
different extensions happen only in the case of a "User" icon.

in the case of size == K3Icon::user in findMatchingIcon, the code will go into 
the first iteration of the first for loop in the foreach and then return. 
so ... it wouldn't really save much time at all i'm afraid except for the 
setup and tear down of the foreach and the first inner for, which is probably 
negligable. you could always benchmark it though? and again, this is only in 
the case of K3Icon::User which isn't from my experience overly common anyways 
=)

the code is very confusing and not particularly clear in a number of places. 
it makes hacking in there pretty ... interesting at times. it could really 
use with a complete rewrite, but that'll have to wait for some far future day 
at this point i think =) it works right now and that the important bit. 

what would -really- speed things up is if we could get rid of as many usages 
of QImage in there as possible. those are just killer.

> If KIconLoader::findMatchingIcon must be called with an icon name
> without extension then Line 618:
>
>      QString name = removeIconExtension( _name );
>
> is redundant.

you can pass in a full path to an icon, which means it will likely have an 
extension, so it does need to behave properly (remove the extension) in this 
case. this is a dubious design call imho (you can pass in /path/to/foo.svg 
and get back /path/to/foo.png) but then why else would you pass a full path 
in if not to find the "best" match for it? would be interesting to see where 
it is used this way, and why.

if it can be determined that the full path option is not useful (wouldn't be 
surprise), which would take spending some time on lxr.kde.org and/or grepping 
code, it can probably be changed.

-- 
Aaron J. Seigo
humru othro a kohnu se
GPG Fingerprint: 8B8B 2209 0C6F 7C47 B1EA  EE75 D6B7 2EB1 A7F1 DB43

KDE core developer sponsored by Trolltech

[Attachment #5 (application/pgp-signature)]

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<


[prev in list] [next in list] [prev in thread] [next in thread]