Playlist rc5
All points raised in Playlist rc3 are corrected here
Merge request reports
Activity
added 7 commits
- 5ee88052 - MetadataParser: Import playlist files
- 0e4dfbea - Folder: Add a table containing root points to ignore
- 689f2f37 - MetadataParser: Exclude created entry folder from refresh list
- c91e23d3 - Paylist: Add a table associating folders and playlists
- de6f2296 - MediaLibrary: Add info log when a folder is being deleted
- fa7c95a4 - FsDiscoverer: Add playlist deletion case
- 904e87cd - IMediaLibrary: Add a function to destroy a MediaLibrary object
Toggle commit listIs the task executed without this commit?
In theory, VLCMetadataService is considered as completed as soon as
vlcMedia.isValid() == true
:bool VLCMetadataService::isCompleted( const parser::Task& task ) const { // We always need to run this task if the metadata extraction isn't completed return task.vlcMedia.isValid() == true; }
that being said, this feels weird, as the
ParserStep::MetadataExtraction
will never be set if isCompleted returns true and the task doesn't run.My current intuition is that this commit is correct, but another commit is needed:
VLCMetadataService::isCompleted
should be implemented just like the otherParserService
, and simply check for its associate bit.
- src/discoverer/probe/CrawlerProbe.h 0 → 100644
88 virtual std::shared_ptr<Playlist> getPlaylistParent() override 89 { 90 return nullptr; 91 } 92 93 private: 94 static bool hasDotNoMediaFile( const fs::IDirectory& directory ) 95 { 96 const auto& files = directory.files(); 97 return std::find_if( begin( files ), end( files ), []( const std::shared_ptr<fs::IFile>& file ){ 98 return strcasecmp( file->name().c_str(), ".nomedia" ) == 0; 99 }) != end( files ); 100 } 101 102 private: 103 bool m_discoverNoMedia{ false }; - src/discoverer/probe/PathProbe.cpp 0 → 100644
80 return false; 81 m_splitPath.pop(); 82 return true; 83 } 84 85 bool PathProbe::proceedOnFile( const fs::IFile& file ) 86 { 87 if ( m_isDirectory == true && m_isDiscoveryEnded == false && m_splitPath.empty() == true ) 88 { 89 if ( utils::file::toPath( file.mrl() ) != m_path ) 90 return true; 91 m_isDiscoveryEnded = true; 92 return false; 93 } 94 95 if ( m_path == utils::file::stripScheme( file.mrl() ) ) - src/discoverer/probe/PathProbe.cpp 0 → 100644
32 33 namespace medialibrary 34 { 35 36 namespace prober 37 { 38 39 PathProbe::PathProbe( const std::string& path, bool isDirectory, std::shared_ptr<Playlist> parentPlaylist, 40 std::shared_ptr<Folder> parentFolder, const std::string& parentFolderPath, bool reload ) 41 : m_isDirectory( isDirectory ) 42 , m_isDiscoveryEnded( false ) 43 , m_parentPlaylist( std::move( parentPlaylist ) ) 44 , m_parentFolder( std::move( parentFolder ) ) 45 , m_path( path ) 46 { 47 assert( path.size() >= parentFolderPath.size() ); This might not warranty
m_splitPath.size() > splitPath( parentFolderPath ).size()
You might want to move the assertion down in theif ( m_parentFolder != nullptr && m_splitPath.empty() == false )
scope once you can compare both stacks size.Actually you can also check that all elements from the parentFolder stack can be found at the top of the
m_splitPath
task
- src/discoverer/probe/PathProbe.h 0 → 100644
37 38 class PathProbe : public IProbe 39 { 40 public: 41 explicit PathProbe( const std::string& path, // The path we target 42 bool isDirectory, // The PathProbe behave differently between files & folders 43 std::shared_ptr<Playlist> parentPlaylist, // Playlist to be attach to the file addition task 44 std::shared_ptr<Folder> parentFolder, // known parent folder to start from, if any 45 const std::string& parentFolderPath, // known parent folder path 46 bool reload ); 47 48 virtual bool proceedOnDirectory( const fs::IDirectory& directory ) override; 49 50 virtual bool isHidden( const fs::IDirectory& ) override 51 { 52 // We want to add the provided path without checking for .nomedia files Oh OK I think I got it. However I'm a bit confused with the nomedia checks now (see dc37f183 (comment 10292))
- src/discoverer/probe/CrawlerProbe.h 0 → 100644
37 38 CrawlerProbe() = default; 39 40 void setDiscoverNoMedia( bool discoverNoMedia ) 41 { 42 m_discoverNoMedia = discoverNoMedia; 43 } 44 45 virtual bool proceedOnDirectory( const fs::IDirectory& ) override 46 { 47 return true; 48 } 49 50 virtual bool isHidden( const fs::IDirectory& directory ) override 51 { 52 bool hidden = m_discoverNoMedia == false && hasDotNoMediaFile( directory ) == true; I wanted to keep
proceedOnDirectory
andisHidden
separated. They are checked at different places, an depending of the actual implementation of the Probe object they performed unwanted side-effects that I wasn't able to resolve when I tried to unite these methods. But with two methods with two implementations it's OK
75 80 76 81 parser::Task::Status MetadataParser::run( parser::Task& task ) 77 82 { 83 bool alreadyInParser = false; 84 int nbSubitem = task.vlcMedia.subitems()->count(); 85 // Assume that file containing subitem(s) is a Playlist 86 if ( nbSubitem > 0 ) 87 { 88 auto res = addPlaylistMedias( task, nbSubitem ); 89 task.markStepCompleted( parser::Task::ParserStep::Completed ); 90 task.file = task.media->addFile( *task.fileFs, task.parentFolder->id(), 91 task.parentFolderFs->device()->isRemovable(), 92 File::Type::Main ); 93 if ( task.file == nullptr ) 126 // Voluntarily trigger an exception for a valid, but less common case, to avoid database overhead 127 catch ( sqlite::errors::ConstraintViolation& ex ) 94 128 { 95 LOG_ERROR( "Failed to add file ", task.mrl, " to media #", task.media->id() ); 96 return parser::Task::Status::Fatal; 129 LOG_INFO( "Creation of Media & File failed because ", ex.what(), 130 ". Assuming this task is a duplicate" ); 131 // Try to retrieve file & Media from database 132 auto fileInDB = File::fromMrl( m_ml, task.mrl ); 133 if ( fileInDB == nullptr ) // That wasn't a duplicate, after all 134 { 135 LOG_ERROR( "Constraint violation on not duplicate task: ", task.mrl, " aborting"); I'm enclined to think "no". Or at worst, if the file happens to be deleted in the database, it means it has been deleted on disk and the FsDiscoverer realized that. In which case, it's fine for the file to be deleted. Insertion by another thread can't happen since the only thread responsible for inserting is the one you tried to insert from
You need the media in case in needs to be added to the playlist, but the file would be fetched for nothing, since you'll set
alreadyInParser
totrue
, causing the code a few lines later to mark the entire task as completed and returning Success, causing the parsing of this specific task to be stopped
126 // Voluntarily trigger an exception for a valid, but less common case, to avoid database overhead 127 catch ( sqlite::errors::ConstraintViolation& ex ) 94 128 { 95 LOG_ERROR( "Failed to add file ", task.mrl, " to media #", task.media->id() ); 96 return parser::Task::Status::Fatal; 129 LOG_INFO( "Creation of Media & File failed because ", ex.what(), 130 ". Assuming this task is a duplicate" ); 131 // Try to retrieve file & Media from database 132 auto fileInDB = File::fromMrl( m_ml, task.mrl ); 133 if ( fileInDB == nullptr ) // That wasn't a duplicate, after all 134 { 135 LOG_ERROR( "Constraint violation on not duplicate task: ", task.mrl, " aborting"); 136 return parser::Task::Status::Fatal; 137 } 138 task.file = fileInDB; 139 task.media = fileInDB->media(); 237 auto playlistPtr = Playlist::create( m_ml, playlistName ); 238 if ( playlistPtr == nullptr ) 239 { 240 LOG_ERROR( "Failed to create playlist ", task.mrl, " to the media library" ); 241 return false; 242 } 243 task.file = playlistPtr->addFile( *task.fileFs, task.parentFolder->id(), task.parentFolderFs->device()->isRemovable() ); 244 if ( task.file == nullptr ) 245 { 246 LOG_ERROR( "Failed to add playlist file ", task.mrl ); 247 return false; 248 } 249 t->commit(); 250 auto subitems = task.vlcMedia.subitems(); 251 for ( int i = 0; i < nbSubitem; ++i ) // FIXME: Interrupt loop if paused 252 { It exposes a better API to bind with any language in which we can't use
delete
Edited by Alexandre Fernandez