Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TrackRef = Track Location + Track ID #901

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build/depends.py
Original file line number Diff line number Diff line change
Expand Up @@ -957,6 +957,7 @@ def sources(self, build):
"track/tracknumbers.cpp",
"track/trackmetadata.cpp",
"track/trackmetadatataglib.cpp",
"track/trackref.cpp",

"mixer/auxiliary.cpp",
"mixer/baseplayer.cpp",
Expand Down
15 changes: 8 additions & 7 deletions src/library/scanner/importfilestask.cpp
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#include "library/scanner/importfilestask.h"

#include "library/scanner/libraryscanner.h"
#include "track/trackref.h"
#include "util/timer.h"

ImportFilesTask::ImportFilesTask(LibraryScanner* pScanner,
Expand Down Expand Up @@ -29,27 +30,27 @@ void ImportFilesTask::run() {
return;
}

const QString filePath(fileInfo.filePath());
//qDebug() << "ImportFilesTask::run" << filePath;
const QString trackLocation(TrackRef::location(fileInfo));
//qDebug() << "ImportFilesTask::run" << trackLocation;

// If the file does not exist in the database then add it. If it
// does then it is either in the user's library OR the user has
// "removed" the track via "Right-Click -> Remove". These tracks
// stay in the library, but their mixxx_deleted column is 1.
if (m_scannerGlobal->trackExistsInDatabase(filePath)) {
if (m_scannerGlobal->trackExistsInDatabase(trackLocation)) {
// If the track is in the database, mark it as existing. This code gets
// executed when other files in the same directory have changed (the
// directory hash has changed).
emit(trackExists(filePath));
emit(trackExists(trackLocation));
} else {
if (!fileInfo.exists()) {
qWarning() << "ImportFilesTask: Skipping inaccessible file"
<< filePath;
<< trackLocation;
continue;
}
qDebug() << "Importing track" << filePath;
qDebug() << "Importing track" << trackLocation;

emit(addNewTrack(filePath));
emit(addNewTrack(trackLocation));
}
}
// Insert or update the hash in the database.
Expand Down
82 changes: 82 additions & 0 deletions src/test/trackreftest.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
#include <gtest/gtest.h>

#include <QDir>
#include <QTemporaryFile>
#include <QtDebug>

#include "track/trackref.h"

namespace {

class TrackRefTest : public testing::Test {
protected:

TrackRefTest()
: m_tempFile("TrackRefTest.tmp"),
m_tempFileDir(QDir::tempPath()),
m_tempFileName(m_tempFile.fileName()),
m_tempFileInfo(m_tempFileDir, m_tempFileName),
m_validTrackId(123) {
}

virtual void SetUp() {
ASSERT_TRUE(m_validTrackId.isValid());
ASSERT_FALSE(m_invalidTrackId.isValid());
}

virtual void TearDown() {
}

static void verifyFileInfo(const TrackRef& actual, const QFileInfo& fileInfo) {
EXPECT_TRUE(actual.hasLocation());
EXPECT_EQ(TrackRef::location(fileInfo), actual.getLocation());
EXPECT_TRUE(actual.hasCanonicalLocation());
EXPECT_EQ(TrackRef::canonicalLocation(fileInfo), actual.getCanonicalLocation());
}

const QTemporaryFile m_tempFile;
const QDir m_tempFileDir;
const QString m_tempFileName;
const QFileInfo m_tempFileInfo;
const TrackId m_validTrackId;
const TrackId m_invalidTrackId;
};

TEST_F(TrackRefTest, DefaultConstructor) {
TrackRef actual;

EXPECT_FALSE(actual.hasLocation());
EXPECT_FALSE(actual.hasCanonicalLocation());
EXPECT_FALSE(actual.hasId());
}

TEST_F(TrackRefTest, FromFileInfoWithId) {
const TrackRef actual(
TrackRef::fromFileInfo(m_tempFileInfo, m_validTrackId));

verifyFileInfo(actual, m_tempFileInfo);
EXPECT_TRUE(actual.hasId());
EXPECT_EQ(m_validTrackId, actual.getId());
}

TEST_F(TrackRefTest, FromFileInfoWithoutId) {
const TrackRef actual(
TrackRef::fromFileInfo(m_tempFileInfo));

verifyFileInfo(actual, m_tempFileInfo);
EXPECT_FALSE(actual.hasId());
EXPECT_EQ(m_invalidTrackId, actual.getId());
}

TEST_F(TrackRefTest, CopyAndSetId) {
const TrackRef withoutId(
TrackRef::fromFileInfo(m_tempFileInfo));

const TrackRef actual(withoutId, m_validTrackId);

verifyFileInfo(actual, m_tempFileInfo);
EXPECT_TRUE(actual.hasId());
EXPECT_EQ(m_validTrackId, actual.getId());
}

} // namespace
28 changes: 28 additions & 0 deletions src/track/trackref.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
#include "track/trackref.h"


bool TrackRef::verifyConsistency() const {
DEBUG_ASSERT_AND_HANDLE(hasLocation() || !hasCanonicalLocation()) {
return false;
}
DEBUG_ASSERT_AND_HANDLE(hasLocation() || !hasId()) {
return false;
}
return true;
}

std::ostream& operator<<(std::ostream& os, const TrackRef& trackRef) {
return os << '[' << trackRef.getLocation().toStdString()
<< " | " << trackRef.getCanonicalLocation().toStdString()
<< " | " << trackRef.getId()
<< ']';

}

QDebug operator<<(QDebug debug, const TrackRef& trackRef) {
debug.nospace() << '[' << trackRef.getLocation()
<< " | " << trackRef.getCanonicalLocation()
<< " | " << trackRef.getId()
<< ']';
return debug.space();
}
132 changes: 132 additions & 0 deletions src/track/trackref.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
#ifndef TRACKREF_H
#define TRACKREF_H


#include <QFileInfo>

#include "track/trackid.h"


// A track in the library is identified by a location and an id.
// The location is mandatory to identify the file, whereas the id
// only exists after the track has been inserted into the database.
//
// This class is intended to be used as a simple, almost immutable
// value object. Only the id can be set once.
class TrackRef {
public:
// All file-related track properties are snapshots from the provided
// QFileInfo. Obtaining them might involve accessing the file system
// and should be used consciously! The QFileInfo class does some
// caching behind the scenes.
// Please note that the canonical location of QFileInfo may change at
// any time, when the underlying file system structure is modified.
// It becomes empty if the file is deleted.
static QString location(const QFileInfo& fileInfo) {
return fileInfo.absoluteFilePath();
}
static QString canonicalLocation(const QFileInfo& fileInfo) {
return fileInfo.canonicalFilePath();
}

// Converts a QFileInfo and an optional TrackId into a TrackRef. This
// involves obtaining the file-related track properties from QFileInfo
// (see above) and should used consciously!
static TrackRef fromFileInfo(
const QFileInfo& fileInfo,
TrackId id = TrackId()) {
return TrackRef(
location(fileInfo),
canonicalLocation(fileInfo),
id);
}

// Default constructor
TrackRef() {
DEBUG_ASSERT(verifyConsistency());
}
// Regular copy constructor
TrackRef(const TrackRef& other) = default;
// Custom copy constructor: Creates a copy of an existing TrackRef,
// but overwrite the TrackId with a custom value.
TrackRef(
const TrackRef& other,
TrackId id)
: m_location(other.m_location),
m_canonicalLocation(other.m_canonicalLocation),
m_id(id) {
DEBUG_ASSERT(verifyConsistency());
}

// The human-readable identifier of a track in Mixxx. The location is
// immutable and the starting point for accessing a track's file.
const QString& getLocation() const {
return m_location;
}
bool hasLocation() const {
return !getLocation().isEmpty();
}

// The unique identifier of a track's file at runtime and used
// for caching. The canonical location is empty for inexistent
// files.
const QString& getCanonicalLocation() const {
return m_canonicalLocation;
}
bool hasCanonicalLocation() const {
return !getCanonicalLocation().isEmpty();
}

// The primary key of a track in the Mixxx library. The id must only
// be set once after inserting into or after loading from the database.
// Tracks that have not been stored in the database don't have an id.
const TrackId& getId() const {
return m_id;
}
bool hasId() const {
return getId().isValid();
}

bool isValid() const {
return hasId() || hasCanonicalLocation();
}
protected:
// Initializing constructor
TrackRef(
const QString& location,
const QString& canonicalLocation,
TrackId id = TrackId())
: m_location(location),
m_canonicalLocation(canonicalLocation),
m_id(id) {
DEBUG_ASSERT(verifyConsistency());
}

private:
bool verifyConsistency() const;

QString m_location;
QString m_canonicalLocation;
TrackId m_id;
};

inline
bool operator==(const TrackRef& lhs, const TrackRef& rhs) {
Copy link
Member

Choose a reason for hiding this comment

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

This is a candidate for missus since it might fail even if we reference the same track.
I would prefer to introduce a named function for comparing.
Like isRefferencingTheSameTrack() or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree. This is the canonical implementation of the equality operator
for a simple value object, member by member.

If on the other hand the class name of TrackRef is misleading than we should
instead choose a different one.

On 03/04/2016 04:02 PM, Daniel Schürmann wrote:

In src/track/trackref.h
#901 (comment):

  • }
  • bool isValid() const {
  •    return hasId() || hasCanonicalLocation();
    
  • }

+private:

  • bool verifyConsistency() const;
  • QString m_location;
  • QString m_canonicalLocation;
  • TrackId m_id;
    +};

