Merge lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming into lp:~mixxxdevelopers/mixxx/trunk

[Mika Haulo]

This is a fix proposal for wishlist bug #661461: Allow playlist and crate renaming (https://bugs.launchpad.net/mixxx/+bug/661461).

[RAFFI TEA (raffitea)]

Hey Mika,

thank you very much for your contribution. I have reviewed your branch and found a bug: Assume you have two playlists "A" and "B". Renaming "B" to "A" will not fail and results in two playlists "A" and "A". Therefore, you must check if the renamed playlist name exists or not. Furthermore, I would like see an UNIQUE constraint on attribute <name> in the <playlist table> (if other developers agree).

@Albert & Developers
I've done a lot of library improvements in my Traktor branch, e.g., n-level tree-structures for child models. If we gonna merge his branch to trunk first, we end up in numerous conflicts when merging Traktor feature in. I recommend to merge his breach into the Traktor feature branch.

review: Needs Fixing

[lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming]

2618. By Mika Haulo on 2010-12-28

Added better input validation for playlist and crate renaming.

2619. By Mika Haulo on 2010-12-28

Getting rid of one extra debug print

[Mika Haulo (mhaulo)]

>
> thank you very much for your contribution. I have reviewed your branch and
> found a bug: Assume you have two playlists "A" and "B". Renaming "B" to "A"
> will not fail and results in two playlists "A" and "A". Therefore, you must
> check if the renamed playlist name exists or not.

Ah, how on earth did I forget that... anyway, a fix for this should be available in my branch now.

I also changed the behaviour a little bit. First of all, the user input is trimmed to eliminate extra whitespace. This effectively prevents whitespace-only names. If the name didn't change from the old name, the function simply returns doing nothing more. And the whole thing is in a loop that keeps popping up an input dialog until the user either gives a valid name or cancels the action.

I'd suggest this kind of behaviour also to be added to the new crate/playlist feature. What do you think?

> Furthermore, I would like
> see an UNIQUE constraint on attribute <name> in the <playlist table> (if other
> developers agree).

I agree with this.

[RAFFI TEA (raffitea)]

Looks all good to me now :-) Your contribution is really helpful. In my opinion we can approve the proposal.

Preventing whitespace-only names is a problem. If do not mind go ahead and commit some fixes.

review: Approve

[William Good (bkgood)]

Why is it not allowable to have two playlists with the same name? If we use the name as a primary key, shouldn't it be UNIQUE anyway?

[RAFFI TEA (raffitea)]

> Why is it not allowable to have two playlists with the same name?
I think this questions depends on how you consider the library domain. I guess most users do not want to have playlists with same name unless we have n-level tree structures.

>If we use the name as a primary key, shouldn't it be UNIQUE anyway?
Unfortunately, the name is not a primary key. The playlist table is made of an artificial primary key followed by the name attribute and so on.

[William Good (bkgood)]

Okay, I thought this was due to some problem arising from design -- like the name was the pkey, and allowing the user to name two playlists the same thing would result in a database error. Using an auto-generated primary key is good database design as far as I've been taught.

As much as the power user in me thinks I should be able to name two playlists the same thing, the usability issues trump my desires.

In regards to merging, I vote for Tobias merging it into his branch. Tobias: if you think the code is good, I'd just merge it now and it'll be better scrutinized when the traktor branch is merged. Keeping branches merge-able with moving targets is a pita.

review: Abstain

[Mika Haulo (mhaulo)]

> Preventing whitespace-only names is a problem. If do not mind go ahead and
> commit some fixes.

I uploaded the changes to this branch:
https://code.launchpad.net/~mhaulo/mixxx/validate_playlist_and_crate_creation

> In regards to merging, I vote for Tobias merging it into his branch. Tobias:
> if you think the code is good, I'd just merge it now and it'll be better
> scrutinized when the traktor branch is merged. Keeping branches merge-able
> with moving targets is a pita.

I'm not completely aware of your future plans about this playlist&traktor thing, but if it helps, I can create a patch directly against the branch Bill mentioned above (or whatever branch you prefer, I just need a url), including the stuff in this merge request.

[RAFFI TEA (raffitea)]

Thanks for your offer Mika. But I've just merged both branches ~mhaulo/mixxx/validate_playlist_and_crate_creation and lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming into lp:~mixxxdevelopers/mixxx/traktor_library. There were some merge conflicts since Mika and I worked on same files :-)

JFI: We plan to add a Traktor library to Mixxx as we did it with iTunes. Traktor is popular commercial DJ application.

=== modified file 'mixxx/src/library/cratefeature.cpp'
--- mixxx/src/library/cratefeature.cpp	2010-11-25 01:03:57 +0000
+++ mixxx/src/library/cratefeature.cpp	2010-12-28 15:47:10 +0000
@@ -26,6 +26,10 @@
     m_pDeleteCrateAction = new QAction(tr("Remove"),this);
     connect(m_pDeleteCrateAction, SIGNAL(triggered()),
             this, SLOT(slotDeleteCrate()));
