Skip to content
Snippets Groups Projects

Playlist rc5

Closed Alexandre Fernandez requested to merge Nerf/medialibrary:playlist-rc5 into master

All points raised in Playlist rc3 are corrected here

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • 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

    Compare with previous version

    • Is 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 other ParserService, and simply check for its associate bit.

    • So, TL;DR version: LGTM, I'll refactor VLCMetadataService::isCompleted

    • Meh, actually I'm afraid changing VLCMetadataService::isCompleted will cause some files to never be parsed... Maybe the correct way is to mark the step completed when creating the original task? Although that seems like a bit of a layer violation

    • Please register or sign in to reply
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 };
  • Please keep all the initializations in the constructor (although you could just remove the boolean initialization since it will be initialized to false anyway. However, a comment stating so will prevent our future selves from wondering about the initial value, most likely.

  • Please register or sign in to reply
  • 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() ) )
  • 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 the if ( 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

    • Actually you can also check that all elements from the parentFolder stack can be found at the top of the m_splitPath task

      Isn't that what is already done in the loop: assert( parentSplit.top() == m_splitPath.top() );?

    • Hm, true for the top most element, I was thinking about checking all elements in parentSplit, but that's not doable with an std::stack without poping all elements and reinserting them afterward. That seems overkill.

    • Woops, that's actually what you're already doing, my bad :)

    • Please register or sign in to reply
  • I'm afraid I'll have to ask for at least a functional test for this

  • 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
  • 72 74 return false;
    73 75
    74 76 std::shared_ptr<fs::IDirectory> fsDir = m_fsFactory->createDirectory( entryPoint );
    77 auto fsDirMrl = fsDir->mrl();
  • 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 thought you mentionned that it was a bad idea to merge hasDotNoMediaFile in the prober? Did something change or did I miss another comment somewhere?

    • I wanted to keep proceedOnDirectory and isHidden 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

    • Please register or sign in to reply
  • 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");
  • 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 {
  • 123 130 return self;
    124 131 }
    125 132
    133 void Folder::excludeEntryFolder( MediaLibraryPtr ml, int64_t folderId )
    134 {
    135 std::string req = "INSERT INTO ExcludedEntryFolder(folder_id) VALUES(" + std::to_string( folderId ) + ")";
  • Please register or sign in to reply
    Loading