[Merge] lp:~phablet-team/qtubuntu-media/enable-mpris-controls into lp:qtubuntu-media/stable

[Alfonso Sanchez-Beato]

Review: Needs Fixing

Looks good, some minor issues detected. I would also like to have a branch for trunk too.

Diff comments:

> 
> === modified file 'src/aal/aalmediaplayerservice.cpp'
> --- src/aal/aalmediaplayerservice.cpp	2015-08-18 15:49:35 +0000
> +++ src/aal/aalmediaplayerservice.cpp	2015-09-08 20:15:10 +0000
> @@ -36,7 +36,7 @@
>  #include <qtubuntu_media_signals.h>
>  
>  // Uncomment to re-enable media-hub Player session detach/reattach functionality

As now this is on by default, this comment should change to something like "Comment to disable...". Or directly remove the macro and all instances of it in the code, as I understand we now want this to be always defined.

> -//#define DO_PLAYER_ATTACH_DETACH
> +#define DO_PLAYER_ATTACH_DETACH
>  
>  // Defined in aalvideorenderercontrol.h
>  #ifdef MEASURE_PERFORMANCE
> @@ -293,16 +293,17 @@
>      qDebug() << "Setting media to: " << url;
>      const media::Track::UriType uri(url.url().toStdString());
>      try {
> -        if (m_mediaPlaylistProvider != NULL)
> -            m_mediaPlaylistProvider->addMedia(QMediaContent(url));
> -        else
> +        if (m_mediaPlaylistProvider == nullptr)

We should do the try/catch around "m_hubPlayerSession->open_uri(uri);" only, and move the "if" statement out of it.

>              m_hubPlayerSession->open_uri(uri);
>      }
>      catch (const std::runtime_error &e) {
> -        qWarning() << "Failed to open media " << url << ": " << e.what();
> +        qWarning() << "Failed to set media " << url << ": " << e.what();
>          return;
>      }
>  
> +    // Make sure this player can be controlled by MPRIS if appropriate
> +    updateCurrentPlayer();
> +
>      if (isVideoSource())
>          m_videoOutput->setupSurface();
>  }
> @@ -770,12 +776,29 @@
>      }
>  }
>  
> -void AalMediaPlayerService::disconnect_signals()
> +void AalMediaPlayerService::disconnectSignals()
>  {
>      if (m_endOfStreamConnection.is_connected())
>          m_endOfStreamConnection.disconnect();
>  }
>  
> +void AalMediaPlayerService::updateCurrentPlayer()
> +{
> +    try {
> +        // If this player is a multimedia audioRole, then it should possible to
> +        // use it for MPRIS control
> +        if (audioRole() == QMediaPlayer::MultimediaRole)

Here we can move the "if" statement out of the try/catch block too.

> +        {
> +            qDebug() << "Setting player as current player";
> +            const media::Player::PlayerKey key = m_hubPlayerSession->key();
> +            m_hubService->set_current_player(key);
> +        }
> +    } catch (const std::runtime_error &e) {
> +        qWarning() << "Failed to set player as current player: " << e.what();
> +        return;
> +    }
> +}
> +
>  void AalMediaPlayerService::onError(const core::ubuntu::media::Player::Error &error)
>  {
>      qWarning() << "** Media playback error: " << error;
> 
> === modified file 'tests/unit/service.cpp'
> --- tests/unit/service.cpp	2015-06-08 19:15:43 +0000
> +++ tests/unit/service.cpp	2015-09-08 20:15:10 +0000
> @@ -54,6 +54,10 @@
>      return 0;
>  }
>  
> +void TestService::set_current_player(Player::PlayerKey)

Is this a place holder for a future unit test to be implemented? A TODO comment would be helpful if that is the case.

> +{
> +}
> +
>  void TestService::pause_other_sessions(Player::PlayerKey)
>  {
>  }


-- 
https://code.launchpad.net/~phablet-team/qtubuntu-media/enable-mpris-controls/+merge/270445
Your team Ubuntu Phablet Team is subscribed to branch lp:qtubuntu-media/stable.