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

[Seb Ruiz]

2009/10/7 Seb Ruiz <ruiz at kde.org>:
> 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.

A quick squiz at the source tells me that we should be calling
View::scrollToActiveTrack() in
Playlist::Actions::engineNewTrackPlaying() and abolishing
engineNewTrackPlaying() from the view object -- the view should
respond to changes from the model, and the model can be controlled by
actions.



-- 
Seb Ruiz

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