[Merge] lp:~chunsang/media-hub/wip_media-hub-interface into lp:media-hub

[Alfonso Sanchez-Beato]

Review: Needs Fixing

See comments below

Diff comments:

> === added directory 'snap'
> === added directory 'snap/ubuntu-app-platform'
> === modified file 'snapcraft.yaml'
> --- snapcraft.yaml	2017-02-17 15:56:44 +0000
> +++ snapcraft.yaml	2017-03-15 07:53:24 +0000
> @@ -7,11 +7,31 @@
>    https://code.launchpad.net/media-hub
>  confinement: strict
>  
> +slots:
> +  session-service:
> +    interface: media-hub
> +  mpris-service:
> +    interface: mpris
> +    name: "MediaHub"
> +

I think it would be better:

slots:
  service:
    interface: media-hub
  mpris:
    interface: mpris

So the slots that appear are "media-hub:service" and "media-hub:mpris"

(why was "name" needed btw?)

>  apps:
> -  service:
> -    command: usr/bin/media-hub-server
> -    slots: [mpris]
> -    plugs: [pulseaudio]
> +  media-hub-server:
> +    command: desktop-launch ${SNAP}/usr/bin/media-hub-server
> +    daemon: simple
> +    slots: [session-service]

This should be [service mpris]

> +    plugs: [home, pulseaudio, platform]
> +#ad-hoc until daemon runs as user session

Do we have a snapd bug that we can refer from here? If there is none, we should create one and link here.

> +  media-hub-service:
> +    command: desktop-launch ${SNAP}/usr/bin/media-hub-server
> +    slots: [session-service]

This should be [service mpris]

> +    plugs: [home, pulseaudio, platform]
> +
> +plugs:
> +  platform:
> +    interface: content
> +    content: ubuntu-app-platform1
> +    target:  ubuntu-app-platform
> +    default-provider: ubuntu-app-platform

Please move the definition of plugs right after the definition of slots.

>  
>  parts:
>    service:
> @@ -49,7 +69,15 @@
>      stage-packages:
>        # Ok to ship by default since Ubuntu has an mp3 license
>        - gstreamer1.0-fluendo-mp3
> -
> +      - gstreamer1.0-libav
> +      - gstreamer1.0-plugins-bad-faad
> +      - gstreamer1.0-plugins-bad-videoparsers

Please also add gstreamer1.0-plugins-bad

> +      - gstreamer1.0-plugins-good
> +      - gstreamer1.0-plugins-ugly-amr

Are we sure we have licenses for AMR? @Jim? iirc we had to split ugly to include AMR in the phone, but that the phones had licenses does not imply the same for desktop.

> +      - gstreamer1.0-pulseaudio
> +      - xdg-user-dirs
> +      - mime-support
> +      - shared-mime-info
>      configflags:
>        - -DCMAKE_INSTALL_PREFIX:PATH=/usr
>        - -DCMAKE_LIBRARY_PATH=/usr/lib
> @@ -75,6 +101,11 @@
>          - -usr/lib/*.la
>          - -usr/lib/*/*.la
>          - -usr/lib/*/*.o
> +    after:
> +      - desktop-ubuntu-app-platform

Is this needed?

>  
>      snap:
>        - $unwanted
> +  environment:
> +    source: snap/
> +    plugin: dump

Please rename this part, I find "environment" confusing especially as now there is a key with the same name that can be used when defining the apps. Something like "overlay" might do. Also, add a line separation before.

> 
> === modified file 'src/core/media/apparmor/ubuntu.cpp'
> --- src/core/media/apparmor/ubuntu.cpp	2016-04-06 15:42:15 +0000
> +++ src/core/media/apparmor/ubuntu.cpp	2017-03-15 07:53:24 +0000
> @@ -74,7 +74,10 @@
>  static constexpr std::size_t index_package{1};
>  static constexpr std::size_t index_app{2};
>  static const std::string unity_name{"unity8-dash"};
> +static const std::string unity8_snap_name{"snap.unity8-session.unity8-session"};
>  
> +// adhoc for mediaplayer-app until it settles down with proper handling

Please add a link to the bug

> +static const std::string mediaplayer_snap_name{"snap.mediaplayer-app.mediaplayer-app"};
>  // Returns true if the context name is a valid Ubuntu app id.
>  // If it is, out is populated with the package and app name.
>  bool process_context_name(const std::string& s, std::smatch& out,
> @@ -116,6 +119,7 @@
>          {
>              "apparmor::ubuntu::Context: Invalid profile name " + str()
>          };
> +

Remove this blank line

>  }
>  
>  bool apparmor::ubuntu::Context::is_unconfined() const
> @@ -207,7 +211,7 @@
>      // TODO: when the trust store lands, check it to see if this app can access the dirs and
>      // then remove the explicit whitelist of the music-app, and gallery-app
>      else if ((context.package_name() == "com.ubuntu.music" || context.package_name() == "com.ubuntu.gallery" ||
> -              context.profile_name() == unity_name) &&
> +              context.profile_name() == unity_name || context.profile_name() == unity8_snap_name || context.profile_name() == mediaplayer_snap_name) &&

Please try to keep lines shorter, max 100 chars

>              (parsed_uri.path.find(std::string("Music/")) != std::string::npos ||
>               parsed_uri.path.find(std::string("Videos/")) != std::string::npos ||
>               parsed_uri.path.find(std::string("/media")) != std::string::npos))


-- 
https://code.launchpad.net/~chunsang/media-hub/wip_media-hub-interface/+merge/319896
Your team Ubuntu Phablet Team is subscribed to branch lp:media-hub.