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

List:       kde-devel
Subject:    Re: kiconloader.cpp, Help needed
From:       James Richard Tyrer <tyrerj () acm ! org>
Date:       2007-07-29 0:55:08
Message-ID: 46ABE56C.5040006 () acm ! org
[Download RAW message or body]

Aaron J. Seigo wrote:
> 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.

OK, then what happens for a system icon?

IT checks for "Illegal icon group"

Then sets the size.  IIUC, it sets an actual size as a negative number.

Then checks for an empty name

And then we have recursion (I think this is recursion):

	K3Icon icon = findMatchingIcon(name, size);

It appears that this statement could end the recursion:

	if (!QDir::isRelativePath(_name)) return _name;

So, I looked that up and it means what I thought.  So recursion is ended 
when there is an absolute path.

Wouldn't:

	if (QDir::isAbsolutePath(_name)) return _name;

be clearer?

IAC, I can't figure out how "_name" is ever set to the absolute path of 
the icon.

I agree that this code is a mess.  I see several mistakes which would 
have resulted in lowering of my grade if I had had the nerve to turn in 
such code for a college assignment.

The worst part of it is that it goes against the idea of making C++ code 
easy to maintain.

-- 
JRT
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]