Improve error handling.

Errors reported via:

* the DummyPlayer didn't work due to it not doing the usual state
  transitions/emission
* MediaObject::setSource() due to errors being emitted
  before connections being set up.
* A general state bug.

Task-number: QTBUG-4752
Reviewed-by: Gareth Stockwell

7 files changed, 57 insertions, 51 deletions

diff --git a/src/3rdparty/phonon/mmf/abstractmediaplayer.cpp b/src/3rdparty/phonon/mmf/abstractmediaplayer.cpp
index af2c31e..998e861 100644
--- a/src/3rdparty/phonon/mmf/abstractmediaplayer.cpp
+++ b/src/3rdparty/phonon/mmf/abstractmediaplayer.cpp
@@ -359,26 +359,27 @@ qint64 MMF::AbstractMediaPlayer::toMilliSeconds(const TTimeIntervalMicroSeconds
     return in.Int64() / 1000;
 }
 
+//-----------------------------------------------------------------------------
+// Slots
+//-----------------------------------------------------------------------------
+
+void MMF::AbstractMediaPlayer::tick()
+{
+    // For the MWC compiler, we need to qualify the base class.
+    emit MMF::AbstractPlayer::tick(currentTime());
+}
+
 void MMF::AbstractMediaPlayer::changeState(PrivateState newState)
 {
-    TRACE_CONTEXT(AbstractPlayer::changeState, EAudioInternal);
-    TRACE_ENTRY("state %d newState %d", privateState(), newState);
+    TRACE_CONTEXT(AbstractMediaPlayer::changeState, EAudioInternal);
 
     // TODO: add some invariants to check that the transition is valid
+    AbstractPlayer::changeState(newState);
 
     const Phonon::State oldPhononState = phononState(privateState());
     const Phonon::State newPhononState = phononState(newState);
-    if (oldPhononState != newPhononState) {
-        TRACE("emit stateChanged(%d, %d)", newPhononState, oldPhononState);
-        emit stateChanged(newPhononState, oldPhononState);
-    }
-
-    setState(newState);
 
-    if (
-        LoadingState == oldPhononState
-        and StoppedState == newPhononState
-    ) {
+    if (LoadingState == oldPhononState && StoppedState == newPhononState) {
         // Ensure initial volume is set on MMF API before starting playback
         doVolumeChanged();
 
@@ -391,17 +392,6 @@ void MMF::AbstractMediaPlayer::changeState(PrivateState newState)
         }
     }
 
-    TRACE_EXIT_0();
-}
-
-//-----------------------------------------------------------------------------
-// Slots
-//-----------------------------------------------------------------------------
-
-void MMF::AbstractMediaPlayer::tick()
-{
-    // For the MWC compiler, we need to qualify the base class.
-    emit MMF::AbstractPlayer::tick(currentTime());
 }
 
 QT_END_NAMESPACE
diff --git a/src/3rdparty/phonon/mmf/abstractmediaplayer.h b/src/3rdparty/phonon/mmf/abstractmediaplayer.h
index 698b899..1ea236b 100644
--- a/src/3rdparty/phonon/mmf/abstractmediaplayer.h
+++ b/src/3rdparty/phonon/mmf/abstractmediaplayer.h
@@ -71,10 +71,6 @@ protected:
     virtual int setDeviceVolume(int mmfVolume) = 0;
     virtual int openFile(RFile& file) = 0;
     virtual void close() = 0;
-
-    /**
-     * Changes state and emits stateChanged()
-     */
     virtual void changeState(PrivateState newState);
 
 protected:
diff --git a/src/3rdparty/phonon/mmf/abstractplayer.cpp b/src/3rdparty/phonon/mmf/abstractplayer.cpp
index e3c0ecb..de2722d 100644
--- a/src/3rdparty/phonon/mmf/abstractplayer.cpp
+++ b/src/3rdparty/phonon/mmf/abstractplayer.cpp
@@ -118,12 +118,14 @@ void MMF::AbstractPlayer::videoOutputChanged()
     // Default behaviour is empty - overridden by VideoPlayer
 }
 
-void MMF::AbstractPlayer::setError(Phonon::ErrorType error)
+void MMF::AbstractPlayer::setError(Phonon::ErrorType error,
+                                   const QString &errorMessage)
 {
     TRACE_CONTEXT(AbstractPlayer::setError, EAudioInternal);
     TRACE_ENTRY("state %d error %d", m_state, error);
 
     m_error = error;
+    m_errorString = errorMessage;
     changeState(ErrorState);
 
     TRACE_EXIT_0();
@@ -138,9 +140,7 @@ Phonon::ErrorType MMF::AbstractPlayer::errorType() const
 
 QString MMF::AbstractPlayer::errorString() const
 {
-    // TODO: put in proper error strings
-    QString result;
-    return result;
+    return m_errorString;
 }
 
 Phonon::State MMF::AbstractPlayer::phononState() const
@@ -173,5 +173,29 @@ void MMF::AbstractPlayer::setState(PrivateState newState)
     m_state = newState;
 }
 
+void MMF::AbstractPlayer::changeState(PrivateState newState)
+{
+    TRACE_CONTEXT(AbstractPlayer::changeState, EAudioInternal);
+    TRACE_ENTRY("state %d newState %d", privateState(), newState);
+
+    // TODO: add some invariants to check that the transition is valid
+
+    const Phonon::State oldPhononState = phononState(privateState());
+
+    // We need to change the state before we emit stateChanged(), because
+    // some user code, for instance the mediaplayer, switch on MediaObject's
+    // state.
+    setState(newState);
+
+    const Phonon::State newPhononState = phononState(newState);
+
+    if (oldPhononState != newPhononState) {
+        TRACE("emit stateChanged(%d, %d)", newPhononState, oldPhononState);
+        emit stateChanged(newPhononState, oldPhononState);
+    }
+
+    TRACE_EXIT_0();
+}
+
 QT_END_NAMESPACE
 
