Fix the track order during drag and drop Lp1829601 #2237

daschuer · 2019-08-13T00:19:55Z

This fixes the original issue described in https://bugs.launchpad.net/mixxx/+bug/1829601
And introduces some de-duplication and some view update issues.

This can be merged after 2.2.2 if its too late to merge before.

… an a wron application layer anyway.

…se of it.

…tAndEnsureTrackIds()

uklotzde · 2019-08-13T10:45:26Z

We shouldn't rush these changes into the current release.

uklotzde

On hold until version 2.2.2 has been tagged.

daschuer · 2019-08-13T20:48:25Z

Yes, don't delay 2.2.2

uklotzde · 2019-08-15T20:27:09Z

src/library/trackcollection.h

@@ -66,6 +66,10 @@ class TrackCollection : public QObject,

    void relocateDirectory(QString oldDir, QString newDir);

+    // This function returns a track ID of all file in the list not already visible,
+    // it adds and unhides the tracks as well.
+    QList<TrackId> getAndEnsureTrackIds(const QList<QFileInfo>& files, bool addMissingTracks);


The name of this function has to be improved if you need to add detailed comments upon every usage. Also the name "get" doesn't fit if it is not read-only or idempotent.

This is actually the third name of this function. A strong hint that it does to much at once. :-/
Unfortunately this is required for a quick execution.
I was lately fine with this name, but I am happy to change it.

Ideas:
createOrGetTrackIds()
addMissingTracksAndGetAllTrackIds()

uklotzde · 2019-08-15T20:37:04Z

src/library/trackcollection.cpp

@@ -87,6 +87,13 @@ void TrackCollection::relocateDirectory(QString oldDir, QString newDir) {
    GlobalTrackCacheLocker().relocateCachedTracks(&m_trackDao);
 }

