Merge lp:~jpakkane/mediascanner/immutable-mediafile into lp:mediascanner/v2

[Jussi Pakkanen]

Made MediaFile immutable.

Made MediaFile immutable. This is done by creating a new MediaFileBuilder class that you can use to incrementally store MediaFile properties. MediaFile can then be built with one atomic step. This ensures no half-initialised MediaFiles can escape into the wild.

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mediascanner-v2-ci/5/rebuild

review: Approve (continuous-integration)

[lp:~jpakkane/mediascanner/immutable-mediafile]

191. By Jussi Pakkanen on 2013-11-28

Merged trunk.

192. By Jussi Pakkanen on 2013-11-28

Update tests.

193. By Jussi Pakkanen on 2013-11-28

Capitalisation fix.

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mediascanner-v2-ci/6/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mediascanner-v2-ci/7/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

Is there any reason why you removed MediaFile.addUri()? I added it because I was using it in the scope.

review: Needs Information

[James Henstridge (jamesh)]

Also, as far as API designs go, what do you think of the one described here?

http://www.parashift.com/c++-faq/named-parameter-idiom.html

[lp:~jpakkane/mediascanner/immutable-mediafile]

194. By Jussi Pakkanen on 2013-11-29

Removed unnecessary includes.

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mediascanner-v2-ci/8/rebuild

review: Approve (continuous-integration)

[Jussi Pakkanen (jpakkane)]

MediaFileBuilder is already almost exactly that. It would just need to change the setXXX functions to return reference to *this. I can make that change if it is deemed useful.

The reason I moved the Uri function (I assume you meant getUri instead of addUri) is that after changing to mediafilebuilder the uri function was used only in one place (gstreamer pipeline building) and at that point a MediaFile had not been created yet.

If it is necessary I guess I can put it back, but since Mediascanner only deals with local files, it seems cleaner to me to only deal with filenames.

[James Henstridge (jamesh)]

We needed URIs in the daemon internals, and we needed URIs in some users of the media scanner (the scope is an obvious example, Drag and Drop also deals with URIs, etc).

It's not clear that having the method makes the interface more difficult to understand, so I'd prefer to keep it unless there is a good reason for it to go.

[lp:~jpakkane/mediascanner/immutable-mediafile]

195. By Jussi Pakkanen on 2013-12-02

Return of getUri.

[PS Jenkins bot (ps-jenkins)]

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

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mediascanner-v2-ci/10/rebuild

review: Approve (continuous-integration)

[James Henstridge (jamesh)]

[1]
Since the aim of this branch is to make MediaFile act like a value type, would it make sense to declare its data members to be const? That would ensure that the the MediaFile code itself doesn't start mutating the instance.

[2]
Since the big long argument list for the MediaFile constructor is a problem, would it be worth getting rid of it all together? At present, it is only being used in (a) MediaStore.make_media() and (b) tests, which could both be converted to MediaFileBuilder. This would also mean those pieces of code wouldn't immediately need updating whenever a new data member is added to MediaFile.

If we followed the example from the C++ FAQ, we could have a MediaFile constructor taking a Builder instance.

[3]
I'm on the fence on this one, but would it make sense for the MediaFileBuilder constructor to take the file name directly? This is the one field that is mandatory in a MediaFile, so it doesn't seem so bad to require it when creating the builder.

[Jussi Pakkanen (jpakkane)]

[1] This is something I thought about for a long time. It you make instance variables const, you can't copy them. Meaning you can't do this:

mf = some_func();

This would make Mediafile assigment atomic but not constant. Sometimes this is what you need. But I see the const issue as well. I guess we could start by making them const and unconsting them if necessary.

[2]

I'm a bit on the fence about this. I like being able to explicitly instantiate objects without other helper objects. This is a potential source of bugs, though.

[3]

Yes, change coming up.

[lp:~jpakkane/mediascanner/immutable-mediafile]

196. By Jussi Pakkanen on 2013-12-02

