Fwd: [Amarok] Huge performance improvements for the Cover Manage

[Seb Ruiz]

2009/10/26 Michael Reiher <redm at gmx.de>:
> On Sunday 18 October 2009 00:32:58 Seb Ruiz wrote:
>> ---------- Forwarded message ----------
>> From: Michael Reiher <redm at gmx.de>
>> Date: 2009/10/17
>> Subject: Re: [Amarok] Huge performance improvements for the Cover Manage
>> To: Seb Ruiz <ruiz at kde.org>
>>
>> On Friday 16 October 2009 08:25:31 you wrote:
>> > 2009/10/16 Mark Kretschmann <kretschmann at kde.org>:
>> > > -    if( !m_hasImageChecked )
>> > > -        m_hasImage = ! const_cast<SqlAlbum*>( this )->image( size
>> > > ).isNull(); +    if( !m_hasImageChecked ) {
>> > > +
>> > > +        m_hasImageChecked = true;
>> > > +
>> > > +        QString image = const_cast<SqlAlbum*>( this )->findImage( size
>> > > ); +
>> > > +        // The user has explicitly set no cover
>> > > +        if( image == AMAROK_UNSET_MAGIC )
>> > > +            m_hasImage = false;
>> > > +        else m_hasImage = !image.isEmpty();
>> > > +    }
>> > > +
>> >
>> > Long lived caches can be dangerous. This introduces a defect: you
>> > aren't clearing the value of m_hasImage and m_hasImageChecked  when
>> > unsetting an album cover.
>>
>> Hi
>>
>> I'm not sure I understand what you mean here... I didn't introduce those
>> m_hasImageChecked and m_hasImage members. I actually made hasImage() work
>>  more or less along the lines of image() in this regard. If you are
>>  referring to removeImage(), you might be right, it should probably reset
>>  these... (see attached patch). That's not my fault though ;) I didn't
>>  touch this.
>>
>> Regarding the patch: I removed setting the cached image. This shouldn't be
>> necessary, as its done when fetching the updated image anyway.
>>
>> Greets Michael
>>
>
> Could someone be so nice to commit this patch for me? Provided it's correct of
> course...

I've commit the patch for you. The patch had one bug: there was no
need to reset the m_hasImageChecked variable (as we know there is no
image).




-- 
Seb Ruiz

http://www.sebruiz.net/
http://amarok.kde.org/