diff --git a/src/3rdparty/phonon/mmf/abstractplayer.h b/src/3rdparty/phonon/mmf/abstractplayer.h
index 08558cf..1c4ea02 100644
--- a/src/3rdparty/phonon/mmf/abstractplayer.h
+++ b/src/3rdparty/phonon/mmf/abstractplayer.h
@@ -93,7 +93,8 @@ public:
     /**
      * Records error and changes state to ErrorState
      */
-    void setError(Phonon::ErrorType error);
+    void setError(Phonon::ErrorType error,
+                  const QString &errorMessage = QString());
 
     Phonon::State state() const;
 Q_SIGNALS:
@@ -132,7 +133,10 @@ protected:
 
     PrivateState privateState() const;
 
-    virtual void changeState(PrivateState newState) = 0;
+    /**
+     * Changes state and emits stateChanged()
+     */
+    virtual void changeState(PrivateState newState);
 
     /**
      * Modifies m_state directly. Typically you want to call changeState(),
@@ -152,6 +156,7 @@ protected:
 private:
     PrivateState                m_state;
     Phonon::ErrorType           m_error;
+    QString                     m_errorString;
     qint32                      m_tickInterval;
     qint32                      m_transitionTime;
     qint32                      m_prefinishMark;
diff --git a/src/3rdparty/phonon/mmf/dummyplayer.cpp b/src/3rdparty/phonon/mmf/dummyplayer.cpp
index bd21d20..e6f3855 100644
--- a/src/3rdparty/phonon/mmf/dummyplayer.cpp
+++ b/src/3rdparty/phonon/mmf/dummyplayer.cpp
@@ -87,11 +87,6 @@ qint64 MMF::DummyPlayer::currentTime() const
     return 0;
 }
 
-QString MMF::DummyPlayer::errorString() const
-{
-    return QString();
-}
-
 Phonon::ErrorType MMF::DummyPlayer::errorType() const
 {
     return Phonon::NoError;
@@ -127,9 +122,5 @@ void MMF::DummyPlayer::doSetTickInterval(qint32)
 
 }
 
-void MMF::DummyPlayer::changeState(PrivateState)
-{
-}
-
 QT_END_NAMESPACE
 
diff --git a/src/3rdparty/phonon/mmf/dummyplayer.h b/src/3rdparty/phonon/mmf/dummyplayer.h
index 9ff9f78..c6270c9 100644
--- a/src/3rdparty/phonon/mmf/dummyplayer.h
+++ b/src/3rdparty/phonon/mmf/dummyplayer.h
@@ -54,7 +54,6 @@ public:
     virtual bool isSeekable() const;
     virtual qint64 currentTime() const;
     virtual Phonon::State state() const;
-    virtual QString errorString() const;
     virtual Phonon::ErrorType errorType() const;
     virtual qint64 totalTime() const;
     virtual MediaSource source() const;
@@ -64,9 +63,6 @@ public:
 
     // AbstractPlayer
     virtual void doSetTickInterval(qint32 interval);
-
-protected:
-    virtual void changeState(PrivateState newState);
 };
 }
 }
diff --git a/src/3rdparty/phonon/mmf/mediaobject.cpp b/src/3rdparty/phonon/mmf/mediaobject.cpp
index 29ac2df..74aaa58 100644
--- a/src/3rdparty/phonon/mmf/mediaobject.cpp
+++ b/src/3rdparty/phonon/mmf/mediaobject.cpp
@@ -239,6 +239,7 @@ void MMF::MediaObject::createPlayer(const MediaSource &source)
     const bool oldPlayerSeekable = oldPlayer->isSeekable();
 
     Phonon::ErrorType error = NoError;
+    QString errorMessage;
 
     // Determine media type
     switch (source.type()) {
@@ -253,7 +254,7 @@ void MMF::MediaObject::createPlayer(const MediaSource &source)
                 mediaType = fileMediaType(url.toLocalFile());
             }
             else {
-                TRACE_0("Network streaming not supported yet");
+                errorMessage = QLatin1String("Network streaming not supported yet");
                 error = NormalError;
             }
         }
@@ -286,7 +287,8 @@ void MMF::MediaObject::createPlayer(const MediaSource &source)
             newPlayer = new DummyPlayer();
         }
 
-        newPlayer->setError(NormalError);
+        error = NormalError;
+        errorMessage = tr("Media type could not be determined");
         break;
 
     case MediaTypeAudio:
@@ -321,9 +323,11 @@ void MMF::MediaObject::createPlayer(const MediaSource &source)
     connect(m_player.data(), SIGNAL(finished()), SIGNAL(finished()));
     connect(m_player.data(), SIGNAL(tick(qint64)), SIGNAL(tick(qint64)));
 
-    if (error != NoError ) {
-        newPlayer = new DummyPlayer();
-        newPlayer->setError(error);
+    // We need to call setError() after doing the connects, otherwise the
+    // error won't be received.
+    if (error != NoError) {
+        Q_ASSERT(m_player);
+        m_player->setError(error, errorMessage);
     }
 
     TRACE_EXIT_0();