Make MediaFileBuilder take the filename in a constructor because that is mandatory.

197. By Jussi Pakkanen on 2013-12-02

Use initialisers in a constructor.

[James Henstridge (jamesh)]

Looks good. After discussing in person, I think we can clean up the other improvement issues post-merge.

review: Approve

[lp:~jpakkane/mediascanner/immutable-mediafile]

198. By Jussi Pakkanen on 2013-12-02

Fixed initialisation order.

=== modified file 'src/daemon/MetadataExtractor.cc'
--- src/daemon/MetadataExtractor.cc	2013-11-28 04:08:20 +0000
+++ src/daemon/MetadataExtractor.cc	2013-12-02 14:22:54 +0000
@@ -18,6 +18,8 @@
  */
 
 #include "../mediascanner/MediaFile.hh"
+#include "../mediascanner/MediaFileBuilder.hh"
+#include "../mediascanner/utils.hh"
 #include "MetadataExtractor.hh"
 
 #include <glib-object.h>
@@ -32,6 +34,7 @@
 
 using namespace std;
 
+
 struct MetadataExtractorPrivate {
     std::unique_ptr<GstDiscoverer,void(*)(void *)> discoverer;
     MetadataExtractorPrivate() : discoverer(nullptr, g_object_unref) {};
@@ -102,7 +105,7 @@
 
 static void
 extract_tag_info (const GstTagList * list, const gchar * tag, gpointer user_data) {
-    MediaFile *mf = (MediaFile *) user_data;
+    MediaFileBuilder *mfb = (MediaFileBuilder *) user_data;
     int i, num;
     string tagname(tag);
 
@@ -113,23 +116,23 @@
         val = gst_tag_list_get_value_index (list, tag, i);
         if (G_VALUE_HOLDS_STRING(val)) {
             if (tagname == GST_TAG_ARTIST)
-                mf->setAuthor(g_value_get_string(val));
+                mfb->setAuthor(g_value_get_string(val));
             else if (tagname == GST_TAG_TITLE)
-                mf->setTitle(g_value_get_string(val));
+                mfb->setTitle(g_value_get_string(val));
             else if (tagname == GST_TAG_ALBUM)
-                mf->setAlbum(g_value_get_string(val));
+                mfb->setAlbum(g_value_get_string(val));
             else if (tagname == GST_TAG_ALBUM_ARTIST)
-                mf->setAlbumArtist(g_value_get_string(val));
+                mfb->setAlbumArtist(g_value_get_string(val));
         } else if (G_VALUE_HOLDS(val, GST_TYPE_DATE_TIME)) {
             if (tagname == GST_TAG_DATE_TIME) {
                 GstDateTime *dt = static_cast<GstDateTime*>(g_value_get_boxed(val));
                 char *dt_string = gst_date_time_to_iso8601_string(dt);
-                mf->setDate(dt_string);
+                mfb->setDate(dt_string);
                 g_free(dt_string);
             }
         } else if (G_VALUE_HOLDS_UINT(val)) {
             if (tagname == GST_TAG_TRACK_NUMBER) {
-                mf->setTrackNumber(g_value_get_uint(val));
+                mfb->setTrackNumber(g_value_get_uint(val));
             }
         }
 
@@ -137,11 +140,12 @@
 }
 
 MediaFile MetadataExtractor::extract(const DetectedFile &d) {
-    MediaFile mf(d.filename);
-    mf.setETag(d.etag);
-    mf.setContentType(d.content_type);
-    mf.setType(d.type);
-    string uri = mf.getUri();
+    MediaFileBuilder mfb(d.filename);
+    mfb.setETag(d.etag);
+    mfb.setContentType(d.content_type);
+    mfb.setType(d.type);
+    string uri = getUri(d.filename);
+
     GError *error = nullptr;
     unique_ptr<GstDiscovererInfo, void(*)(void *)> info(
         gst_discoverer_discover_uri(p->discoverer.get(), uri.c_str(), &error),
@@ -151,21 +155,21 @@
         g_error_free(error);
 
         string msg = "Discovery of file ";
-        msg += mf.getFileName();
+        msg += d.filename;
         msg += " failed: ";
         msg += errortxt;
         throw runtime_error(msg);
     }
 
     if (gst_discoverer_info_get_result(info.get()) != GST_DISCOVERER_OK) {
-        throw runtime_error("Unable to discover file " + mf.getFileName());
+        throw runtime_error("Unable to discover file " + d.filename);
     }
 
     const GstTagList *tags = gst_discoverer_info_get_tags(info.get());
     if (tags != NULL) {
-        gst_tag_list_foreach(tags, extract_tag_info, &mf);
+        gst_tag_list_foreach(tags, extract_tag_info, &mfb);
     }
-    mf.setDuration(static_cast<int>(
+    mfb.setDuration(static_cast<int>(
         gst_discoverer_info_get_duration(info.get())/GST_SECOND));
-    return mf;
+    return mfb.build();
 }

=== modified file 'src/mediascanner/CMakeLists.txt'
--- src/mediascanner/CMakeLists.txt	2013-11-14 15:04:14 +0000
+++ src/mediascanner/CMakeLists.txt	2013-12-02 14:22:54 +0000
@@ -1,5 +1,6 @@
 add_library(mediascanner SHARED
   MediaFile.cc
+  MediaFileBuilder.cc
   Album.cc
   MediaStore.cc
   utils.cc

=== modified file 'src/mediascanner/MediaFile.cc'
--- src/mediascanner/MediaFile.cc	2013-11-25 07:53:45 +0000
+++ src/mediascanner/MediaFile.cc	2013-12-02 14:22:54 +0000
@@ -17,9 +17,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-#include <stdexcept>
-#include <glib.h>
 #include "MediaFile.hh"
+#include "utils.hh"
 
 using namespace std;
 
@@ -73,44 +72,8 @@
     return type;
 }
 
-void MediaFile::setContentType(const std::string &content_type) noexcept {
-    this->content_type = content_type;
-}
-
-void MediaFile::setETag(const std::string &etag) noexcept {
-    this->etag = etag;
-}
-
-void MediaFile::setTitle(const std::string &title) noexcept {
-    this->title = title;
-}
-
-void MediaFile::setAuthor(const std::string &author) noexcept {
-    this->author = author;
-}
-
-void MediaFile::setAlbum(const std::string &album) noexcept {
-    this->album = album;
-}
-
-void MediaFile::setAlbumArtist(const std::string &album_artist) noexcept {
-    this->album_artist = album_artist;
-}
-
-void MediaFile::setDate(const std::string &date) noexcept {
-    this->date = date;
-}
-
-void MediaFile::setTrackNumber(int track_number) noexcept {
-    this->track_number = track_number;
-}
-
-void MediaFile::setDuration(int duration) noexcept {
-    this->duration = duration;
-}
-
-void MediaFile::setType(MediaType type) noexcept {
-    this->type = type;
+std::string MediaFile::getUri() const {
+    return ::getUri(filename);
 }
 
 bool MediaFile::operator==(const MediaFile &other) const {
@@ -132,16 +95,3 @@
     return !(*this == other);
 }
 
-std::string MediaFile::getUri() const {
-    GError *error = NULL;
-    char *uristr = g_filename_to_uri(filename.c_str(), "", &error);
-    if (error) {
-        string msg("Could not build URI: ");
-        msg += error->message;
-        g_error_free(error);
-        throw runtime_error(msg);
-    }
-    string uri(uristr);
-    g_free(uristr);
-    return uri;
-}

=== modified file 'src/mediascanner/MediaFile.hh'
--- src/mediascanner/MediaFile.hh	2013-11-25 07:53:45 +0000
+++ src/mediascanner/MediaFile.hh	2013-12-02 14:22:54 +0000
@@ -40,24 +40,16 @@
     const std::string& getAlbum() const noexcept;
     const std::string& getAlbumArtist() const noexcept;
     const std::string& getDate() const noexcept;
+    std::string getUri() const;
+
     int getTrackNumber() const noexcept;
     int getDuration() const noexcept;
     MediaType getType() const noexcept;
     bool operator==(const MediaFile &other) const;
     bool operator!=(const MediaFile &other) const;
 
-    void setContentType(const std::string& content_type) noexcept;
-    void setETag(const std::string& etag) noexcept;
-    void setTitle(const std::string& title) noexcept;
-    void setAuthor(const std::string& author) noexcept;
-    void setAlbum(const std::string& album) noexcept;
-    void setAlbumArtist(const std::string& album_artist) noexcept;
-    void setDate(const std::string& date) noexcept;
-    void setTrackNumber(int track_number) noexcept;
-    void setDuration(int duration) noexcept;
-    void setType(MediaType type) noexcept;
-
-    std::string getUri() const;
+    // There are no setters. MediaFiles are immutable.
+    // For piecewise construction use MediaFileBuilder.
 
 private:
     std::string filename;

=== added file 'src/mediascanner/MediaFileBuilder.cc'
--- src/mediascanner/MediaFileBuilder.cc	1970-01-01 00:00:00 +0000
+++ src/mediascanner/MediaFileBuilder.cc	2013-12-02 14:22:54 +0000
@@ -0,0 +1,116 @@
+/*
+ * Copyright (C) 2013 Canonical, Ltd.
+ *
+ * Authors:
+ *    Jussi Pakkanen <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of version 3 of the GNU General Public License as published
+ * by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include"MediaFileBuilder.hh"
+#include"MediaFile.hh"
+#include<stdexcept>
+
+MediaFileBuilder::MediaFileBuilder(const std::string &fname) {
+    filename = fname;
+}
+
+MediaFileBuilder::MediaFileBuilder(const MediaFile &mf) :
+    type(mf.getType()),
+    filename(mf.getFileName()),
+    content_type(mf.getContentType()),
+    etag(mf.getETag()),
+    title(mf.getTitle()),
+    date(mf.getDate()),
+    author(mf.getAuthor()),
+    album(mf.getAlbum()),
+    album_artist(mf.getAlbumArtist()),
+    track_number(mf.getTrackNumber()),
+    duration(mf.getDuration()) {
+}
+
+MediaFile MediaFileBuilder::build() const {
+    return MediaFile(filename, content_type, etag, title, date, author,
+            album, album_artist, track_number, duration, type);
+}
+
+void MediaFileBuilder::setType(MediaType t) {
+    if(type_set)
+        throw std::invalid_argument("Tried to set type when it was already set.");
+    type = t;
+    type_set = true;
+}
+
+void MediaFileBuilder::setETag(const std::string &e) {
+    if(etag_set)
+        throw std::invalid_argument("Tried to set filename when it was already set.");
+    etag = e;
+    etag_set = true;
+
+}
+void MediaFileBuilder::setContentType(const std::string &c) {
+    if(content_type_set)
+        throw std::invalid_argument("Tried to set filename when it was already set.");
+    content_type = c;
+    content_type_set = true;
+
+}
+
+void MediaFileBuilder::setTitle(const std::string &t) {
+    if(title_set)
+        throw std::invalid_argument("Tried to set title when it was already set.");
+    title = t;
+    title_set = true;
+}
+
+void MediaFileBuilder::setDate(const std::string &d) {
+    if(date_set)
+        throw std::invalid_argument("Tried to set date when it was already set.");
+    date = d;
+    date_set = true;
+}
+
+void MediaFileBuilder::setAuthor(const std::string &a) {
+    if(author_set)
+        throw std::invalid_argument("Tried to set author when it was already set.");
+    author = a;
+    author_set = true;
+}
+
+void MediaFileBuilder::setAlbum(const std::string &a) {
+    if(album_set)
+        throw std::invalid_argument("Tried to set album when it was already set.");
+    album = a;
+    album_set = true;
+}
+
+void MediaFileBuilder::setAlbumArtist(const std::string &a) {
+    if(album_artist_set)
+        throw std::invalid_argument("Tried to set album artist when it was already set.");
+    album_artist = a;
+    album_artist_set = true;
+}
+
+void MediaFileBuilder::setTrackNumber(int n) {
+    if(track_number_set)
+        throw std::invalid_argument("Tried to set track number when it was already set.");
+    track_number = n;
+    track_number_set = true;
+}
+
+void MediaFileBuilder::setDuration(int n) {
+    if(duration_set)
+        throw std::invalid_argument("Tried to set duration when it was already set.");
+    duration = n;
+    duration_set = true;
+}

=== added file 'src/mediascanner/MediaFileBuilder.hh'
--- src/mediascanner/MediaFileBuilder.hh	1970-01-01 00:00:00 +0000
+++ src/mediascanner/MediaFileBuilder.hh	2013-12-02 14:22:54 +0000
@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2013 Canonical, Ltd.
+ *
+ * Authors:
+ *    Jussi Pakkanen <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of version 3 of the GNU General Public License as published
+ * by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef MEDIAFILEBUILDER_H_
+#define MEDIAFILEBUILDER_H_
+
+#include"scannercore.hh"
+#include<string>
+
+class MediaFile;
+
+class MediaFileBuilder final {
+public:
+    MediaFileBuilder(const std::string &filename);
+    MediaFileBuilder(const MediaFile &mf);
+    MediaFileBuilder(const MediaFileBuilder &) = delete;
+    MediaFileBuilder& operator=(MediaFileBuilder &) = delete;
+
+    MediaFile build() const;
+
+    void setType(MediaType t);
+    void setETag(const std::string &e);
+    void setContentType(const std::string &c);
+    void setTitle(const std::string &t);
+    void setDate(const std::string &d);
+    void setAuthor(const std::string &a);
+    void setAlbum(const std::string &a);
+    void setAlbumArtist(const std::string &a);
+    void setTrackNumber(int n);
+    void setDuration(int d);
+
+private:
+    bool type_set = false;
+    MediaType type = UnknownMedia;
+
+    std::string filename;
+
+    bool content_type_set = false;
+    std::string content_type;
+
+    bool etag_set = false;
+    std::string etag;
+
+    bool title_set = false;
+    std::string title;
+
+    bool date_set = false;
+    std::string date;
+
+    bool author_set = false;
+    std::string author;
+
+    bool album_set = false;
+    std::string album;
+
+    bool album_artist_set = false;
+    std::string album_artist;
+
+    int track_number = 0;
+    bool track_number_set = false;
+
+    int duration = 0;
+    bool duration_set = false;
+};
+
+
+#endif

=== modified file 'src/mediascanner/utils.cc'
--- src/mediascanner/utils.cc	2013-11-12 04:35:16 +0000
+++ src/mediascanner/utils.cc	2013-12-02 14:22:54 +0000
@@ -20,6 +20,8 @@
 #include"utils.hh"
 
 #include<vector>
+#include<stdexcept>
+#include<glib.h>
 
 std::string sqlQuote(const std::string &input) {
     std::vector<char> out;
@@ -67,3 +69,19 @@
     }
     return result;
 }
+
+std::string getUri(const std::string &filename) {
+    GError *error = NULL;
+    char *uristr = g_filename_to_uri(filename.c_str(), "", &error);
+    if (error) {
+        std::string msg("Could not build URI: ");
+        msg += error->message;
+        g_error_free(error);
+        throw std::runtime_error(msg);
+    }
+
+    std::string uri(uristr);
+    g_free(uristr);
+    return uri;
+}
+

=== modified file 'src/mediascanner/utils.hh'
--- src/mediascanner/utils.hh	2013-11-12 04:35:16 +0000
+++ src/mediascanner/utils.hh	2013-12-02 14:22:54 +0000
@@ -24,5 +24,6 @@
 
 std::string sqlQuote(const std::string &input);
 std::string filenameToTitle(const std::string &filename);
+std::string getUri(const std::string &filename);
 
 #endif

=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt	2013-11-18 10:23:49 +0000
+++ test/CMakeLists.txt	2013-12-02 14:22:54 +0000
@@ -38,3 +38,7 @@
 add_executable(test_sqliteutils test_sqliteutils.cc)
 target_link_libraries(test_sqliteutils gtest ${MEDIASCANNER_LIBRARIES})
 add_test(test_sqliteutils test_sqliteutils)
+
+add_executable(test_mfbuilder test_mfbuilder.cc)
+target_link_libraries(test_mfbuilder gtest mediascanner)
+add_test(test_mfbuilder test_mfbuilder)

=== modified file 'test/basic.cc'
--- test/basic.cc	2013-11-28 04:08:20 +0000
+++ test/basic.cc	2013-12-02 14:22:54 +0000
@@ -18,6 +18,7 @@
  */
 
 #include <mediascanner/MediaFile.hh>
+#include <mediascanner/MediaFileBuilder.hh>
 #include <mediascanner/MediaStore.hh>
 #include <daemon/MetadataExtractor.hh>
 #include <daemon/SubtreeWatcher.hh>
@@ -181,8 +182,9 @@
     MediaFile media = store.lookup(outfile);
     EXPECT_NE(media.getETag(), "");
     EXPECT_EQ(media.getTitle(), "track1");
-    media.setTitle("something else");
-    store.insert(media);
+    MediaFileBuilder mfb(media);
+    mfb.setTitle("something else");
+    store.insert(mfb.build());
 
     /* Scan again, and note that the data hasn't been updated */
     scanFiles(store, testdir, AudioMedia);
@@ -190,8 +192,9 @@
     EXPECT_EQ(media.getTitle(), "something else");
 
     /* Now change the stored etag, to trigger an update */
-    media.setETag("old-etag");
-    store.insert(media);
+    MediaFileBuilder mfb2(media);
+    mfb2.setETag("old-etag");
+    store.insert(mfb2.build());
     scanFiles(store, testdir, AudioMedia);
     media = store.lookup(outfile);
     EXPECT_EQ(media.getTitle(), "track1");

=== modified file 'test/test_mediastore.cc'
--- test/test_mediastore.cc	2013-11-28 03:54:39 +0000
+++ test/test_mediastore.cc	2013-12-02 14:22:54 +0000
@@ -47,6 +47,10 @@
 TEST_F(MediaStoreTest, init) {
     MediaStore store(":memory:", MS_READ_WRITE);
 }
+TEST_F(MediaStoreTest, mediafile_uri) {
+    MediaFile media("/path/to/file.ogg");
+    EXPECT_EQ(media.getUri(), "file:///path/to/file.ogg");
+}
 
 TEST_F(MediaStoreTest, equality) {
     MediaFile audio1("a", "type", "etag", "1900", "b", "c", "d", "e", 1, 5, AudioMedia);
@@ -64,11 +68,6 @@
     EXPECT_NE(audio2, video2);
 }
 
-TEST_F(MediaStoreTest, mediafile_uri) {
-    MediaFile media("/path/to/file.ogg");
-    EXPECT_EQ(media.getUri(), "file:///path/to/file.ogg");
-}
-
 TEST_F(MediaStoreTest, lookup) {
     MediaFile audio("aaa", "type", "etag", "bbb bbb", "1900-01-01", "ccc", "ddd", "eee", 3, 5, AudioMedia);
     MediaStore store(":memory:", MS_READ_WRITE);

=== added file 'test/test_mfbuilder.cc'
--- test/test_mfbuilder.cc	1970-01-01 00:00:00 +0000
+++ test/test_mfbuilder.cc	2013-12-02 14:22:54 +0000
@@ -0,0 +1,120 @@
+/*
+ * Copyright (C) 2013 Canonical, Ltd.
+ *
+ * Authors:
+ *    Jussi Pakkanen <[e-mail]>
+ *
+ * This library is free software; you can redistribute it and/or modify it under
+ * the terms of version 3 of the GNU General Public License as published
+ * by the Free Software Foundation.
+ *
+ * This library is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include<gtest/gtest.h>
+#include<stdexcept>
+#include"mediascanner/MediaFile.hh"
+#include"mediascanner/MediaFileBuilder.hh"
+
+/**
+ * This is a helper class to build MediaFiles. Since we want them
+ * to be immutable and always valid, the user needs to always list
+ * all variables in the constructor. This is cumbersome so this class
+ * allows you to gather them one by one and then finally construct
+ * a fully valid MediaFile.
+ *
+ * If you try to assign the same property twice, an exception is thrown.
+ * This means that MediaFileBuilders are meant to build only one
+ * MediaFile. To build a new one create a new MediaFileBuilder. This is to
+ * ensure that no state leaks from the first MediaFile to the second.
+ */
+
+class MFBTest : public ::testing::Test {
+ protected:
+  MFBTest() {
+  }
+
+  virtual ~MFBTest() {
+  }
+
+  virtual void SetUp() {
+  }
+
+  virtual void TearDown() {
+  }
+};
+
+TEST_F(MFBTest, basic) {
+    MediaType type(AudioMedia);
+    std::string fname("abc");
+    std::string title("def");
+    std::string date("ghi");
+    std::string author("jkl");
+    std::string album("mno");
+    std::string album_artist("pqr");
+    std::string etag("stu");
+    std::string content_type("vwx");
+    int track_number = 13;
+    int duration = 99;
+
+    MediaFileBuilder b(fname);
+
+    b.setType(type);
+    ASSERT_THROW(b.setType(type), std::invalid_argument);
+
+    b.setTitle(title);
+    ASSERT_THROW(b.setTitle(fname), std::invalid_argument);
+
+    b.setDate(date);
+    ASSERT_THROW(b.setDate(date), std::invalid_argument);
+
+    b.setAuthor(author);
+    ASSERT_THROW(b.setAuthor(author), std::invalid_argument);
+
+    b.setAlbum(album);
+    ASSERT_THROW(b.setAlbum(album), std::invalid_argument);
+
+    b.setAlbumArtist(album_artist);
+    ASSERT_THROW(b.setAlbumArtist(album_artist), std::invalid_argument);
+
+    b.setTrackNumber(track_number);
+    ASSERT_THROW(b.setTrackNumber(track_number), std::invalid_argument);
+
+    b.setDuration(duration);
+    ASSERT_THROW(b.setDuration(duration), std::invalid_argument);
+
+    b.setETag(etag);
+    ASSERT_THROW(b.setETag(etag), std::invalid_argument);
+
+    b.setContentType(content_type);
+    ASSERT_THROW(b.setContentType(content_type), std::invalid_argument);
+
+    // Now see if data survives a round trip.
+    MediaFile mf = b.build();
+    ASSERT_EQ(mf.getType(), type);
+    ASSERT_EQ(mf.getFileName(), fname);
+    ASSERT_EQ(mf.getTitle(), title);
+    ASSERT_EQ(mf.getDate(), date);
+    ASSERT_EQ(mf.getAuthor(), author);
+    ASSERT_EQ(mf.getAlbum(), album);
+    ASSERT_EQ(mf.getAlbumArtist(), album_artist);
+    ASSERT_EQ(mf.getTrackNumber(), track_number);
+    ASSERT_EQ(mf.getDuration(), duration);
+    ASSERT_EQ(mf.getETag(), etag);
+    ASSERT_EQ(mf.getContentType(), content_type);
+
+    MediaFileBuilder mfb2(mf);
+    MediaFile mf2 = mfb2.build();
+    ASSERT_EQ(mf, mf2);
+}
+
+int main(int argc, char **argv) {
+    ::testing::InitGoogleTest(&argc, argv);
+    return RUN_ALL_TESTS();
+}