Merge lp:~jamesh/mediascanner/respect-privacy-setting into lp:mediascanner

[James Henstridge]

Respect the Unity dash "remote-content-search" privacy setting within the media scanner by disabling additional metadata resolvers and album art download.

Respect the Unity dash "remote-content-search" privacy setting within the media scanner by disabling additional metadata resolvers and album art download.

I know that creating additional instances of the Settings class inside those two bits of code isn't the cleanest solution, but the code base isn't really set up to provide access to the one set up by mediascanner-service's main routine, and we want to make sure this code reacts to changes in the setting (something that isn't hooked up for any of the media scanner's other settings).

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:398
http://jenkins.qa.ubuntu.com/job/mediascanner-ci/79/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-amd64-ci/79/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-armhf-ci/79/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-i386-ci/79/console

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mediascanner-ci/79/rebuild

review: Needs Fixing (continuous-integration)

[lp:~jamesh/mediascanner/respect-privacy-setting]

399. By James Henstridge on 2013-10-15

Ignore warnings about missing com.canonical.Unity.Lenses schema in the
file system scanner tests: it is not a hard dependency, and the package
it belongs to is not a build dependency.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:399
http://jenkins.qa.ubuntu.com/job/mediascanner-ci/80/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-amd64-ci/80
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-armhf-ci/80
        deb: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-armhf-ci/80/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mediascanner-saucy-i386-ci/80

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mediascanner-ci/80/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

remote_content_search is a bit ambiguous. Could it be renamed should_search_remote or something like that?

The lookup function has this:

if (not settings)
 return T();

but its return value is checked like this:

lookup(unity_lens_settings_, kRemoteContentSearch) != "none";

So if settings evaluates to false, empty string is returned and this comparison returns true. Is this what should happen?

review: Needs Fixing

[James Henstridge (jamesh)]

The intent is to search remote content by default, and only disable it if the Unity GSettings schema exists and the key has been toggled. Here's the definition of the key that is being checked here:

    <key type="s" name="remote-content-search">
      <default>'all'</default>
      <choices><choice value='all'/><choice value='none'/></choices>
      <summary>Content fetching from remote source preference.</summary>
      <description>"all" is to enable the supported default lens to search from remote and commercial sources. "none" will indicate the lenses to not perform that remote search at all.</description>
    </key>

So I think the '!= "none"` check is appropriate, treating "all" and "" as equivalent.

[Jussi Pakkanen (jpakkane)]

That's all right then. What about the renaming? I'm not super strict about it, so if you feel it does not need to be done, then we can just merge this.

Unmerged revisions

399. By James Henstridge on 2013-10-15

Ignore warnings about missing com.canonical.Unity.Lenses schema in the
file system scanner tests: it is not a hard dependency, and the package
it belongs to is not a build dependency.

398. By James Henstridge on 2013-10-15

Disable metadata resolvers and album art download when remote content
search has been disabled.

397. By James Henstridge on 2013-10-15

Add accessor for Unity remote-content-search setting.

=== modified file 'src/mediascanner/mediaartdownloader.cpp'
--- src/mediascanner/mediaartdownloader.cpp	2013-10-08 14:44:18 +0000
+++ src/mediascanner/mediaartdownloader.cpp	2013-10-15 13:14:24 +0000
@@ -22,6 +22,7 @@
 #include "mediascanner/glibutils.h"
 #include "mediascanner/mediaartcache.h"
 #include "mediascanner/mediaartdownloader.h"
+#include "mediascanner/settings.h"
 
 #define ALBUM_ART_PLUGIN "grl-lastfm-albumart"
 #define ALBUM_ART_SOURCE "grl-lastfm-albumart"
@@ -71,6 +72,7 @@
     MediaArtCache cache;
     Wrapper<GrlSource> art_resolver;
     Wrapper<SoupSession> session;
+    Settings settings;
 
     std::vector<ResolveDataPtr> jobs;
 
@@ -153,6 +155,9 @@
     if (not d->art_resolver || not d->session)
         return;
 