+			
+	m_pRenameCrateAction = new QAction(tr("Rename"),this);
+	connect(m_pRenameCrateAction, SIGNAL(triggered()),
+			this, SLOT(slotRenameCrate()));
 
     m_crateListTableModel.setTable("crates");
     m_crateListTableModel.removeColumn(m_crateListTableModel.fieldIndex("id"));
@@ -124,6 +128,7 @@
     QMenu menu(NULL);
     menu.addAction(m_pCreateCrateAction);
     menu.addSeparator();
+	menu.addAction(m_pRenameCrateAction);
     menu.addAction(m_pDeleteCrateAction);
     menu.exec(globalPos);
 }
@@ -175,3 +180,50 @@
         qDebug() << "Failed to delete crateId" << crateId;
     }
 }
+
+void CrateFeature::slotRenameCrate() {
+    QString oldName = m_lastRightClickedIndex.data().toString();
+    int crateId = m_pTrackCollection->getCrateDAO().getCrateIdByName(oldName);
+
+    QString newName;
+    bool validNameGiven = false;
+    
+    do {
+        bool ok = false;
+        newName = QInputDialog::getText(NULL,
+                                        tr("Rename Crate"),
+                                        tr("New crate name:"),
+                                        QLineEdit::Normal,
+                                        oldName,
+                                        &ok).trimmed();
+                                        
+        if (!ok || newName == oldName) {
+            return;
+        }
+
+        int existingId = m_pTrackCollection->getCrateDAO().getCrateIdByName(newName);
+
+        if (existingId != -1) {
+            QMessageBox::warning(NULL,
+                                tr("Renaming Crate Failed"),
+                                tr("A crate by that name already exists."));
+        }
+        else if (newName.isEmpty()) {
+            QMessageBox::warning(NULL,
+                                tr("Renaming Crate Failed"),
+                                tr("A crate cannot have a blank name."));
+        }
+        else {
+            validNameGiven = true;
+        }
+    } while (!validNameGiven);
+
+
+    if (m_pTrackCollection->getCrateDAO().renameCrate(crateId, newName)) {
+        m_crateListTableModel.select();
+        emit(featureUpdated());
+        m_crateTableModel.setCrate(crateId);
+    } else {
+        qDebug() << "Failed to rename crateId" << crateId;
+    }
+}

=== modified file 'mixxx/src/library/cratefeature.h'
--- mixxx/src/library/cratefeature.h	2010-02-23 20:06:25 +0000
+++ mixxx/src/library/cratefeature.h	2010-12-28 15:47:10 +0000
@@ -41,11 +41,13 @@
 
     void slotCreateCrate();
     void slotDeleteCrate();
+    void slotRenameCrate();
 
   private:
     TrackCollection* m_pTrackCollection;
     QAction *m_pCreateCrateAction;
     QAction *m_pDeleteCrateAction;
+    QAction *m_pRenameCrateAction;
     QSqlTableModel m_crateListTableModel;
     CrateTableModel m_crateTableModel;
     QModelIndex m_lastRightClickedIndex;

=== modified file 'mixxx/src/library/dao/cratedao.cpp'
--- mixxx/src/library/dao/cratedao.cpp	2010-10-19 01:35:26 +0000
+++ mixxx/src/library/dao/cratedao.cpp	2010-12-28 15:47:10 +0000
@@ -51,6 +51,25 @@
     return false;
 }
 
+bool CrateDAO::renameCrate(int crateId, const QString& newName) {
+    qDebug() << "renameCrate()";
+
+    Q_ASSERT(m_database.transaction());
+    QSqlQuery query;
+    query.prepare("UPDATE " CRATE_TABLE " SET name = :name WHERE id = :id");
+    query.bindValue(":name", newName);
+    query.bindValue(":id", crateId);
+
+    if (!query.exec()) {
+        qDebug() << query.executedQuery() << query.lastError();
+        Q_ASSERT(m_database.rollback());
+        return false;
+    }
+
+    Q_ASSERT(m_database.commit());
+    return true;
+}
+
 bool CrateDAO::deleteCrate(int crateId) {
     Q_ASSERT(m_database.transaction());
 

=== modified file 'mixxx/src/library/dao/cratedao.h'
--- mixxx/src/library/dao/cratedao.h	2010-10-19 01:35:26 +0000
+++ mixxx/src/library/dao/cratedao.h	2010-12-28 15:47:10 +0000
@@ -23,6 +23,7 @@
     unsigned int crateCount();
     bool createCrate(const QString& name);
     bool deleteCrate(int crateId);
+    bool renameCrate(int crateId, const QString& newName);
     int getCrateIdByName(const QString& name);
     int getCrateId(int position);
     QString crateName(int crateId);

=== modified file 'mixxx/src/library/dao/playlistdao.cpp'
--- mixxx/src/library/dao/playlistdao.cpp	2010-10-07 03:05:48 +0000
+++ mixxx/src/library/dao/playlistdao.cpp	2010-12-28 15:47:10 +0000
@@ -145,6 +145,27 @@
     //TODO: Crap, we need to shuffle the positions of all the playlists?
 }
 
