Avoid having similar filenames when organising songs (number them instead)

src/core/organise.cpp

@@ -36,7 +36,7 @@ const int Organise::kTranscodeProgressInterval = 500;
 Organise::Organise(TaskManager* task_manager,
                    boost::shared_ptr<MusicStorage> 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) {
+  for (const NewSongInfo& song_info : songs_info) {
-    tasks_pending_ << Task(song);
+    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_;
+      task.song_info_.song_.url().toLocalFile() : task.transcoded_filename_;
-    job.destination_ = format_.GetFilenameForSong(song);
+    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

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<NewSongInfo> NewSongInfoList;
+
   Organise(TaskManager* task_manager,
            boost::shared_ptr<MusicStorage> 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())
+    explicit Task(const NewSongInfo& song_info = NewSongInfo())
-      : song_(song), transcode_progress_(0.0) {}
+      : song_info_(song_info), transcode_progress_(0.0) {}
 
-    Song song_;
+    NewSongInfo song_info_;
 
     float transcode_progress_;
     QString transcoded_filename_;

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 =
+  else if (tag == "extension")   value = QFileInfo(song.url().toLocalFile()).suffix();
-      song.url().toLocalFile().section('.', -1, -1);
   else if (tag == "artistinitial") {
     value = song.effective_albumartist().trimmed();
     if (replace_the_ && !value.isEmpty())

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 <QDir>
 #include <QFileInfo>
+#include <QHash>
 #include <QMenu>
 #include <QPushButton>
 #include <QResizeEvent>
@@ -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<QString, int> 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();
 

src/ui/organisedialog.h

@@ -23,6 +23,9 @@
 #include <QMap>
 #include <QUrl>
 
+#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<OrganiseErrorDialog> error_dialog_;
 
   bool resized_by_user_;
+
+  FRIEND_TEST(OrganiseDialogTest, ComputeNewSongsFilenamesTest);
 };
 
 #endif // ORGANISEDIALOG_H

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)

tests/organisedialog_test.cpp

@@ -0,0 +1,71 @@
+/* This file is part of Clementine.
+   Copyright 2014, David Sansome <me@davidsansome.com>
+
+   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 <http://www.gnu.org/licenses/>.
+*/
+
+#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_);
+}
+