[Amarok] Fix: Automatic playlist scrolling was broken in so

[Mark Kretschmann]

On Wed, Oct 7, 2009 at 1:22 PM, Seb Ruiz <ruiz at kde.org> wrote:
> 2009/10/7 Mark Kretschmann <kretschmann at kde.org>:
>> commit a9a46b6ceab835c646b89d744bfcc5a7176ef5c1
>> Author:     Mark Kretschmann <kretschmann at kde.org>
>> AuthorDate: Wed Oct 7 12:50:25 2009 +0200
>> Commit:     Mark Kretschmann <kretschmann at kde.org>
>> CommitDate: Wed Oct 7 12:50:25 2009 +0200
>>
>>    Fix: Automatic playlist scrolling was broken in some cases.
>>
>>    The actual bug was very very nasty. EngineObserver used QSet for storing
>>    observer pointers. This is what Qt docs say:
>>    "QSet is unordered, so an iterator's sequence cannot be assumed to be predictable."
>>    Now we're using QList, whose iteration order is predictable.
>>
>>    Thanks to Anthony Vital for pointing this out.
>>
>>    BUG: 193459
>>
>> diff --git a/ChangeLog b/ChangeLog
>> index 2518826..b89cadf 100644
>> --- a/ChangeLog
>> +++ b/ChangeLog
>> @@ -38,6 +38,7 @@ VERSION 2.2.1
>>       the lyrics applet.
>>
>>   BUGFIXES:
>> +    * Automatic playlist scrolling was broken in some cases. (BR 193459)
>>     * The On Screen Display now uses a sane font size with all display
>>       resolutions. (BR 195186)
>>     * Fix rare regression where inaccessible subfolders (due to permissions)
>> diff --git a/src/EngineObserver.cpp b/src/EngineObserver.cpp
>> index d62f40d..37b85fe 100644
>> --- a/src/EngineObserver.cpp
>> +++ b/src/EngineObserver.cpp
>> @@ -209,13 +209,13 @@ EngineSubject::attach( EngineObserver *observer )
>>     if( object )
>>         connect( object, SIGNAL( destroyed( QObject* ) ), this, SLOT( observerDestroyed( QObject* ) ) );
>>
>> -    Observers.insert( observer );
>> +    Observers.append( observer );
>>  }
>>
>>  void
>>  EngineSubject::detach( EngineObserver *observer )
>>  {
>> -    Observers.remove( observer );
>> +    Observers.removeAll( observer );
>>  }
>>
>>  void
>> @@ -224,7 +224,7 @@ EngineSubject::observerDestroyed( QObject* object ) //SLOT
>>     DEBUG_BLOCK
>>
>>     EngineObserver* observer = reinterpret_cast<EngineObserver*>( object ); // cast is OK, it's guaranteed to be an EngineObserver
>> -    Observers.remove( observer );
>> +    Observers.removeAll( observer );
>>
>>     debug() << "Removed EngineObserver: " << observer;
>>  }
>> diff --git a/src/EngineObserver.h b/src/EngineObserver.h
>> index c9f023c..92f15f6 100644
>> --- a/src/EngineObserver.h
>> +++ b/src/EngineObserver.h
>> @@ -24,7 +24,6 @@
>>  #include <Phonon/Global>
>>  #include <QHash>
>>  #include <QPointer>
>> -#include <QSet>
>>
>>  class EngineSubject;
>>  class QString;
>> @@ -187,7 +186,7 @@ private:
>>     bool isMetaDataSpam( QHash<qint64, QString> newMetaData );
>>
>>     QList<QHash<qint64, QString> > m_metaDataHistory;
>> -    QSet<EngineObserver*> Observers;
>> +    QList<EngineObserver*> Observers;
>>     Phonon::State m_realState; // To work around the buffering issue
>>  };
>>
>
> Curious - how does this solve the problem? Why should it matter what
> order the observers are in - I dislike that there is any code that
> relies on some deterministic order for iterating over them (each
> observer should be disjoint from the other)

I agree that relying on the order is dangerous. So maybe the real
problem is in some other part of Amarok, design wise.

-- 
Mark Kretschmann
Amarok Developer
www.kde.org - amarok.kde.org