+
+void PlaylistDAO::renamePlaylist(int playlistId, const QString& newName)
+{
+    qDebug() << "renamePlaylist()";
+    
+    m_database.transaction();
+    QSqlQuery query(m_database);
+    query.prepare("UPDATE Playlists SET name = :name WHERE id = :id");
+    query.bindValue(":name", newName);
+    query.bindValue(":id", playlistId);
+    
+    if (!query.exec()) {
+        qDebug() << query.executedQuery() << query.lastError();
+        m_database.rollback();
+    }
+    else {
+        m_database.commit();
+    }
+}
+
+
 /** Append a track to a playlist */
 void PlaylistDAO::appendTrackToPlaylist(int trackId, int playlistId)
 {

=== modified file 'mixxx/src/library/dao/playlistdao.h'
--- mixxx/src/library/dao/playlistdao.h	2010-09-12 20:00:07 +0000
+++ mixxx/src/library/dao/playlistdao.h	2010-12-28 15:47:10 +0000
@@ -17,6 +17,8 @@
     bool createPlaylist(QString name, bool hidden = false);
     /** Delete a playlist */
     void deletePlaylist(int playlistId);
+    /** Rename a playlist */
+    void renamePlaylist(int playlistId, const QString& newName);
     /** Append a track to a playlist */
     void appendTrackToPlaylist(int trackId, int playlistId);
     /** Find out how many playlists exist. */

=== modified file 'mixxx/src/library/playlistfeature.cpp'
--- mixxx/src/library/playlistfeature.cpp	2010-11-25 01:06:46 +0000
+++ mixxx/src/library/playlistfeature.cpp	2010-12-28 15:47:10 +0000
@@ -27,6 +27,10 @@
     connect(m_pDeletePlaylistAction, SIGNAL(triggered()),
             this, SLOT(slotDeletePlaylist()));
 
+    m_pRenamePlaylistAction = new QAction(tr("Rename"),this);
+    connect(m_pRenamePlaylistAction, SIGNAL(triggered()),
+            this, SLOT(slotRenamePlaylist()));
+
     // Setup the sidebar playlist model
     m_playlistTableModel.setTable("Playlists");
     m_playlistTableModel.setFilter("hidden=0");
@@ -95,6 +99,7 @@
     QMenu menu(NULL);
     menu.addAction(m_pCreatePlaylistAction);
     menu.addSeparator();
+    menu.addAction(m_pRenamePlaylistAction);
     menu.addAction(m_pDeletePlaylistAction);
     menu.exec(globalPos);
 }
@@ -143,6 +148,52 @@
     }
 }
 
+void PlaylistFeature::slotRenamePlaylist()
+{
+    qDebug() << "slotRenamePlaylist()";
+
+    QString oldName = m_lastRightClickedIndex.data().toString();
+    int playlistId = m_playlistDao.getPlaylistIdFromName(oldName);
+    
+    QString newName;
+    bool validNameGiven = false;
+    
+    do {
+        bool ok = false;
+        newName = QInputDialog::getText(NULL,
+                                        tr("Rename Playlist"),
+                                        tr("New playlist name:"),
+                                        QLineEdit::Normal,
+                                        oldName,
+                                        &ok).trimmed();
+  
+        if (!ok || oldName == newName) {
+            return;
+        }
+
+        int existingId = m_playlistDao.getPlaylistIdFromName(newName);
+        
+        if (existingId != -1) {
+            QMessageBox::warning(NULL,
+                                tr("Renaming Playlist Failed"),
+                                tr("A playlist by that name already exists."));
+        }
+        else if (newName.isEmpty()) {
+            QMessageBox::warning(NULL,
+                                tr("Renaming Playlist Failed"),
+                                tr("A playlist cannot have a blank name."));
+        }
+        else {
+            validNameGiven = true;
+        }
+    } while (!validNameGiven);
+
+    m_playlistDao.renamePlaylist(playlistId, newName);
+    m_playlistTableModel.select();
+    emit(featureUpdated());
+    m_pPlaylistTableModel->setPlaylist(playlistId);
+}
+
 void PlaylistFeature::slotDeletePlaylist()
 {
     //qDebug() << "slotDeletePlaylist() row:" << m_lastRightClickedIndex.data();

=== modified file 'mixxx/src/library/playlistfeature.h'
--- mixxx/src/library/playlistfeature.h	2010-02-23 20:06:25 +0000
+++ mixxx/src/library/playlistfeature.h	2010-12-28 15:47:10 +0000
@@ -45,6 +45,7 @@
 
     void slotCreatePlaylist();
     void slotDeletePlaylist();
+	void slotRenamePlaylist();
 
  private:
     PlaylistTableModel* m_pPlaylistTableModel;
@@ -52,6 +53,7 @@
     TrackDAO &m_trackDao;
     QAction *m_pCreatePlaylistAction;
     QAction *m_pDeletePlaylistAction;
+    QAction *m_pRenamePlaylistAction;
     QSqlTableModel m_playlistTableModel;
     QModelIndex m_lastRightClickedIndex;
 };