Selective playlist name updates. Fixing Lp1837315 #2214

daschuer · 2019-07-21T18:12:41Z

This fixes https://bugs.launchpad.net/mixxx/+bug/1837315

The bug only occurs in the rare case, if two instances of Mixxx are running on the same DB.

…ed()

…playlist table on every update. This fixes lp1837315.

uklotzde · 2019-07-21T19:04:25Z

src/library/baseplaylistfeature.cpp


    int row = 0;
    for (QList<QPair<int, QString> >::const_iterator it = m_playlistList.begin();
         it != m_playlistList.end(); ++it, ++row) {
        int playlist_id = it->first;
        QString playlist_name = it->second;

-        if (selected_id == playlist_id) {
+        if (playlistId == playlist_id) {


The same name in camel-case and snake case notation. Who should understand this code?

uklotzde · 2019-07-21T19:19:31Z

Modifying the database concurrently is not supported. Why should we fix undefined behaviour for just this single use case and ignore all others?

Without a generic concept how to deal with this situation I don't see no value in such an attempt. Even worse it requires additional comments and introduces inconsistencies on the code level.

uklotzde · 2019-07-21T19:27:06Z

src/library/playlistfeature.cpp

+                break;
+            }
+        }
+        break;


This is a very strange way to fetch just the first element from the result set. It took me some time until I noticed that the outer loop is actually not a loop at all.

Yes, I think it is my leak of knowledge how to use the Qt API correct in this case. I we haves such constructs in other places where we loop through a table even though we expect only one or no rows. How can I fix this?

We expect at most a single row here. This can easily be expressed through the code.

DEBUG_ASSERT(playlistTableModel.rowCount() <= 1); if (playlistTableModel.rowCount() > 0) { ...

Ah, yes that's easy. Can we also fix the while loop above?

uklotzde · 2019-07-21T19:27:27Z

src/library/playlistfeature.cpp

+        for (auto it = m_playlistList.begin();
+                it != m_playlistList.end(); ++it) {
+            if (it->first == playlist_id) {
+                it->second = QString("%1 (%2) %3").arg(name,


This formatting code is now duplicated.

uklotzde · 2019-07-21T19:29:01Z

src/library/playlistfeature.h

-    void buildPlaylistList();
-    void decorateChild(TreeItem *pChild, int playlist_id);
+    void buildPlaylistList() override;
+    void updatePlaylistList(int playlist_id) override;


I don't understand what the difference between buildPlaylistList and updatePlaylistList is. Looking at their implementation both functions do fundamentally different things despite their very similar names.

buildPlaylistList(), the original function clears the list of playlists m_playlistList and populates it again with fresh data from the database. This function was also used to refresh single tree view items, without refreshing the whole tree view with the recently fetched database data.
updatePlaylistList(int playlist_id) now only fetches the requested playlist and updates this in m_playlistList and GUI.
This way both lists are still consistent and immune against external changes, the root cause of the crash.

Should I rename updatePlaylistList(int playlist_id) to refreshPlaylistInPlaylistList((int playlist_id)?
Or do you have a better idea to rename both?

The whole naming is confusing, i.e. "Playlist" vs. "List". But leave it as it is to keep the impact low.

refresh/reloadPlaylistInPlaylistList works for me.

List -> Listing, Summary, Overview, ... someone else should wrap their head around this.

uklotzde · 2019-07-21T19:29:54Z

src/library/setlogfeature.cpp

+                break;
+            }
+        }
+        break;


Once again a non-loop in disguise

daschuer · 2019-07-21T21:14:30Z

Modifying the database concurrently is not supported. Why should we fix undefined behaviour for just this single use case and ignore all others?

Because of the way this bug happens to me. First it was just a crash that strikes me and I had no Idea why. It took me a while to figure out that this is related to two Mixxx instances. I often start accidantally two Mixxx instances. Since I know now why it crashed, I like to have that fixed.
During seeking for the root cause I have found this solution of the two inconsistent tables in the GUI and the model. The change makes sure both tables are consistent and has also a small perfomance benefit. I am open to discuss the best solution for it.

Without a generic concept how to deal with this situation I don't see no value in such an attempt. Even worse it requires additional comments and introduces inconsistencies on the code level.

I wanted to keep the scope of this original crash fix limited. The concept is: Mixxx is the a the single DB user but it should at least not crash in the exceptional case of two concurrent access.

What are your ideas?

daschuer · 2019-07-21T22:12:52Z

OK, I hope it is better now.

src/library/playlistfeature.cpp

uklotzde · 2019-07-22T05:55:26Z

src/library/playlistfeature.cpp

+    }
+}
+
+void PlaylistFeature::reloadPlaylistInPlaylistList(int playlist_id) {


Please use camel case for new code, that's what the style guide recommends.

uklotzde · 2019-07-22T05:58:13Z

src/library/playlistfeature.cpp

+
+    DEBUG_ASSERT(playlistTableModel.rowCount() <= 1);
+    if (playlistTableModel.rowCount() > 0) {
+        for (auto it = m_playlistList.begin();


This loop now appears multiple times. Can't we extract it in a function named replacePlaylistLabel(id, label)? That would improve readability.

A better name for m_playlistList would actually be m_playlistLabels.

I noticed this only after your added the function for formatting the label. Before it wasn't obvious and hidden behind a pile of code. Inspecting the actual type in the header was just too far away.

And out of a sudden an appropriate naming appears ;)

buildPlaylistList -> createPlaylistLabels()
reloadPlaylistInPlaylistList -> updatePlaylistLabel(int playlistId)
How much effort would it be to change the naming?

Eclipse has a rename feature, so it should be no deal.

…e type

daschuer · 2019-07-25T22:14:58Z

done

uklotzde · 2019-07-27T08:39:25Z

src/library/baseplaylistfeature.cpp

-void BasePlaylistFeature::updateChildModel(int selected_id) {
-    buildPlaylistList();
+void BasePlaylistFeature::updateChildModel(int changedPlaylistId) {
+    // the following call fetches whole m_playlistList.


The comments is outdated and doesn't really help:

m_playlistList

How updatePlaylistLabel() actually works should not matter here

uklotzde · 2019-07-27T08:51:40Z

src/library/baseplaylistfeature.cpp

+void BasePlaylistFeature::updateChildModel(int changedPlaylistId) {
+    // the following call fetches whole m_playlistList.
+    // Anything may have change due to a concurrent Mixxx instance.
+    updatePlaylistLabel(changedPlaylistId);


The parameter changedPlaylistId is too verbose. This functions doesn't need to care what happened to the playlist before. It is instructed to update the tree item of a particular playlistId and that's it.

The functions should actually be named updatePlaylistItem(), because it updates the entire item and not only the label.

The code is still more complex than it needs to be:

QString updatedLabel = updatePlaylistLabel(playlistId); TreeItem* playlistItem = findPlaylistItem(playlistId); VERIFY_OR_DEBUG_ASSERT(playlistItem) { return; } DEBUG_ASSERT(playlistItem->getData() == playlistId); item->setLabel(updatedLabel); decorateChild(playlistItem, playlistId);

Is m_playlistLabels actually needed if all this information is stored redundantly in the TreeItems?

See also: CrateFeature and CrateSummary, no redundant and error-prone extra bookkeeping needed.

daschuer · 2019-07-27T17:37:44Z

OK, I have managed to dispose m_playlistLable finally.
A bit to much for a bugfix, but the root cause is entirely fixed now.

uklotzde · 2019-07-27T17:46:34Z

src/library/baseplaylistfeature.cpp

-void BasePlaylistFeature::updateChildModel(int selected_id) {
-    buildPlaylistList();
+void BasePlaylistFeature::updateChildModel(int playlistId) {
+    QString playlistLable = fetchPlaylistLabel(playlistId);


Typo: Lable -> Label
Please check all occurrences

uklotzde · 2019-07-27T17:52:50Z

src/library/baseplaylistfeature.h

    virtual QModelIndex constructChildModel(int selected_id);
    virtual void updateChildModel(int selected_id);
    virtual void clearChildModel();
-    virtual void buildPlaylistList() = 0;
-    virtual void decorateChild(TreeItem *pChild, int playlist_id) = 0;
+    virtual void createPlaylistLabels(QList<IdAndLabel>* pPlaylistLabels) = 0;


The QList can be returned by-value

daschuer · 2019-07-28T09:30:20Z

done

uklotzde · 2019-07-28T19:58:18Z

src/library/baseplaylistfeature.cpp

+        if (!pTreeItem->hasChildren()) { // leaf node
+            bool ok;
+            int playlistId = pTreeItem->getData().toInt(&ok);
+            if (ok) {


This conversion should never fail: VERIFY_OR_DEBUG_ASSERT(ok) { continue; }

uklotzde · 2019-07-28T20:08:28Z

src/library/baseplaylistfeature.cpp

-         it != m_playlistList.end(); ++it, ++row) {
-        int playlist_id = it->first;
-        QString playlist_name = it->second;
+    for (auto it = playlistLabels.constBegin();


Shorter and simpler variant:

for (const IdAndLabel& idAndLabel : createPlaylistLabels()) {

The actual container type doesn't matter for the iteration, so why depend on it.

I would even use const auto&& here and let the compiler do all the type checking. But I know that you prefer old-school explicit type declarations ;)

But don't forget to the row++ in the loop body. That's the only drawback.

uklotzde · 2019-07-28T20:22:59Z

Patching an unsuitable design by piling up more code on top of it doesn't work out in the long term. What started as an attempt to fix a rare bug by adding more code has become a real improvement that should prevent future errors. That's the right approach!

daschuer · 2019-07-28T21:36:36Z

done

daschuer · 2019-07-31T22:01:11Z

Thank you for review.

daschuer added 3 commits July 21, 2019 12:28

use auto and a const iterator in BasePlaylistFeature::slotTrackSelect…

4d904c5

…ed()

Don't emit a slot

8dcd5cb

Introduce a updatePlaylistList() function to avoid to read the whole …

57636ce

…playlist table on every update. This fixes lp1837315.

daschuer added this to the 2.2.2 milestone Jul 21, 2019

daschuer added the minor bug label Jul 21, 2019

Replace DEBUG_ASSERT by VERIFY_OR_DEBUG_ASSERT just to be sure.

3498726

uklotzde reviewed Jul 21, 2019

View reviewed changes

src/library/setlogfeature.cpp Outdated

break;

}

}

break;

Copy link

Contributor

uklotzde Jul 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again a non-loop in disguise

fix some naming and code duplication issues

a0b263f

uklotzde reviewed Jul 22, 2019

View reviewed changes

src/library/playlistfeature.cpp Show resolved Hide resolved

uklotzde reviewed Jul 22, 2019

View reviewed changes

daschuer added 5 commits July 25, 2019 21:26

Move QSqlTableModel out of createPlaylistLable

cab268c

use cammle case for playlistId

b0842e3

Add function replacePlaylistLabel()

bba569e

rename m_playlistList to m_playlistLabels and create a struct as stor…

b43f1e6

…e type

Rename more functions

60772f3

uklotzde reviewed Jul 27, 2019

View reviewed changes

daschuer added 4 commits July 27, 2019 12:17

Simplify names in updateChildModel() and remove outdates comment

3a15f58

Split out fetchPlaylistLabel

f504522

Make m_playlistLabels private

bef6c0d

dispose m_playlistLable

16702fb

uklotzde reviewed Jul 27, 2019

View reviewed changes

daschuer added 2 commits July 28, 2019 11:02

fix typo

157033a

return playlistLabels by value, a QList is implicitly shared.

f347f00

uklotzde reviewed Jul 28, 2019

View reviewed changes

user range base loop and a debug assert

2c31be1

uklotzde merged commit 0295027 into mixxxdj:2.2 Jul 31, 2019

daschuer deleted the lp1837315 branch September 26, 2021 17:36

mixxxbot mentioned this pull request Aug 23, 2022

DEBUG_ASSERT when changing tree view item #9697

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Selective playlist name updates. Fixing Lp1837315 #2214

Selective playlist name updates. Fixing Lp1837315 #2214

daschuer commented Jul 21, 2019

uklotzde Jul 21, 2019

uklotzde commented Jul 21, 2019

uklotzde Jul 21, 2019

daschuer Jul 21, 2019

uklotzde Jul 21, 2019

daschuer Jul 21, 2019

uklotzde Jul 21, 2019

uklotzde Jul 21, 2019

daschuer Jul 21, 2019

uklotzde Jul 21, 2019

uklotzde Jul 21, 2019

daschuer commented Jul 21, 2019

daschuer commented Jul 21, 2019

uklotzde Jul 22, 2019

uklotzde Jul 22, 2019

uklotzde Jul 22, 2019 •

edited

uklotzde Jul 24, 2019

daschuer Jul 24, 2019

daschuer commented Jul 25, 2019

uklotzde Jul 27, 2019

uklotzde Jul 27, 2019

uklotzde Jul 27, 2019 •

edited

uklotzde Jul 27, 2019

daschuer commented Jul 27, 2019

uklotzde Jul 27, 2019

uklotzde Jul 27, 2019

daschuer commented Jul 28, 2019

uklotzde Jul 28, 2019

uklotzde Jul 28, 2019

uklotzde Jul 28, 2019

uklotzde commented Jul 28, 2019

daschuer commented Jul 28, 2019

daschuer commented Jul 31, 2019

Selective playlist name updates. Fixing Lp1837315 #2214

Selective playlist name updates. Fixing Lp1837315 #2214

Conversation

daschuer commented Jul 21, 2019

Choose a reason for hiding this comment

uklotzde commented Jul 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 21, 2019

daschuer commented Jul 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde Jul 27, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer commented Jul 28, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uklotzde commented Jul 28, 2019

daschuer commented Jul 28, 2019

daschuer commented Jul 31, 2019

uklotzde Jul 22, 2019 •

edited

uklotzde Jul 27, 2019 •

edited