Skip to content

Commit

Permalink
Keep old track IDs when moving files within the library
Browse files Browse the repository at this point in the history 
Our previous FileHooks implementation was such that moving a scanned audio
file from on folder within the library to another folder within the library
caused the track object matching the said file to be deleted and the
recreated. Hence, the track got a new ID and it got removed from any
playlists where it belonged to.

Now we handle file moving separately and differently than a file deletion
followed by file recreation, and the track gets updated "in place", without
changing the ID or removing it from existing playlists.

If, on the other hand, an entire folder full of scanned audio files gets
moved, then we do one of two things:
1) In case there's 30 or less audio files contained, we handle each file
separately the same way as if those 30 files were moved individually
2) In case there are more than 30 audio files contained, we remove them all
from the library and the user needs to rescan the files.

The second case is suboptimal but we can't handle any number of files
one-by-one during the moving because that could slow down the moving
operation too much. In the best case this would annoy the user and in the
worst case it would cause a time-out and break the entire operation.

This is a partial fix for #1173.
  • Loading branch information
@paulijar
paulijar committed Nov 13, 2024
1 parent c964e20 commit 9c43c9c
Show file tree
Hide file tree
Showing 2 changed files with 116 additions and 44 deletions.
91 changes: 59 additions & 32 deletions lib/Hooks/FileHooks.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,8 @@ public function __construct($filesystemRoot) {
* Invoke auto update of music database after file or folder deletion
* @param Node $node pointing to the file or folder
*/
public static function deleted(Node $node) {
$app = \OC::$server->query(Application::class);
$container = $app->getContainer();
public static function deleted(Node $node) : void {
$container = self::getContainer();
$scanner = $container->query('Scanner');

if ($node->getType() == FileInfo::TYPE_FILE) {
Expand All @@ -47,7 +46,7 @@ public static function deleted(Node $node) {
* Invoke auto update of music database after file update or file creation
* @param Node $node pointing to the file
*/
public static function updated(Node $node) {
public static function updated(Node $node) : void {
// At least on Nextcloud 13, it sometimes happens that this hook is triggered
// when the core creates some temporary file and trying to access the provided
// node throws an exception, probably because the temp file is already removed
Expand All @@ -56,8 +55,7 @@ public static function updated(Node $node) {
// File::fopen, this hook gets triggered immediately after the opening succeeds,
// before anything is actually written and while the file is *exclusively locked
// because of the write mode*. See #638.
$app = \OC::$server->query(Application::class);
$container = $app->getContainer();
$container = self::getContainer();
try {
self::handleUpdated($node, $container);
} catch (\OCP\Files\NotFoundException $e) {
Expand All @@ -69,18 +67,11 @@ public static function updated(Node $node) {
}
}

private static function handleUpdated(Node $node, IAppContainer $container) {
private static function handleUpdated(Node $node, IAppContainer $container) : void {
// we are interested only about updates on files, not on folders
if ($node->getType() == FileInfo::TYPE_FILE) {
$scanner = $container->query('Scanner');
$userId = $container->query('UserId');

// When a file is uploaded to a folder shared by link, we end up here without current user.
// In that case, fall back to using file owner
if (empty($userId)) {
$owner = $node->getOwner();
$userId = $owner ? $owner->getUID() : null; // @phpstan-ignore-line At least some versions of NC may violate their PhpDoc and return null owner
}
$userId = self::getUser($node, $container);

// Ignore event if we got no user or folder or the user has not yet scanned the music
// collection. The last condition is especially to prevent problems when creating new user
Expand All @@ -91,39 +82,75 @@ private static function handleUpdated(Node $node, IAppContainer $container) {
}
}

private static function moved(Node $node) : void {
$container = self::getContainer();
try {
self::handleMoved($node, $container);
} catch (\OCP\Files\NotFoundException $e) {
$logger = $container->query('Logger');
$logger->log('FileHooks::moved triggered for a non-existing file', 'warn');
} catch (\OCP\Lock\LockedException $e) {
$logger = $container->query('Logger');
$logger->log('FileHooks::moved triggered for a locked file ' . $node->getName(), 'warn');
}
}

private static function handleMoved(Node $node, IAppContainer $container) : void {
$scanner = $container->query('Scanner');
$userId = self::getUser($node, $container);

if (!empty($userId) && self::userHasMusicLib($userId, $container)) {
if ($node->getType() == FileInfo::TYPE_FILE) {
$scanner->fileMoved($node, $userId);
} else {
$scanner->folderMoved($node, $userId);
}
}
}

private static function getUser(Node $node, IAppContainer $container) : ?string {
$userId = $container->query('UserId');

// When a file is uploaded to a folder shared by link, we end up here without current user.
// In that case, fall back to using file owner
if (empty($userId)) {
// At least some versions of NC may violate their PhpDoc and return null owner, hence we need to aid PHPStan a bit about the type.
/** @var \OCP\IUser|null $owner */
$owner = $node->getOwner();
$userId = $owner ? $owner->getUID() : null;
}

return $userId;
}

private static function getContainer() : IAppContainer {
$app = \OC::$server->query(Application::class);
return $app->getContainer();
}

/**
* Check if user has any scanned tracks in his/her music library
* @param string $userId
* @param IAppContainer $container
*/
private static function userHasMusicLib(string $userId, IAppContainer $container) {
private static function userHasMusicLib(string $userId, IAppContainer $container) : bool {
$trackBusinessLayer = $container->query('TrackBusinessLayer');
return 0 < $trackBusinessLayer->count($userId);
}

public static function preRenamed(Node $source, Node $target) {
// We are interested only if the path of the folder of the file changes:
// that could move a music file out of the scanned folder or remove a
// cover image file from album folder.
public static function postRenamed(Node $source, Node $target) : void {
// Beware: the $source describes the past state of the file and some of its functions will throw upon calling

if ($source->getParent()->getId() != $target->getParent()->getId()) {
self::deleted($source);
// $target here doesn't point to an existing file, hence we need also
// the postRenamed hook
self::moved($target);
} else {
self::updated($target);
}
}

public static function postRenamed(Node $source, Node $target) {
// Renaming/moving file may
// a) move it into the folder scanned by Music app
// b) have influence on the display name of the track
// Both of these cases can be handled like file addition/modification.
self::updated($target);
}

public function register() {
$this->filesystemRoot->listen('\OC\Files', 'postWrite', [__CLASS__, 'updated']);
$this->filesystemRoot->listen('\OC\Files', 'preDelete', [__CLASS__, 'deleted']);
$this->filesystemRoot->listen('\OC\Files', 'preRename', [__CLASS__, 'preRenamed']);
$this->filesystemRoot->listen('\OC\Files', 'postRename', [__CLASS__, 'postRenamed']);
}
}
69 changes: 57 additions & 12 deletions lib/Utility/Scanner.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* @author Morris Jobke <[email protected]>
* @author Pauli Järvinen <[email protected]>
* @copyright Morris Jobke 2013, 2014
* @copyright Pauli Järvinen 2016 - 2023
* @copyright Pauli Järvinen 2016 - 2024
*/

namespace OCA\Music\Utility;
Expand Down Expand Up @@ -83,25 +83,71 @@ public function __construct(Extractor $extractor,
* Gets called by 'post_write' (file creation, file update) and 'post_share' hooks
*/
public function update(File $file, string $userId, string $filePath) : void {
// debug logging
$this->logger->log("update - $filePath", 'debug');
$mimetype = $file->getMimeType();
$this->logger->log("update - $filePath - $mimetype", 'debug');

// skip files that aren't inside the user specified path
if (!$this->librarySettings->pathBelongsToMusicLibrary($filePath, $userId)) {
$this->logger->log("skipped - file is outside of specified music folder", 'debug');
return;
}
elseif (Util::startsWith($mimetype, 'image')) {
$this->updateImage($file, $userId);
}
elseif (Util::startsWith($mimetype, 'audio') && !self::isPlaylistMime($mimetype)) {
$libraryRoot = $this->librarySettings->getFolder($userId);
$this->updateAudio($file, $userId, $libraryRoot, $filePath, $mimetype, /*partOfScan=*/false);
}
}

public function fileMoved(File $file, string $userId) : void {
$mimetype = $file->getMimeType();

// debug logging
$this->logger->log("update - mimetype $mimetype", 'debug');
$this->logger->log('fileMoved - '. $file->getPath() . " - $mimetype", 'debug');

if (Util::startsWith($mimetype, 'image')) {
// we don't need to track the identity of images and moving a file can be handled as it was
// a file deletion followed by a file addition
$this->deleteImage([$file->getId()], [$userId]);
$this->updateImage($file, $userId);
} elseif (Util::startsWith($mimetype, 'audio') && !self::isPlaylistMime($mimetype)) {
$libraryRoot = $this->librarySettings->getFolder($userId);
$this->updateAudio($file, $userId, $libraryRoot, $filePath, $mimetype, /*partOfScan=*/false);
}
elseif (Util::startsWith($mimetype, 'audio') && !self::isPlaylistMime($mimetype)) {
if ($this->librarySettings->pathBelongsToMusicLibrary($file->getPath(), $userId)) {
// In the new path, the file (now or still) belongs to the library. Even if it was already in the lib,
// the new path may have an influence on the album or artist name (in case of incomplete metadata).
$libraryRoot = $this->librarySettings->getFolder($userId);
$this->updateAudio($file, $userId, $libraryRoot, $file->getPath(), $mimetype, /*partOfScan=*/false);
} else {
// In the new path, the file doesn't (still or any longer) belong to the library. Remove it if
// it happened to be in the library.
$this->deleteAudio([$file->getId()], [$userId]);
}
}
}

public function folderMoved(Folder $folder, string $userId) : void {
$this->logger->log('folderMoved - '. $folder->getPath(), 'debug');

$audioFiles = $folder->searchByMime('audio');

if (\count($audioFiles) > 0) {
if ($this->librarySettings->pathBelongsToMusicLibrary($folder->getPath(), $userId)) {
// The new path of the folder belongs to the library but this doesn't necessarily mean
// that all the file paths below belong to the library, because of the path exclusions.
// Each file needs to be checked and updated separately but this may take too much time
// if there is extensive number of files.
if (\count($audioFiles) <= 30) {
foreach ($audioFiles as $file) {
$this->fileMoved($file, $userId);
}
} else {
// Remove the scanned files to get them rescanned when the Music app is opened.
// TODO: Better handling e.g. by marking the files as dirty.
$this->deleteAudio(Util::extractIds($audioFiles), [$userId]);
}
}
else {
// The new path of the folder doesn't belong to the library so neither does any of the
// contained files. Remove audio files from the lib if found.
$this->deleteAudio(Util::extractIds($audioFiles), [$userId]);
}
}
}

Expand Down Expand Up @@ -172,7 +218,6 @@ private function updateAudio(File $file, string $userId, Folder $libraryRoot, st
$this->cache->remove($userId, 'collection');
}

// debug logging
$this->logger->log('imported entities - ' .
"artist: $artistId, albumArtist: $albumArtistId, album: $albumId, track: {$track->getId()}",
'debug');
Expand Down

0 comments on commit 9c43c9c

Please sign in to comment.