Merge lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming into lp:~mixxxdevelopers/mixxx/trunk
- allow-playlist-and-crate-renaming
- Merge into trunk
Status: | Merged |
---|---|
Merge reported by: | RAFFI TEA |
Merged at revision: | not available |
Proposed branch: | lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming |
Merge into: | lp:~mixxxdevelopers/mixxx/trunk |
Diff against target: |
274 lines (+150/-0) 8 files modified
mixxx/src/library/cratefeature.cpp (+52/-0) mixxx/src/library/cratefeature.h (+2/-0) mixxx/src/library/dao/cratedao.cpp (+19/-0) mixxx/src/library/dao/cratedao.h (+1/-0) mixxx/src/library/dao/playlistdao.cpp (+21/-0) mixxx/src/library/dao/playlistdao.h (+2/-0) mixxx/src/library/playlistfeature.cpp (+51/-0) mixxx/src/library/playlistfeature.h (+2/-0) |
To merge this branch: | bzr merge lp:~mhaulo/mixxx/allow-playlist-and-crate-renaming |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
William Good | Abstain | ||
RAFFI TEA | Approve | ||
Review via email: mp+44722@code.launchpad.net |
Commit message
Description of the change
This is a fix proposal for wishlist bug #661461: Allow playlist and crate renaming (https:/
- 2618. By Mika Haulo
-
Added better input validation for playlist and crate renaming.
- 2619. By Mika Haulo
-
Getting rid of one extra debug print
Mika Haulo (mhaulo) wrote : | # |
>
> 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) wrote : | # |
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.
William Good (bkgood) wrote : | # |
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) wrote : | # |
> 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) wrote : | # |
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.
Mika Haulo (mhaulo) wrote : | # |
> 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:/
> 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) wrote : | # |
Thanks for your offer Mika. But I've just merged both branches ~mhaulo/
JFI: We plan to add a Traktor library to Mixxx as we did it with iTunes. Traktor is popular commercial DJ application.
Preview Diff
1 | === modified file 'mixxx/src/library/cratefeature.cpp' |
2 | --- mixxx/src/library/cratefeature.cpp 2010-11-25 01:03:57 +0000 |
3 | +++ mixxx/src/library/cratefeature.cpp 2010-12-28 15:47:10 +0000 |
4 | @@ -26,6 +26,10 @@ |
5 | m_pDeleteCrateAction = new QAction(tr("Remove"),this); |
6 | connect(m_pDeleteCrateAction, SIGNAL(triggered()), |
7 | this, SLOT(slotDeleteCrate())); |
8 | + |
9 | + m_pRenameCrateAction = new QAction(tr("Rename"),this); |
10 | + connect(m_pRenameCrateAction, SIGNAL(triggered()), |
11 | + this, SLOT(slotRenameCrate())); |
12 | |
13 | m_crateListTableModel.setTable("crates"); |
14 | m_crateListTableModel.removeColumn(m_crateListTableModel.fieldIndex("id")); |
15 | @@ -124,6 +128,7 @@ |
16 | QMenu menu(NULL); |
17 | menu.addAction(m_pCreateCrateAction); |
18 | menu.addSeparator(); |
19 | + menu.addAction(m_pRenameCrateAction); |
20 | menu.addAction(m_pDeleteCrateAction); |
21 | menu.exec(globalPos); |
22 | } |
23 | @@ -175,3 +180,50 @@ |
24 | qDebug() << "Failed to delete crateId" << crateId; |
25 | } |
26 | } |
27 | + |
28 | +void CrateFeature::slotRenameCrate() { |
29 | + QString oldName = m_lastRightClickedIndex.data().toString(); |
30 | + int crateId = m_pTrackCollection->getCrateDAO().getCrateIdByName(oldName); |
31 | + |
32 | + QString newName; |
33 | + bool validNameGiven = false; |
34 | + |
35 | + do { |
36 | + bool ok = false; |
37 | + newName = QInputDialog::getText(NULL, |
38 | + tr("Rename Crate"), |
39 | + tr("New crate name:"), |
40 | + QLineEdit::Normal, |
41 | + oldName, |
42 | + &ok).trimmed(); |
43 | + |
44 | + if (!ok || newName == oldName) { |
45 | + return; |
46 | + } |
47 | + |
48 | + int existingId = m_pTrackCollection->getCrateDAO().getCrateIdByName(newName); |
49 | + |
50 | + if (existingId != -1) { |
51 | + QMessageBox::warning(NULL, |
52 | + tr("Renaming Crate Failed"), |
53 | + tr("A crate by that name already exists.")); |
54 | + } |
55 | + else if (newName.isEmpty()) { |
56 | + QMessageBox::warning(NULL, |
57 | + tr("Renaming Crate Failed"), |
58 | + tr("A crate cannot have a blank name.")); |
59 | + } |
60 | + else { |
61 | + validNameGiven = true; |
62 | + } |
63 | + } while (!validNameGiven); |
64 | + |
65 | + |
66 | + if (m_pTrackCollection->getCrateDAO().renameCrate(crateId, newName)) { |
67 | + m_crateListTableModel.select(); |
68 | + emit(featureUpdated()); |
69 | + m_crateTableModel.setCrate(crateId); |
70 | + } else { |
71 | + qDebug() << "Failed to rename crateId" << crateId; |
72 | + } |
73 | +} |
74 | |
75 | === modified file 'mixxx/src/library/cratefeature.h' |
76 | --- mixxx/src/library/cratefeature.h 2010-02-23 20:06:25 +0000 |
77 | +++ mixxx/src/library/cratefeature.h 2010-12-28 15:47:10 +0000 |
78 | @@ -41,11 +41,13 @@ |
79 | |
80 | void slotCreateCrate(); |
81 | void slotDeleteCrate(); |
82 | + void slotRenameCrate(); |
83 | |
84 | private: |
85 | TrackCollection* m_pTrackCollection; |
86 | QAction *m_pCreateCrateAction; |
87 | QAction *m_pDeleteCrateAction; |
88 | + QAction *m_pRenameCrateAction; |
89 | QSqlTableModel m_crateListTableModel; |
90 | CrateTableModel m_crateTableModel; |
91 | QModelIndex m_lastRightClickedIndex; |
92 | |
93 | === modified file 'mixxx/src/library/dao/cratedao.cpp' |
94 | --- mixxx/src/library/dao/cratedao.cpp 2010-10-19 01:35:26 +0000 |
95 | +++ mixxx/src/library/dao/cratedao.cpp 2010-12-28 15:47:10 +0000 |
96 | @@ -51,6 +51,25 @@ |
97 | return false; |
98 | } |
99 | |
100 | +bool CrateDAO::renameCrate(int crateId, const QString& newName) { |
101 | + qDebug() << "renameCrate()"; |
102 | + |
103 | + Q_ASSERT(m_database.transaction()); |
104 | + QSqlQuery query; |
105 | + query.prepare("UPDATE " CRATE_TABLE " SET name = :name WHERE id = :id"); |
106 | + query.bindValue(":name", newName); |
107 | + query.bindValue(":id", crateId); |
108 | + |
109 | + if (!query.exec()) { |
110 | + qDebug() << query.executedQuery() << query.lastError(); |
111 | + Q_ASSERT(m_database.rollback()); |
112 | + return false; |
113 | + } |
114 | + |
115 | + Q_ASSERT(m_database.commit()); |
116 | + return true; |
117 | +} |
118 | + |
119 | bool CrateDAO::deleteCrate(int crateId) { |
120 | Q_ASSERT(m_database.transaction()); |
121 | |
122 | |
123 | === modified file 'mixxx/src/library/dao/cratedao.h' |
124 | --- mixxx/src/library/dao/cratedao.h 2010-10-19 01:35:26 +0000 |
125 | +++ mixxx/src/library/dao/cratedao.h 2010-12-28 15:47:10 +0000 |
126 | @@ -23,6 +23,7 @@ |
127 | unsigned int crateCount(); |
128 | bool createCrate(const QString& name); |
129 | bool deleteCrate(int crateId); |
130 | + bool renameCrate(int crateId, const QString& newName); |
131 | int getCrateIdByName(const QString& name); |
132 | int getCrateId(int position); |
133 | QString crateName(int crateId); |
134 | |
135 | === modified file 'mixxx/src/library/dao/playlistdao.cpp' |
136 | --- mixxx/src/library/dao/playlistdao.cpp 2010-10-07 03:05:48 +0000 |
137 | +++ mixxx/src/library/dao/playlistdao.cpp 2010-12-28 15:47:10 +0000 |
138 | @@ -145,6 +145,27 @@ |
139 | //TODO: Crap, we need to shuffle the positions of all the playlists? |
140 | } |
141 | |
142 | + |
143 | +void PlaylistDAO::renamePlaylist(int playlistId, const QString& newName) |
144 | +{ |
145 | + qDebug() << "renamePlaylist()"; |
146 | + |
147 | + m_database.transaction(); |
148 | + QSqlQuery query(m_database); |
149 | + query.prepare("UPDATE Playlists SET name = :name WHERE id = :id"); |
150 | + query.bindValue(":name", newName); |
151 | + query.bindValue(":id", playlistId); |
152 | + |
153 | + if (!query.exec()) { |
154 | + qDebug() << query.executedQuery() << query.lastError(); |
155 | + m_database.rollback(); |
156 | + } |
157 | + else { |
158 | + m_database.commit(); |
159 | + } |
160 | +} |
161 | + |
162 | + |
163 | /** Append a track to a playlist */ |
164 | void PlaylistDAO::appendTrackToPlaylist(int trackId, int playlistId) |
165 | { |
166 | |
167 | === modified file 'mixxx/src/library/dao/playlistdao.h' |
168 | --- mixxx/src/library/dao/playlistdao.h 2010-09-12 20:00:07 +0000 |
169 | +++ mixxx/src/library/dao/playlistdao.h 2010-12-28 15:47:10 +0000 |
170 | @@ -17,6 +17,8 @@ |
171 | bool createPlaylist(QString name, bool hidden = false); |
172 | /** Delete a playlist */ |
173 | void deletePlaylist(int playlistId); |
174 | + /** Rename a playlist */ |
175 | + void renamePlaylist(int playlistId, const QString& newName); |
176 | /** Append a track to a playlist */ |
177 | void appendTrackToPlaylist(int trackId, int playlistId); |
178 | /** Find out how many playlists exist. */ |
179 | |
180 | === modified file 'mixxx/src/library/playlistfeature.cpp' |
181 | --- mixxx/src/library/playlistfeature.cpp 2010-11-25 01:06:46 +0000 |
182 | +++ mixxx/src/library/playlistfeature.cpp 2010-12-28 15:47:10 +0000 |
183 | @@ -27,6 +27,10 @@ |
184 | connect(m_pDeletePlaylistAction, SIGNAL(triggered()), |
185 | this, SLOT(slotDeletePlaylist())); |
186 | |
187 | + m_pRenamePlaylistAction = new QAction(tr("Rename"),this); |
188 | + connect(m_pRenamePlaylistAction, SIGNAL(triggered()), |
189 | + this, SLOT(slotRenamePlaylist())); |
190 | + |
191 | // Setup the sidebar playlist model |
192 | m_playlistTableModel.setTable("Playlists"); |
193 | m_playlistTableModel.setFilter("hidden=0"); |
194 | @@ -95,6 +99,7 @@ |
195 | QMenu menu(NULL); |
196 | menu.addAction(m_pCreatePlaylistAction); |
197 | menu.addSeparator(); |
198 | + menu.addAction(m_pRenamePlaylistAction); |
199 | menu.addAction(m_pDeletePlaylistAction); |
200 | menu.exec(globalPos); |
201 | } |
202 | @@ -143,6 +148,52 @@ |
203 | } |
204 | } |
205 | |
206 | +void PlaylistFeature::slotRenamePlaylist() |
207 | +{ |
208 | + qDebug() << "slotRenamePlaylist()"; |
209 | + |
210 | + QString oldName = m_lastRightClickedIndex.data().toString(); |
211 | + int playlistId = m_playlistDao.getPlaylistIdFromName(oldName); |
212 | + |
213 | + QString newName; |
214 | + bool validNameGiven = false; |
215 | + |
216 | + do { |
217 | + bool ok = false; |
218 | + newName = QInputDialog::getText(NULL, |
219 | + tr("Rename Playlist"), |
220 | + tr("New playlist name:"), |
221 | + QLineEdit::Normal, |
222 | + oldName, |
223 | + &ok).trimmed(); |
224 | + |
225 | + if (!ok || oldName == newName) { |
226 | + return; |
227 | + } |
228 | + |
229 | + int existingId = m_playlistDao.getPlaylistIdFromName(newName); |
230 | + |
231 | + if (existingId != -1) { |
232 | + QMessageBox::warning(NULL, |
233 | + tr("Renaming Playlist Failed"), |
234 | + tr("A playlist by that name already exists.")); |
235 | + } |
236 | + else if (newName.isEmpty()) { |
237 | + QMessageBox::warning(NULL, |
238 | + tr("Renaming Playlist Failed"), |
239 | + tr("A playlist cannot have a blank name.")); |
240 | + } |
241 | + else { |
242 | + validNameGiven = true; |
243 | + } |
244 | + } while (!validNameGiven); |
245 | + |
246 | + m_playlistDao.renamePlaylist(playlistId, newName); |
247 | + m_playlistTableModel.select(); |
248 | + emit(featureUpdated()); |
249 | + m_pPlaylistTableModel->setPlaylist(playlistId); |
250 | +} |
251 | + |
252 | void PlaylistFeature::slotDeletePlaylist() |
253 | { |
254 | //qDebug() << "slotDeletePlaylist() row:" << m_lastRightClickedIndex.data(); |
255 | |
256 | === modified file 'mixxx/src/library/playlistfeature.h' |
257 | --- mixxx/src/library/playlistfeature.h 2010-02-23 20:06:25 +0000 |
258 | +++ mixxx/src/library/playlistfeature.h 2010-12-28 15:47:10 +0000 |
259 | @@ -45,6 +45,7 @@ |
260 | |
261 | void slotCreatePlaylist(); |
262 | void slotDeletePlaylist(); |
263 | + void slotRenamePlaylist(); |
264 | |
265 | private: |
266 | PlaylistTableModel* m_pPlaylistTableModel; |
267 | @@ -52,6 +53,7 @@ |
268 | TrackDAO &m_trackDao; |
269 | QAction *m_pCreatePlaylistAction; |
270 | QAction *m_pDeletePlaylistAction; |
271 | + QAction *m_pRenamePlaylistAction; |
272 | QSqlTableModel m_playlistTableModel; |
273 | QModelIndex m_lastRightClickedIndex; |
274 | }; |
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.