+inline
+bool operator==(const TrackRef& lhs, const TrackRef& rhs) {

This is a candidate for missus since it might fail even if we reference
the same track.
I would prefer to introduce a named function for comparing.
Like isRefferencingTheSameTrack() or similar.


Reply to this email directly or view it on GitHub
https://github.com/mixxxdj/mixxx/pull/901/files#r55039820.

return (lhs.getId() == rhs.getId()) &&
(lhs.getLocation() == rhs.getLocation()) &&
(lhs.getCanonicalLocation() == rhs.getCanonicalLocation());
}

inline
bool operator!=(const TrackRef& lhs, const TrackRef& rhs) {
return !(lhs == rhs);
}

Q_DECLARE_METATYPE(TrackRef)

std::ostream& operator<<(std::ostream& os, const TrackRef& trackRef);

QDebug operator<<(QDebug debug, const TrackRef& trackRef);


#endif // TRACKREF_H
8 changes: 3 additions & 5 deletions src/trackinfoobject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,14 @@

#include "trackinfoobject.h"

#include "library/coverartutils.h"
#include "soundsourceproxy.h"
#include "track/beatfactory.h"
#include "track/keyfactory.h"
#include "track/keyutils.h"
#include "track/trackmetadatataglib.h"
#include "track/trackref.h"
#include "util/assert.h"
#include "util/compatibility.h"
#include "util/time.h"
#include "util/xml.h"