+    if (not d->settings.remote_content_search())
+        return;
+
     kInfo("Resolving art for {1} - {2}") % artist % album;
 
     if (d->cache.has_art(artist, album)) {

=== modified file 'src/mediascanner/metadataresolver.cpp'
--- src/mediascanner/metadataresolver.cpp	2013-10-08 14:44:18 +0000
+++ src/mediascanner/metadataresolver.cpp	2013-10-15 13:14:24 +0000
@@ -35,6 +35,7 @@
 #include "mediascanner/logging.h"
 #include "mediascanner/mediautils.h"
 #include "mediascanner/propertyschema.h"
+#include "mediascanner/settings.h"
 #include "mediascanner/utilities.h"
 
 namespace mediascanner {
@@ -157,6 +158,7 @@
                                  GrlMedia *media, void *user_data,
                                  const GError *error);
 
+    Settings settings_;
     std::vector<MetadataSource> available_sources_;
     Wrapper<GrlOperationOptions> options_;
     std::vector<unsigned> current_ops_;
@@ -272,7 +274,8 @@
 
     g_mutex_lock(&mutex_);
 
-    resolve_next(media.get(), data);
+    if (settings_.remote_content_search())
+        resolve_next(media.get(), data);
 
     const size_t removals = pending_pushes_.erase(push_id);
     BOOST_ASSERT(removals == 1);

=== modified file 'src/mediascanner/settings.cpp'
--- src/mediascanner/settings.cpp	2013-09-11 09:49:12 +0000
+++ src/mediascanner/settings.cpp	2013-10-15 13:14:24 +0000
@@ -33,6 +33,8 @@
 #include "mediascanner/logging.h"
 #include "mediascanner/utilities.h"
 
+#define UNITY_LENS_SCHEMA "com.canonical.Unity.Lenses"
+
 namespace mediascanner {
 
 // Boost C++
@@ -50,6 +52,8 @@
 Settings::kMetadataSources("metadata-sources");
 const Settings::Key<Settings::StringList>
 Settings::kMediaRoots("media-roots");
+const Settings::Key<std::string>
+Settings::kRemoteContentSearch("remote-content-search");
 
 // FIXME(M5): Apparently "dbus" is not the proper namespace for this things
 namespace dbus {
@@ -89,12 +93,27 @@
 // TODO(M4): Handle change notifications
 Settings::Settings()
     : settings_(take(g_settings_new(MEDIASCANNER_SETTINGS_ID))) {
+    auto schemas = g_settings_list_schemas();
+    if (schemas) {
+        for (int i = 0; schemas[i]; i++) {
+            if (g_strcmp0(schemas[i], UNITY_LENS_SCHEMA) == 0) {
+                unity_lens_settings_ = g_settings_new(UNITY_LENS_SCHEMA);
+                break;
+            }
+        }
+    }
+    if (not unity_lens_settings_) {
+        kWarning("Missing {1} schema") % UNITY_LENS_SCHEMA;
+    }
 }
 
 template<typename T>
-T Settings::lookup(const Key<T> &key) const {
+T Settings::lookup(Wrapper<GSettings> settings, const Key<T> &key) const {
+    if (not settings)
+        return T();
+
     if (const Wrapper<GVariant> settings_value =
-            take(g_settings_get_value(settings_.get(), key.name_.c_str())))
+            take(g_settings_get_value(settings.get(), key.name_.c_str())))
         return dbus::Type<T>::make_value(settings_value.get());
 
     return T();
@@ -117,19 +136,23 @@
 }
 
 Settings::MediaFormatList Settings::mandatory_containers() const {
-    return lookup(kMandatoryContainers);
+    return lookup(settings_, kMandatoryContainers);
 }
 
 Settings::MediaFormatList Settings::mandatory_decoders() const {
-    return lookup(kMandatoryDecoders);
+    return lookup(settings_, kMandatoryDecoders);
 }
 
 Settings::MetadataSourceList Settings::metadata_sources() const {
-    return lookup(kMetadataSources);
+    return lookup(settings_, kMetadataSources);
 }
 
 Settings::StringList Settings::media_root_urls() const {
-    return lookup(kMediaRoots);
+    return lookup(settings_, kMediaRoots);
+}
+
+bool Settings::remote_content_search() const {
+    return lookup(unity_lens_settings_, kRemoteContentSearch) != "none";
 }
 
 static std::string user_dir_url(GUserDirectory dir) {

=== modified file 'src/mediascanner/settings.h'
--- src/mediascanner/settings.h	2013-09-10 14:02:38 +0000
+++ src/mediascanner/settings.h	2013-10-15 13:14:24 +0000
@@ -99,6 +99,7 @@
     static const Key<MediaFormatList> kMandatoryDecoders;
     static const Key<MetadataSourceList> kMetadataSources;
     static const Key<StringList> kMediaRoots;
+    static const Key<std::string> kRemoteContentSearch;
 
     Settings();
 
@@ -108,8 +109,7 @@
     StringList media_root_urls() const;
     StringList media_root_paths() const;
 
-    template<typename T>
-    T lookup(const Key<T> &key) const;
+    bool remote_content_search() const;
 
     unsigned connect(const KeyName &key, const ChangeListener &listener) const;
     void disconnect(unsigned handler_id) const;
@@ -120,6 +120,10 @@
 
 private:
     Wrapper<GSettings> settings_;
+    Wrapper<GSettings> unity_lens_settings_;
+
+    template<typename T>
+    T lookup(Wrapper<GSettings> settings, const Key<T> &key) const;
 };
 
 } // namespace mediascanner

=== modified file 'tests/auto/filesystemscannertest.cpp'
--- tests/auto/filesystemscannertest.cpp	2013-10-14 07:51:05 +0000
+++ tests/auto/filesystemscannertest.cpp	2013-10-15 13:14:24 +0000
@@ -162,6 +162,8 @@
     LoggingSink sink(logging::warning());
     sink.ignore("warning/roots", "Cannot retrieve information about.*");
     sink.ignore("warning/roots", "Cannot identify mount point.*");
+    sink.ignore("warning/settings",
+                "Missing com.canonical.Unity.Lenses schema");
 
     const MediaRootManagerPtr root_manager(new MediaRootManager);
     root_manager->set_enabled(false);
@@ -276,6 +278,8 @@
     LoggingSink sink(logging::warning());
     sink.ignore("warning/roots", "Cannot retrieve information about.*");
     sink.ignore("warning/roots", "Cannot identify mount point.*");
+    sink.ignore("warning/settings",
+                "Missing com.canonical.Unity.Lenses schema");
 
     const MediaRootManagerPtr root_manager(new MediaRootManager);
     root_manager->set_enabled(false);