+QList<TrackId> TrackCollection::getAndEnsureTrackIds(


The function getTrackIds() has already a name that doesn't fit.

Since it is a lookup and mapping "resolveTrackIds()" comes into my mind. But I didn't check how to combine it with the 2 options addMissingTracks and unhideHiddenTracks.

The boolean parameter should be replace by an enum that represents a bitmask with the options ADD_MISSING_TRACKS and UNHIDE_HIDDEN_TRACKS.

The function name should contain the word "File", because we are going from files to ids.

But I'm still searching for the right verb that characterizes this operation in a single word. It's difficult, because the operation is so manifold. How about emplaceTrackFiles()? @Be-ing Any suggestions from a native speaker?

Maybe "resolveTrackIds()" it reads well in combination with the descriptive enum parameter.
I will try that.

daschuer · 2019-08-18T10:44:46Z

Done.

uklotzde · 2019-09-01T11:46:17Z

src/library/dao/trackdao.h

@@ -40,7 +48,7 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
    void finish();

    TrackId getTrackId(const QString& absoluteFilePath);
-    QList<TrackId> getTrackIds(const QList<QFileInfo>& files, bool addMissingTracks);
+    QList<TrackId> resolveTrackIds(const QList<QFileInfo>& files, ResolveTrackIdOptions options);


Should we add the default option = ResolveTrackIdOption::ResolveOnly? Would make sense here.

It is not obvious that options contains a mask of flags. Maybe a renaming could help (.h/.cpp)?
ResolveTrackIdOptions options -> ResolveTrackIdFlags flags

uklotzde · 2019-09-18T21:27:03Z

src/library/autodj/autodjfeature.cpp

-        m_pTrackCollection->unhideTracks(trackIds);
-    } else {
-        trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true);
+    if (!files.size()) {


uklotzde · 2019-09-18T21:27:55Z

src/library/autodj/autodjfeature.cpp

+        options |= TrackDAO::ResolveTrackIdOption::AddMissing;
+    }
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIds(files, options);
+    if (!trackIds.size()) {


uklotzde · 2019-09-18T21:28:16Z

src/library/crate/cratefeature.cpp

-        if (!trackIds.at(trackIdIndex).isValid()) {
-            trackIds.removeAt(trackIdIndex--);
-        }
+    if (!files.size()) {


uklotzde · 2019-09-18T21:28:37Z

src/library/crate/cratefeature.cpp

+        options |= TrackDAO::ResolveTrackIdOption::AddMissing;
+    }
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIds(files, options);
+    if (!trackIds.size()) {


src/library/dao/trackdao.cpp

uklotzde · 2019-09-18T21:32:09Z

src/library/dao/trackdao.h

@@ -40,7 +48,7 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
    void finish();

    TrackId getTrackId(const QString& absoluteFilePath);
-    QList<TrackId> getTrackIds(const QList<QFileInfo>& files, bool addMissingTracks);
+    QList<TrackId> resolveTrackIds(const QList<QFileInfo>& files, ResolveTrackIdOptions options);


uklotzde · 2019-09-18T21:33:01Z

src/library/playlistfeature.cpp

+        options |= TrackDAO::ResolveTrackIdOption::AddMissing;
+    }
+    QList<TrackId> trackIds = m_pTrackCollection->resolveTrackIds(files, options);
+    if (!trackIds.size()) {


uklotzde · 2019-09-18T21:33:11Z

src/library/playlistfeature.cpp

-        // playlist.
-        // Adds track, does not insert duplicates, handles unremoving logic.
-        trackIds = m_pTrackCollection->getTrackDAO().addMultipleTracks(files, true);
+    if (!files.size()) {


uklotzde · 2019-09-18T21:36:29Z

src/library/dao/trackdao.h

@@ -40,7 +48,7 @@ class TrackDAO : public QObject, public virtual DAO, public virtual GlobalTrackC
    void finish();

    TrackId getTrackId(const QString& absoluteFilePath);
-    QList<TrackId> getTrackIds(const QList<QFileInfo>& files, bool addMissingTracks);
+    QList<TrackId> resolveTrackIds(const QList<QFileInfo>& files, ResolveTrackIdOptions options);


It is not obvious that options contains a mask of flags. Maybe a renaming could help (.h/.cpp)?
ResolveTrackIdOptions options -> ResolveTrackIdFlags flags

uklotzde · 2019-09-18T21:38:41Z

src/library/autodj/autodjfeature.cpp

@@ -138,26 +138,26 @@ void AutoDJFeature::activate() {
 }

 bool AutoDJFeature::dropAccept(QList<QUrl> urls, QObject* pSource) {


The dropAccept()/dropAcceptChild() contain a lot of duplicated code (AutoDJ/Crates/Playlist). Is It possible to extract this code into a function?

…cations() to remove duplicates

daschuer · 2019-09-28T21:23:59Z

Done, hopefully :-)

Be-ing · 2019-09-29T14:09:45Z

@uklotzde ping. Do the latest changes address all your concerns?

uklotzde · 2019-10-03T20:35:11Z

@daschuer Please replace the remaining !size() with isEmpty().

daschuer · 2019-10-03T20:48:40Z

Done.

uklotzde · 2019-10-22T22:14:09Z

Some minor code style issues are still waiting to be fixed. The bug is fixed, though. LGTM.

I'll merge this now to finally close the PR and avoid that more merge conflicts with master appear! Resolving the merge conflicts is already tedious, just managed it for testing.

daschuer added 7 commits August 11, 2019 16:34

Remove unused unremove flag from addMultipleTracks()

af5a36f

Remove superfluid transaction around a single sql statment, which was…

5ad6d78

… an a wron application layer anyway.

introduce new function getAndEnsureTrackIds()

901095c

Use a temp table to keep sorting unchanged in TrackDAO::getTrackIds()

0e9b727

Improve query in TrackDAO::unhideTracks()

af85c1c

Add the addMissingTracks fature to TrackDAO::getTrackIds() and make u…

5eb8ac0

…se of it.

Completely replace obsolete TrackDAO::addMultipleTracks() calls by ge…

acb8444

…tAndEnsureTrackIds()

daschuer added the major bug label Aug 13, 2019

uklotzde added this to the 2.2.x milestone Aug 13, 2019

uklotzde suggested changes Aug 13, 2019

View reviewed changes

daschuer modified the milestones: 2.2.x, 2.2.3 Aug 15, 2019

uklotzde reviewed Aug 15, 2019

View reviewed changes

Renamed resolveTrackIds() and add enum fag option parameter

29b81a7

uklotzde reviewed Sep 1, 2019

View reviewed changes

uklotzde added the library label Sep 1, 2019

uklotzde reviewed Sep 18, 2019

View reviewed changes

daschuer added 4 commits September 28, 2019 22:13

Add new functions resolveTrackIdsFromUrls() and resolveTrackIdsFromLo…

2916740

…cations() to remove duplicates

rename options -> flags

ea855d6

predefere isEmpty() over !size()

ad73dba

rename onHideTracks to hideTracks

ec0b354

change remaining !size() to isEmpty()

04b26e4

uklotzde merged commit cb9abf2 into mixxxdj:2.2 Oct 22, 2019

daschuer deleted the lp1829601 branch September 26, 2021 17:37

mixxxbot mentioned this pull request Aug 23, 2022

AutoDJ mix cant save to playslist in same order? #9661

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the track order during drag and drop Lp1829601 #2237

Fix the track order during drag and drop Lp1829601 #2237

daschuer commented Aug 13, 2019

uklotzde commented Aug 13, 2019

uklotzde left a comment

daschuer commented Aug 13, 2019

uklotzde Aug 15, 2019 •

edited

Loading

daschuer Aug 18, 2019

uklotzde Aug 15, 2019

uklotzde Aug 17, 2019

daschuer Aug 18, 2019

daschuer commented Aug 18, 2019

uklotzde Sep 1, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 29, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 29, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 29, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

uklotzde Sep 18, 2019

daschuer commented Sep 28, 2019

Be-ing commented Sep 29, 2019

uklotzde commented Oct 3, 2019

daschuer commented Oct 3, 2019

uklotzde commented Oct 22, 2019

		@@ -138,26 +138,26 @@ void AutoDJFeature::activate() {
		}

		bool AutoDJFeature::dropAccept(QList<QUrl> urls, QObject* pSource) {

Fix the track order during drag and drop Lp1829601 #2237

Fix the track order during drag and drop Lp1829601 #2237

Conversation

daschuer commented Aug 13, 2019

uklotzde commented Aug 13, 2019

uklotzde left a comment

Choose a reason for hiding this comment

daschuer commented Aug 13, 2019

uklotzde Aug 15, 2019 • edited Loading

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 Aug 18, 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

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 Sep 28, 2019

Be-ing commented Sep 29, 2019

uklotzde commented Oct 3, 2019

daschuer commented Oct 3, 2019

uklotzde commented Oct 22, 2019

uklotzde Aug 15, 2019 •

edited

Loading