namespace {
Expand Down Expand Up @@ -155,15 +153,15 @@ QString TrackInfoObject::getLocation() const {
// (copy-on write). But operating on a single instance of QFileInfo
// might not be thread-safe due to internal caching!
QMutexLocker lock(&m_qMutex);
return m_fileInfo.absoluteFilePath();
return TrackRef::location(m_fileInfo);
}

QString TrackInfoObject::getCanonicalLocation() const {
// Copying QFileInfo is thread-safe due to "implicit sharing"
// (copy-on write). But operating on a single instance of QFileInfo
// might not be thread-safe due to internal caching!
QMutexLocker lock(&m_qMutex);
return m_fileInfo.canonicalFilePath();
return TrackRef::canonicalLocation(m_fileInfo);
}

QString TrackInfoObject::getDirectory() const {
Expand Down
4 changes: 2 additions & 2 deletions src/trackinfoobject.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,8 +62,6 @@ class TrackInfoObject : public QObject {
Q_PROPERTY(int duration READ getDuration WRITE setDuration)
Q_PROPERTY(QString durationFormatted READ getDurationText STORED false)

TrackId getId() const;

QFileInfo getFileInfo() const {
// Copying a QFileInfo is thread-safe (implicit sharing), no locking needed.
return m_fileInfo;
Expand All @@ -73,6 +71,8 @@ class TrackInfoObject : public QObject {
return m_pSecurityToken;
}

TrackId getId() const;

// Accessors for various stats of the file on disk.
// Returns absolute path to the file, including the filename.
QString getLocation() const;
Expand Down
17 changes: 9 additions & 8 deletions src/widget/wtracktableview.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,26 @@
#include <QDrag>
#include <QShortcut>

#include "widget/wwidget.h"
#include "widget/wtracktableview.h"

#include "widget/wcoverartmenu.h"
#include "widget/wskincolor.h"
#include "widget/wtracktableviewheader.h"
#include "widget/wwidget.h"
#include "library/coverartcache.h"
#include "library/librarytablemodel.h"
#include "library/trackcollection.h"
#include "library/dlgtrackinfo.h"
#include "track/trackref.h"
#include "trackinfoobject.h"
#include "controlobject.h"
#include "controlobjectslave.h"
#include "widget/wtracktableview.h"
#include "library/dlgtrackinfo.h"
#include "soundsourceproxy.h"
#include "mixer/playermanager.h"
#include "util/dnd.h"
#include "util/time.h"
#include "preferences/dialog/dlgpreflibrary.h"
#include "waveform/guitick.h"
#include "widget/wcoverartmenu.h"
#include "util/dnd.h"
#include "util/time.h"
#include "util/assert.h"

WTrackTableView::WTrackTableView(QWidget * parent,
Expand Down Expand Up @@ -1167,8 +1169,7 @@ void WTrackTableView::dropEvent(QDropEvent * event) {

QList<QString> fileLocationList;
foreach (const QFileInfo& fileInfo, fileList) {
// TODO(uklotzde): Replace with TrackRef::location()
fileLocationList.append(fileInfo.absoluteFilePath());
fileLocationList.append(TrackRef::location(fileInfo));
}

// Drag-and-drop from an external application
Expand Down