[Merge] lp:~renatofilho/mediaplayer-app/shared-snappy into lp:mediaplayer-app

Jim Hodapp jim.hodapp at canonical.com
Fri Nov 18 14:57:03 UTC 2016


Review: Needs Fixing code

A couple of issues below.

Diff comments:

> === modified file 'config.h.in'
> --- config.h.in	2014-10-01 21:29:22 +0000
> +++ config.h.in	2016-11-17 23:15:21 +0000
> @@ -29,7 +29,11 @@
>  }
>  
>  inline QString mediaPlayerDirectory() {
> -    if (isRunningInstalled()) {
> +    static const QByteArray SNAP_PATH("SNAP");
> +
> +    if (qEnvironmentVariableIsSet(SNAP_PATH)) {

You've tested this both on the desktop and on a phone?

> +        return QString("%1/@MEDIAPLAYER_DIR@").arg(QString(qgetenv(SNAP_PATH)));
> +    } else if (isRunningInstalled()) {
>          return QString("@MEDIAPLAYER_DIR@");
>      } else {
>          return QString("@mediaplayer_src_SOURCE_DIR@");
> 
> === added directory 'snap'
> === added directory 'snap/ubuntu-app-platform'
> === modified file 'snapcraft.yaml'
> --- snapcraft.yaml	2016-09-08 20:56:15 +0000
> +++ snapcraft.yaml	2016-11-17 23:15:21 +0000
> @@ -32,9 +39,7 @@
>        - -DCMAKE_INSTALL_PREFIX:PATH=/usr
>        - -DCMAKE_LIBRARY_PATH=/usr/lib
>  
> -    stage-packages:
> -      - qtubuntu-media
> -      - qtdeclarative5-ubuntu-content1
> +    stage-packages: []

Any reason to keep this around if empty?

>  
>      filesets:
>        unwanted:


-- 
https://code.launchpad.net/~renatofilho/mediaplayer-app/shared-snappy/+merge/311212
Your team Ubuntu Phablet Team is subscribed to branch lp:mediaplayer-app.



More information about the Ubuntu-reviews mailing list