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

List:       kde-core-devel
Subject:    Re: [PATCH] Highlighting thumbnails in konqueror
From:       Martijn Klingens <mklingens () yahoo ! com>
Date:       2001-07-12 17:53:16
[Download RAW message or body]

On Thursday 12 July 2001 11:12, David Faure wrote:
> Looks good, except for the code duplication.
> Add a "reloadIcon" or whatever to KFileIVI, that takes care of
> "if thumbnail then setThumbnailEffect else setIcon"
> (in fact setThumbnailEffect isn't necessary anymore then, you can
> move its code into that method)

If have renamed the function to
    void setEffect( int group, int state );
and made it generic according to your directions.

I also followed Frerich's advice and moved the d pointer into the KFileIVI 
namespace.

Patch is not attached now because I am in the middle of an XF86 upgrade to 
4.1 and a KDE CVS update, so I can't compile libkonq at all atm, and I don't 
think you would appreciate it if I committed without testing locally...

One _VERY_ major question though: the original code calls setIcon() which in 
turn calls m_fileitem->pixmap() for every state change. Is there a reason for 
this, besides killing performance for re-discovering that the icon hasn't 
been changed? Unless it is possible that active icons are different (that is, 
not only the effect, but also the base image itself), it doesn't make sense 
to call this code for highlighting. And now I already have a pixmap for 
caching in place for the thumbs I might as well use it for the normal icons.

Just give me the green light an I'll change that as well ;-)

> This code is duplicated 3 times in your patch...

I think I tried a bit too hard to leave the existing code intact, so that I 
didn't even tried to resolve code duplication :-)

Martijn

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