fs: Return files & directories as shared_ptr rather than strings

include/filesystem/IDirectory.h

@@ -38,9 +38,9 @@ namespace fs
         // Returns the absolute path to this directory
         virtual const std::string& path() const = 0;
         /// Returns a list of absolute files path
-        virtual const std::vector<std::string>& files() const = 0;
+        virtual const std::vector<std::shared_ptr<IFile>>& files() const = 0;
         /// Returns a list of absolute path to this folder subdirectories
-        virtual const std::vector<std::string>& dirs() const = 0;
+        virtual const std::vector<std::shared_ptr<IDirectory>>& dirs() const = 0;
         virtual std::shared_ptr<IDevice> device() const = 0;
     };
 }

src/discoverer/FsDiscoverer.cpp

@@ -139,14 +139,10 @@ void FsDiscoverer::checkFolder( fs::IDirectory& currentFolderFs, Folder& current
     // Load the folders we already know of:
     LOG_INFO( "Checking for modifications in ", currentFolderFs.path() );
     auto subFoldersInDB = Folder::fetchAll( m_dbConn, currentFolder.id() );
-    for ( const auto& subFolderPath : currentFolderFs.dirs() )
+    for ( const auto& subFolder : currentFolderFs.dirs() )
     {
-        auto subFolder = m_fsFactory->createDirectory( subFolderPath );
-        if ( subFolder == nullptr )
-            continue;
-
-        auto it = std::find_if( begin( subFoldersInDB ), end( subFoldersInDB ), [subFolderPath](const std::shared_ptr<Folder>& f) {
-            return f->path() == subFolderPath;
+        auto it = std::find_if( begin( subFoldersInDB ), end( subFoldersInDB ), [&subFolder](const std::shared_ptr<Folder>& f) {
+            return f->path() == subFolder->path();
         });
         // We don't know this folder, it's a new one
         if ( it == end( subFoldersInDB ) )
@@ -154,7 +150,7 @@ void FsDiscoverer::checkFolder( fs::IDirectory& currentFolderFs, Folder& current
             // Check if it is blacklisted
             if ( isBlacklisted( *subFolder, blacklist ) == true )
             {
-                LOG_INFO( "Ignoring blacklisted folder: ", subFolderPath );
+                LOG_INFO( "Ignoring blacklisted folder: ", subFolder->path() );
                 continue;
             }
             if ( hasDotNoMediaFile( *subFolder ) )
@@ -162,7 +158,7 @@ void FsDiscoverer::checkFolder( fs::IDirectory& currentFolderFs, Folder& current
                 LOG_INFO( "Ignoring folder with a .nomedia file" );
                 continue;
             }
-            LOG_INFO( "New folder detected: ", subFolderPath );
+            LOG_INFO( "New folder detected: ", subFolder->path() );
             addFolder( *subFolder, &currentFolder, blacklist );
             continue;
         }
@@ -189,19 +185,18 @@ void FsDiscoverer::checkFiles( fs::IDirectory& parentFolderFs, Folder& parentFol
     static const std::string req = "SELECT * FROM " + policy::FileTable::Name
             + " WHERE folder_id = ?";
     auto files = File::fetchAll<File>( m_dbConn, req, parentFolder.id() );
-    std::vector<std::string> filesToAdd;
+    std::vector<std::shared_ptr<fs::IFile>> filesToAdd;
     std::vector<std::shared_ptr<File>> filesToRemove;
-    for ( const auto& filePath : parentFolderFs.files() )
+    for ( const auto& fileFs: parentFolderFs.files() )
     {        
-        auto it = std::find_if( begin( files ), end( files ), [filePath](const std::shared_ptr<File>& f) {
-            return f->mrl() == filePath;
+        auto it = std::find_if( begin( files ), end( files ), [fileFs](const std::shared_ptr<File>& f) {
+            return f->mrl() == fileFs->fullPath();
         });
         if ( it == end( files ) )
         {
-            filesToAdd.push_back( filePath );
+            filesToAdd.push_back( fileFs );
             continue;
         }
-        auto fileFs = m_fsFactory->createFile( filePath );
         if ( fileFs->lastModificationDate() == (*it)->lastModificationDate() )
         {
             // Unchanged file
@@ -209,12 +204,12 @@ void FsDiscoverer::checkFiles( fs::IDirectory& parentFolderFs, Folder& parentFol
             continue;
         }
         auto& file = (*it);
-        LOG_INFO( "Forcing file refresh ", filePath );
+        LOG_INFO( "Forcing file refresh ", fileFs->fullPath() );
         // Pre-cache the file's media, since we need it to remove. However, better doing it
         // out of a write context, since that way, other threads can also read the database.
         file->media();
         filesToRemove.push_back( file );
-        filesToAdd.push_back( filePath );
+        filesToAdd.push_back( fileFs );
         files.erase( it );
     }
     auto t = m_dbConn->newTransaction();
@@ -227,7 +222,7 @@ void FsDiscoverer::checkFiles( fs::IDirectory& parentFolderFs, Folder& parentFol
         f->media()->removeFile( *f );
     // Insert all files at once to avoid SQL write contention
     for ( auto& p : filesToAdd )
-        m_ml->addFile( p, parentFolder, parentFolderFs );
+        m_ml->addFile( p->fullPath(), parentFolder, parentFolderFs );
     t->commit();
     LOG_INFO( "Done checking files in ", parentFolderFs.path() );
 }
@@ -257,9 +252,8 @@ bool FsDiscoverer::isBlacklisted( const fs::IDirectory& directory, const std::ve
 bool FsDiscoverer::hasDotNoMediaFile( const fs::IDirectory& directory )
 {
     const auto& files = directory.files();
-    return std::find_if( begin( files ), end( files ), []( const std::string& filePath ){
-        constexpr unsigned int endLength = strlen( "/.nomedia" );
-        return filePath.compare( filePath.length() - endLength, endLength, "/.nomedia" ) == 0;
+    return std::find_if( begin( files ), end( files ), []( const std::shared_ptr<fs::IFile>& file ){
+        return file->name() == ".nomedia";
     }) != end( files );
 }
 

src/filesystem/unix/Directory.cpp

@@ -23,6 +23,7 @@
 #include "Directory.h"
 #include "Media.h"
 #include "Device.h"
+#include "filesystem/unix/File.h"
 
 #include <cstring>
 #include <cstdlib>
@@ -52,14 +53,14 @@ const std::string&Directory::path() const
     return m_path;
 }
 
-const std::vector<std::string>& Directory::files() const
+const std::vector<std::shared_ptr<IFile>>& Directory::files() const
 {
     if ( m_dirs.size() == 0 && m_files.size() == 0 )
         read();
     return m_files;
 }
 
-const std::vector<std::string>& Directory::dirs() const
+const std::vector<std::shared_ptr<IDirectory>>& Directory::dirs() const
 {
     if ( m_dirs.size() == 0 && m_files.size() == 0 )
         read();
@@ -129,11 +130,13 @@ void Directory::read() const
             auto dirPath = toAbsolute( path );
             if ( *dirPath.crbegin() != '/' )
                 dirPath += '/';
-            m_dirs.emplace_back( dirPath );
+            //FIXME: This will use toAbsolute again in the constructor.
+            m_dirs.emplace_back( std::make_shared<Directory>( dirPath ) );
         }
         else
         {
-            m_files.emplace_back( toAbsolute( path ) );
+            auto filePath = toAbsolute( path );
+            m_files.emplace_back( std::make_shared<File>( filePath ) );
         }
     }
 }

src/filesystem/unix/Directory.h

@@ -34,8 +34,8 @@ class Directory : public IDirectory
 public:
     explicit Directory( const std::string& path );
     virtual const std::string& path() const override;
-    virtual const std::vector<std::string>& files() const override;
-    virtual const std::vector<std::string>& dirs() const override;
+    virtual const std::vector<std::shared_ptr<IFile>>& files() const override;
+    virtual const std::vector<std::shared_ptr<IDirectory>>& dirs() const override;
     virtual std::shared_ptr<IDevice> device() const override;
 
 private:
@@ -46,8 +46,8 @@ private:
 
 private:
     std::string m_path;
-    mutable std::vector<std::string> m_files;
-    mutable std::vector<std::string> m_dirs;
+    mutable std::vector<std::shared_ptr<IFile>> m_files;
+    mutable std::vector<std::shared_ptr<IDirectory>> m_dirs;
     mutable Cache<std::shared_ptr<IDevice>> m_device;
 };
 

test/mocks/FileSystem.h

@@ -110,23 +110,19 @@ public:
         return m_path;
     }
 
-    virtual const std::vector<std::string>& files() const override
+    virtual const std::vector<std::shared_ptr<fs::IFile>>& files() const override
     {
-        if ( m_filePathes.size() == 0 )
-        {
-            for ( const auto& f : m_files )
-                m_filePathes.push_back( m_path + f.first );
-        }
+        m_filePathes.clear();
+        for ( auto& f : m_files )
+            m_filePathes.push_back( f.second );
         return m_filePathes;
     }
 
-    virtual const std::vector<std::string>& dirs() const override
+    virtual const std::vector<std::shared_ptr<fs::IDirectory>>& dirs() const override
     {
-        if ( m_dirPathes.size() == 0 )
-        {
-            for ( const auto& d : m_dirs )
-                m_dirPathes.push_back( m_path + d.first + "/" );
-        }
+        m_dirPathes.clear();
+        for ( const auto& d : m_dirs )
+            m_dirPathes.push_back( d.second );
         return m_dirPathes;
     }
 
@@ -141,7 +137,6 @@ public:
         if ( subFolder.empty() == true )
         {
             m_files[filePath] = std::make_shared<File>( m_path + filePath );
-            m_filePathes.clear();
         }
         else
         {
@@ -160,7 +155,6 @@ public:
         {
             auto dir = std::make_shared<Directory>( m_path + subFolder, m_device.lock() );
             m_dirs[subFolder] = dir;
-            m_dirPathes.clear();
         }
         else
         {
@@ -178,7 +172,6 @@ public:
             auto it = m_files.find( filePath );
             assert( it != end( m_files ) );
             m_files.erase( it );
-            m_filePathes.clear();
         }
         else
         {
@@ -235,7 +228,6 @@ public:
             auto it = m_dirs.find( subFolder );
             assert( it != end( m_dirs ) );
             m_dirs.erase( it );
-            m_dirPathes.clear();
         }
         else
         {
@@ -245,19 +237,35 @@ public:
         }
     }
 
-    void addMountpoint( const std::string& path )
+    void setMountpointRoot( const std::string& path, std::shared_ptr<Directory> root )
     {
         auto subFolder = utils::file::firstFolder( path );
         auto remainingPath = utils::file::removePath( path, subFolder );
         if ( remainingPath.empty() == true )
         {
-            m_dirs[subFolder] = nullptr;
+            m_dirs[subFolder] = root;
         }
         else
         {
             auto it = m_dirs.find( subFolder );
             assert( it != end( m_dirs ) );
-            it->second->addMountpoint( remainingPath );
+            it->second->setMountpointRoot( remainingPath, root );
+        }
+    }
+
+    void invalidateMountpoint( const std::string& path )
+    {
+        auto subFolder = utils::file::firstFolder( path );
+        auto remainingPath = utils::file::removePath( path, subFolder );
+        if ( remainingPath.empty() == true )
+        {
+            m_dirs[subFolder] = std::make_shared<Directory>( m_path + subFolder, m_device.lock() );
+        }
+        else
+        {
+            auto it = m_dirs.find( subFolder );
+            assert( it != end( m_dirs ) );
+            it->second->invalidateMountpoint( remainingPath );
         }
     }
 
@@ -265,8 +273,8 @@ private:
     std::string m_path;
     std::unordered_map<std::string, std::shared_ptr<File>> m_files;
     std::unordered_map<std::string, std::shared_ptr<Directory>> m_dirs;
-    mutable std::vector<std::string> m_filePathes;
-    mutable std::vector<std::string> m_dirPathes;
+    mutable std::vector<std::shared_ptr<fs::IFile>> m_filePathes;
+    mutable std::vector<std::shared_ptr<fs::IDirectory>> m_dirPathes;
     std::weak_ptr<Device> m_device;
 };
 
@@ -291,6 +299,11 @@ public:
         m_root = std::make_shared<Directory>( m_mountpoint, shared_from_this() );
     }
 
+    std::shared_ptr<Directory> root()
+    {
+        return m_root;
+    }
+
     virtual const std::string& uuid() const override { return m_uuid; }
     virtual bool isRemovable() const override { return m_removable; }
     virtual bool isPresent() const override { return m_present; }
@@ -348,12 +361,19 @@ public:
         return m_root->directory( relPath );
     }
 
-    void addMountpoint( const std::string& path )
+    void setMountpointRoot( const std::string& path, std::shared_ptr<Directory> root )
     {
         auto relPath = relativePath( path );
         // m_root is already a mountpoint, we can't add a mountpoint to it.
         assert( relPath.empty() == false );
-        m_root->addMountpoint( relPath );
+        m_root->setMountpointRoot( relPath, root );
+    }
+
+    void invalidateMountpoint( const std::string& path )
+    {
+        auto relPath = relativePath( path );
+        assert( relPath.empty() == false );
+        m_root->invalidateMountpoint( relPath );
     }
 
 private:
@@ -386,17 +406,19 @@ struct FileSystemFactory : public factory::IFileSystem
     {
         auto dev = std::make_shared<Device>( mountpoint, uuid );
         dev->setupRoot();
-        // If this folder is already known, mark it as a nullptr and assume this means a mountpoint
         auto d = device( mountpoint );
         if ( d != nullptr )
-            d->addMountpoint( mountpoint );
+            d->setMountpointRoot( mountpoint, dev->root() );
         devices.push_back( dev );
         return dev;
     }
 
-    void addDevice( std::shared_ptr<Device> device )
+    void addDevice( std::shared_ptr<Device> dev )
     {
-        devices.push_back( device );
+        auto d = device( dev->mountpoint() );
+        if ( d != nullptr )
+            d->setMountpointRoot( dev->mountpoint(), dev->root() );
+        devices.push_back( dev );
     }
 
     void addFile( const std::string& filePath )
@@ -469,7 +491,7 @@ struct FileSystemFactory : public factory::IFileSystem
         std::shared_ptr<Device> ret;
         for ( auto& d : devices )
         {
-            if ( path.find( d->mountpoint() ) == 0 &&
+            if ( path.find( d->mountpoint() ) == 0 && d->isPresent() == true &&
                  ( ret == nullptr || ret->mountpoint().length() < d->mountpoint().length() ) )
                 ret = d;
         }
@@ -485,6 +507,10 @@ struct FileSystemFactory : public factory::IFileSystem
             return nullptr;
         auto ret = *it;
         devices.erase( it );
+        // Now flag the mountpoint as belonging to its containing device, since it's now
+        // just a regular folder
+        auto d = device( ret->mountpoint() );
+        d->invalidateMountpoint( ret->mountpoint() );
         return ret;
     }
 
@@ -566,12 +592,12 @@ class NoopDirectory : public fs::IDirectory
         abort();
     }
 
-    virtual const std::vector<std::string>&files() const override
+    virtual const std::vector<std::shared_ptr<fs::IFile>>& files() const override
     {
         abort();
     }
 
-    virtual const std::vector<std::string>&dirs() const override
+    virtual const std::vector<std::shared_ptr<fs::IDirectory>>& dirs() const override
     {
         abort();
     }

test/unittest/DeviceTests.cpp

@@ -49,7 +49,7 @@ protected:
         fsMock.reset( new mock::FileSystemFactory );
         cbMock.reset( new mock::WaitForDiscoveryComplete );
         fsMock->addFolder( "/a/mnt/" );
-        auto device = std::static_pointer_cast<mock::Device>( fsMock->addDevice( RemovableDeviceMountpoint, RemovableDeviceUuid ) );
+        auto device = fsMock->addDevice( RemovableDeviceMountpoint, RemovableDeviceUuid );
         device->setRemovable( true );
         fsMock->addFile( RemovableDeviceMountpoint + "removablefile.mp3" );
         fsMock->addFile( RemovableDeviceMountpoint + "removablefile2.mp3" );
@@ -119,7 +119,7 @@ TEST_F( DeviceFs, RemoveDisk )
     auto media = ml->media( RemovableDeviceMountpoint + "removablefile.mp3" );
     ASSERT_NE( nullptr, media );
 
-    fsMock->removeDevice( RemovableDeviceUuid );
+    auto device = fsMock->removeDevice( RemovableDeviceUuid );
 
     cbMock->prepareForReload();
     Reload();
@@ -263,7 +263,7 @@ TEST_F( DeviceFs, RemoveAlbum )
     auto artists = ml->artists();
     ASSERT_EQ( 2u, artists.size() );
 
-    fsMock->removeDevice( RemovableDeviceUuid );
+    auto device = fsMock->removeDevice( RemovableDeviceUuid );
 
     cbMock->prepareForReload();
     Reload();
@@ -302,7 +302,7 @@ TEST_F( DeviceFs, PartialAlbumRemoval )
     auto artist = artists[0];
     ASSERT_EQ( 2u, artist->media().size() );
 
-    fsMock->removeDevice( RemovableDeviceUuid );
+    auto device = fsMock->removeDevice( RemovableDeviceUuid );
     cbMock->prepareForReload();
     Reload();
     bool reloaded = cbMock->waitForReload();
@@ -352,7 +352,7 @@ TEST_F( DeviceFs, ChangeDevice )
     ASSERT_EQ( firstRemovableFilePath, files[0]->mrl() );
     ASSERT_NE( firstRemovableFileId, f->id() );
 
-    fsMock->removeDevice( "{another-removable-device}" );
+    auto device = fsMock->removeDevice( "{another-removable-device}" );
     fsMock->addDevice( oldRemovableDevice );
 
     cbMock->prepareForReload();