From a349a96f5a5c1df47e883856a89932bc14aee134 Mon Sep 17 00:00:00 2001 From: Arnaud Bienner Date: Sun, 2 Feb 2014 19:28:45 +0100 Subject: [PATCH] Avoid having similar filenames when organising songs (number them instead) --- src/core/organise.cpp | 24 ++++++------ src/core/organise.h | 17 +++++++-- src/core/organiseformat.cpp | 3 +- src/ui/organisedialog.cpp | 33 ++++++++++++++-- src/ui/organisedialog.h | 10 +++++ tests/CMakeLists.txt | 1 + tests/organisedialog_test.cpp | 71 +++++++++++++++++++++++++++++++++++ 7 files changed, 138 insertions(+), 21 deletions(-) create mode 100644 tests/organisedialog_test.cpp diff --git a/src/core/organise.cpp b/src/core/organise.cpp index edae628b9..b9efc0dab 100644 --- a/src/core/organise.cpp +++ b/src/core/organise.cpp @@ -36,7 +36,7 @@ const int Organise::kTranscodeProgressInterval = 500; Organise::Organise(TaskManager* task_manager, boost::shared_ptr destination, const OrganiseFormat &format, bool copy, bool overwrite, - const SongList& songs, bool eject_after) + const NewSongInfoList& songs_info, bool eject_after) : thread_(NULL), task_manager_(task_manager), transcoder_(new Transcoder(this)), @@ -45,7 +45,7 @@ Organise::Organise(TaskManager* task_manager, copy_(copy), overwrite_(overwrite), eject_after_(eject_after), - task_count_(songs.count()), + task_count_(songs_info.count()), transcode_suffix_(1), tasks_complete_(0), started_(false), @@ -54,8 +54,8 @@ Organise::Organise(TaskManager* task_manager, { original_thread_ = thread(); - for (const Song& song : songs) { - tasks_pending_ << Task(song); + for (const NewSongInfo& song_info : songs_info) { + tasks_pending_ << Task(song_info); } } @@ -81,7 +81,7 @@ void Organise::ProcessSomeFiles() { if (!destination_->StartCopy(&supported_filetypes_)) { // Failed to start - mark everything as failed :( for (const Task& task : tasks_pending_) - files_with_errors_ << task.song_.url().toLocalFile(); + files_with_errors_ << task.song_info_.song_.url().toLocalFile(); tasks_pending_.clear(); } started_ = true; @@ -124,10 +124,10 @@ void Organise::ProcessSomeFiles() { break; Task task = tasks_pending_.takeFirst(); - qLog(Info) << "Processing" << task.song_.url().toLocalFile(); + qLog(Info) << "Processing" << task.song_info_.song_.url().toLocalFile(); // Use a Song instead of a tag reader - Song song = task.song_; + Song song = task.song_info_.song_; if (!song.is_valid()) continue; @@ -157,14 +157,14 @@ void Organise::ProcessSomeFiles() { QString::number(transcode_suffix_++); task.new_extension_ = preset.extension_; task.new_filetype_ = dest_type; - tasks_transcoding_[task.song_.url().toLocalFile()] = task; + tasks_transcoding_[task.song_info_.song_.url().toLocalFile()] = task; qLog(Debug) << "Transcoding to" << task.transcoded_filename_; // Start the transcoding - this will happen in the background and // FileTranscoded() will get called when it's done. At that point the // task will get re-added to the pending queue with the new filename. - transcoder_->AddJob(task.song_.url().toLocalFile(), preset, task.transcoded_filename_); + transcoder_->AddJob(task.song_info_.song_.url().toLocalFile(), preset, task.transcoded_filename_); transcoder_->Start(); continue; } @@ -172,8 +172,8 @@ void Organise::ProcessSomeFiles() { MusicStorage::CopyJob job; job.source_ = task.transcoded_filename_.isEmpty() ? - task.song_.url().toLocalFile() : task.transcoded_filename_; - job.destination_ = format_.GetFilenameForSong(song); + task.song_info_.song_.url().toLocalFile() : task.transcoded_filename_; + job.destination_ = task.song_info_.new_filename_; job.metadata_ = song; job.overwrite_ = overwrite_; job.remove_original_ = !copy_; @@ -181,7 +181,7 @@ void Organise::ProcessSomeFiles() { this, _1, !task.transcoded_filename_.isEmpty()); if (!destination_->CopyToStorage(job)) { - files_with_errors_ << task.song_.basefilename(); + files_with_errors_ << task.song_info_.song_.basefilename(); } // Clean up the temporary transcoded file diff --git a/src/core/organise.h b/src/core/organise.h index a65142459..b40bf8cb9 100644 --- a/src/core/organise.h +++ b/src/core/organise.h @@ -34,10 +34,19 @@ class Organise : public QObject { Q_OBJECT public: + + struct NewSongInfo { + NewSongInfo(const Song& song = Song(), const QString& new_filename = QString()) + : song_(song), new_filename_(new_filename) {} + Song song_; + QString new_filename_; + }; + typedef QList NewSongInfoList; + Organise(TaskManager* task_manager, boost::shared_ptr destination, const OrganiseFormat& format, bool copy, bool overwrite, - const SongList& songs, bool eject_after); + const NewSongInfoList& songs, bool eject_after); static const int kBatchSize; static const int kTranscodeProgressInterval; @@ -61,10 +70,10 @@ private: private: struct Task { - explicit Task(const Song& song = Song()) - : song_(song), transcode_progress_(0.0) {} + explicit Task(const NewSongInfo& song_info = NewSongInfo()) + : song_info_(song_info), transcode_progress_(0.0) {} - Song song_; + NewSongInfo song_info_; float transcode_progress_; QString transcoded_filename_; diff --git a/src/core/organiseformat.cpp b/src/core/organiseformat.cpp index f911252d8..e8be2030a 100644 --- a/src/core/organiseformat.cpp +++ b/src/core/organiseformat.cpp @@ -161,8 +161,7 @@ QString OrganiseFormat::TagValue(const QString &tag, const Song &song) const { QString::number(song.length_nanosec() / kNsecPerSec); else if (tag == "bitrate") value = QString::number(song.bitrate()); else if (tag == "samplerate") value = QString::number(song.samplerate()); - else if (tag == "extension") value = - song.url().toLocalFile().section('.', -1, -1); + else if (tag == "extension") value = QFileInfo(song.url().toLocalFile()).suffix(); else if (tag == "artistinitial") { value = song.effective_albumartist().trimmed(); if (replace_the_ && !value.isEmpty()) diff --git a/src/ui/organisedialog.cpp b/src/ui/organisedialog.cpp index 3d915b827..27eaf4e7d 100644 --- a/src/ui/organisedialog.cpp +++ b/src/ui/organisedialog.cpp @@ -22,9 +22,11 @@ #include "core/musicstorage.h" #include "core/organise.h" #include "core/tagreaderclient.h" +#include "core/utilities.h" #include #include +#include #include #include #include @@ -162,6 +164,29 @@ void OrganiseDialog::InsertTag(const QString &tag) { ui_->naming->insertPlainText("%" + tag); } +Organise::NewSongInfoList OrganiseDialog::ComputeNewSongsFilenames( + const SongList& songs, const OrganiseFormat& format) { + + // Check if we will have multiple files with the same name. + // If so, they will erase each other if the overwrite flag is set. + // Better to rename them: e.g. foo.bar -> foo(2).bar + QHash filenames; + Organise::NewSongInfoList new_songs_info; + + for (const Song& song : songs) { + QString new_filename = format.GetFilenameForSong(song); + if (filenames.contains(new_filename)) { + QString song_number = QString::number(++filenames[new_filename]); + new_filename = + Utilities::PathWithoutFilenameExtension(new_filename) + + "(" + song_number + ")." + QFileInfo(new_filename).suffix(); + } + filenames.insert(new_filename, 1); + new_songs_info << Organise::NewSongInfo(song, new_filename); + } + return new_songs_info; +} + void OrganiseDialog::UpdatePreviews() { const QModelIndex destination = ui_->destination->model()->index( ui_->destination->currentIndex(), 0); @@ -205,14 +230,16 @@ void OrganiseDialog::UpdatePreviews() { if (!format_valid) return; + new_songs_info_ = ComputeNewSongsFilenames(songs_, format_); + // Update the previews ui_->preview->clear(); ui_->preview_group->setVisible(has_local_destination); ui_->naming_group->setVisible(has_local_destination); if (has_local_destination) { - for (const Song& song : songs_) { + for (const Organise::NewSongInfo& song_info : new_songs_info_) { QString filename = storage->LocalPath() + "/" + - format_.GetFilenameForSong(song); + song_info.new_filename_; ui_->preview->addItem(QDir::toNativeSeparators(filename)); } } @@ -278,7 +305,7 @@ void OrganiseDialog::accept() { const bool copy = ui_->aftercopying->currentIndex() == 0; Organise* organise = new Organise( task_manager_, storage, format_, copy, ui_->overwrite->isChecked(), - songs_, ui_->eject_after->isChecked()); + new_songs_info_, ui_->eject_after->isChecked()); connect(organise, SIGNAL(Finished(QStringList)), SLOT(OrganiseFinished(QStringList))); organise->Start(); diff --git a/src/ui/organisedialog.h b/src/ui/organisedialog.h index 3b545ccd7..a9a2ce0b8 100644 --- a/src/ui/organisedialog.h +++ b/src/ui/organisedialog.h @@ -23,6 +23,9 @@ #include #include +#include "gtest/gtest_prod.h" + +#include "core/organise.h" #include "core/organiseformat.h" #include "core/song.h" @@ -68,17 +71,24 @@ private slots: void OrganiseFinished(const QStringList& files_with_errors); private: + static Organise::NewSongInfoList ComputeNewSongsFilenames( + const SongList& songs, + const OrganiseFormat& format); + Ui_OrganiseDialog* ui_; TaskManager* task_manager_; OrganiseFormat format_; SongList songs_; + Organise::NewSongInfoList new_songs_info_; quint64 total_size_; std::unique_ptr error_dialog_; bool resized_by_user_; + + FRIEND_TEST(OrganiseDialogTest, ComputeNewSongsFilenamesTest); }; #endif // ORGANISEDIALOG_H diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 24f7165ea..9149e4b1f 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -132,6 +132,7 @@ add_test_file(fmpsparser_test.cpp false) #add_test_file(m3uparser_test.cpp false) add_test_file(mergedproxymodel_test.cpp false) add_test_file(organiseformat_test.cpp false) +add_test_file(organisedialog_test.cpp false) #add_test_file(playlist_test.cpp true) #add_test_file(plsparser_test.cpp false) add_test_file(scopedtransaction_test.cpp false) diff --git a/tests/organisedialog_test.cpp b/tests/organisedialog_test.cpp new file mode 100644 index 000000000..53460fffd --- /dev/null +++ b/tests/organisedialog_test.cpp @@ -0,0 +1,71 @@ +/* This file is part of Clementine. + Copyright 2014, David Sansome + + Clementine is free software: you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation, either version 3 of the License, or + (at your option) any later version. + + Clementine 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 Clementine. If not, see . +*/ + +#include "gtest/gtest.h" +#include "test_utils.h" + +#include "core/song.h" +#include "core/organiseformat.h" +#include "ui/organisedialog.h" + + +TEST(OrganiseDialogTest, ComputeNewSongsFilenamesTest) { + // Create some songs, with multiple similar songs + SongList songs; + { + Song song; + song.set_title("Test1"); + song.set_album("Album"); + songs << song; + } + // Empty song + { + Song song; + song.set_basefilename("filename.mp3"); + songs << song; + } + // Without extension + for (int i = 0; i < 2; i++) { + Song song; + song.set_title("Test2"); + song.set_url(QUrl("file://test" + QString::number(i))); + songs << song; + } + + // With file extension + for (int i = 0; i < 3; i++) { + Song song; + song.set_artist("Foo"); + song.set_title("Bar"); + song.set_url(QUrl("file://foobar" + QString::number(i) + ".mp3")); + songs << song; + } + + // Generate new filenames + OrganiseFormat format; + format.set_format(OrganiseDialog::kDefaultFormat); + Organise::NewSongInfoList new_songs_info = OrganiseDialog::ComputeNewSongsFilenames(songs, format); + + EXPECT_EQ("/Album/Test1.", new_songs_info[0].new_filename_); + EXPECT_EQ("//filename.mp3", new_songs_info[1].new_filename_); + EXPECT_EQ("//Test2.", new_songs_info[2].new_filename_); + EXPECT_EQ("//Test2(2).", new_songs_info[3].new_filename_); + EXPECT_EQ("Foo//Bar.mp3", new_songs_info[4].new_filename_); + EXPECT_EQ("Foo//Bar(2).mp3", new_songs_info[5].new_filename_); + EXPECT_EQ("Foo//Bar(3).mp3", new_songs_info[6].new_filename_); +} +