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

[Seb Ruiz]

2009/10/7 Mark Kretschmann <kretschmann at kde.org>:
> 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.

Indeed - the bug report seems to indicate that the scrolling happens
before the the new track is set. If this is the case, then we need to
ensure we call ensureVisible after the track is reset.

I think that we should revert this commit because a QSet is really the
datastructure of choice here. It's important to fix the cause of the
problem, not address the symptom here.


-- 
Seb Ruiz

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