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.

Sign up for GitHub

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

Track metadata import: Rename enumeration and indicate modifications #3858

Merged
merged 2 commits into from
May 14, 2021
Merged

Track metadata import: Rename enumeration and indicate modifications #3858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
788b3a3
Track metadata import/update: Rename enumeration
uklotzde May 13, 2021
834980c
Indicate if modified/unmodified when importing track metadata
uklotzde May 13, 2021
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
30 changes: 17 additions & 13 deletions src/sources/soundsourceproxy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,9 @@ SoundSourceProxy::allProviderRegistrationsForUrl(

//static
ExportTrackMetadataResult
SoundSourceProxy::exportTrackMetadataBeforeSaving(Track* pTrack, UserSettingsPointer pConfig) {
SoundSourceProxy::exportTrackMetadataBeforeSaving(
Track* pTrack,
const UserSettingsPointer& pConfig) {
DEBUG_ASSERT(pTrack);
const auto fileInfo = pTrack->getFileInfo();
mixxx::MetadataSourcePointer pMetadataSource;
Expand Down Expand Up @@ -333,7 +335,7 @@ SoundSourceProxy::exportTrackMetadataBeforeSaving(Track* pTrack, UserSettingsPoi
} else {
kLogger.warning()
<< "Unable to export track metadata into file"
<< fileInfo.location();
<< fileInfo;
return ExportTrackMetadataResult::Skipped;
}
}
Expand Down Expand Up @@ -512,19 +514,19 @@ SoundSourceProxy::importTrackMetadataAndCoverImage(
pCoverImage);
}

void SoundSourceProxy::updateTrackFromSource(
ImportTrackMetadataMode importTrackMetadataMode) {
bool SoundSourceProxy::updateTrackFromSource(
UpdateTrackFromSourceMode mode) {
DEBUG_ASSERT(m_pTrack);

if (getUrl().isEmpty()) {
// Silently skip tracks without a corresponding file
return; // abort
return false; // abort
}
if (!m_pSoundSource) {
kLogger.warning()
<< "Unable to update track from unsupported file type"
<< getUrl().toString();
return; // abort
return false; // abort
}

// The SoundSource provides the actual type of the corresponding file
Expand All @@ -543,8 +545,7 @@ void SoundSourceProxy::updateTrackFromSource(
// if the user did not explicitly choose to (re-)import metadata
// explicitly from this file.
bool mergeImportedMetadata = false;
if (metadataSynchronized &&
(importTrackMetadataMode == ImportTrackMetadataMode::Once)) {
if (metadataSynchronized && mode == UpdateTrackFromSourceMode::Once) {
// No (re-)import needed or desired, only merge missing properties
mergeImportedMetadata = true;
}
Expand All @@ -570,7 +571,7 @@ void SoundSourceProxy::updateTrackFromSource(
<< getUrl().toString();
}
} else {
// (Re-)import embedded cover art
// Request reimport of embedded cover art
pCoverImg = &coverImg;
}
}
Expand All @@ -596,9 +597,11 @@ void SoundSourceProxy::updateTrackFromSource(
if (metadataImported.first == mixxx::MetadataSource::ImportResult::Succeeded) {
// Partial import of properties that are not (yet) stored
// in the database
m_pTrack->mergeImportedMetadata(trackMetadata);
} // else: Nothing to do if no metadata has been imported
return;
return m_pTrack->mergeImportedMetadata(trackMetadata);
} else {
// Nothing to do if no metadata has been imported
return false;
}
}

// Full import
Expand Down Expand Up @@ -675,7 +678,7 @@ void SoundSourceProxy::updateTrackFromSource(

// Do not continue with unknown and maybe invalid metadata!
if (metadataImported.first != mixxx::MetadataSource::ImportResult::Succeeded) {
return;
return false;
}

m_pTrack->importMetadata(trackMetadata, metadataImported.second);
Expand Down Expand Up @@ -708,6 +711,7 @@ void SoundSourceProxy::updateTrackFromSource(
m_pTrack->setCoverInfo(coverInfo);
}

return true;
}

mixxx::AudioSourcePointer SoundSourceProxy::openAudioSource(
Expand Down
11 changes: 7 additions & 4 deletions src/sources/soundsourceproxy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class SoundSourceProxy {

/// Controls which (metadata/coverart) and how tags are (re-)imported from
/// audio files when creating a SoundSourceProxy.
enum class ImportTrackMetadataMode {
enum class UpdateTrackFromSourceMode {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
enum class UpdateTrackFromSourceMode {
enum class UpdateTrackMetadataFromSourceMode {

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a class TrackMetadata that doesn't include cover art. Since we update the whole track including the cover info just Track is a better fit and it also closely mirrors the function name.

Copy link
Member

Choose a reason for hiding this comment

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

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The distinction between the different levels of track properties and their naming (TrackMetadata < TrackRecord) could still be improved. But this is not urgent as long as the names are used consistently.

// Import both track metadata and cover image once for new track objects.
// Otherwise the request is ignored and the track object is not modified.
Once,
Expand Down Expand Up @@ -139,8 +139,10 @@ class SoundSourceProxy {
/// too many possible reasons for failure to consider that cannot be handled
/// properly. The application log will contain warning messages for a detailed
/// analysis in case unexpected behavior has been reported.
void updateTrackFromSource(
ImportTrackMetadataMode importTrackMetadataMode = ImportTrackMetadataMode::Default);
///
/// Returns true if the track has been modified and false otherwise.
bool updateTrackFromSource(
UpdateTrackFromSourceMode mode = UpdateTrackFromSourceMode::Default);

/// Opening the audio source through the proxy will update the
/// audio properties of the corresponding track object. Returns
Expand All @@ -167,7 +169,8 @@ class SoundSourceProxy {

friend class TrackCollectionManager;
static ExportTrackMetadataResult exportTrackMetadataBeforeSaving(
Track* pTrack, UserSettingsPointer pConfig);
Track* pTrack,
const UserSettingsPointer& pConfig);

// Special case: Construction from a url is needed
// for writing metadata immediately before the TIO is destroyed.
Expand Down
2 changes: 1 addition & 1 deletion src/test/autodjprocessor_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ class AutoDJProcessorTest : public LibraryTest {
static TrackPointer newTestTrack(TrackId trackId) {
TrackPointer pTrack(
Track::newDummy(kTrackLocationTest, trackId));
SoundSourceProxy(pTrack).updateTrackFromSource();
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource());
return pTrack;
}

Expand Down
2 changes: 1 addition & 1 deletion src/test/coverartutils_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ TEST_F(CoverArtUtilTest, searchImage) {

// Looking for a track with embedded cover.
pTrack = Track::newTemporary(kTrackLocationTest);
SoundSourceProxy(pTrack).updateTrackFromSource();
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource());
CoverInfo result = pTrack->getCoverInfoWithLocation();
EXPECT_EQ(result.type, CoverInfo::METADATA);
EXPECT_EQ(result.source, CoverInfo::GUESSED);
Expand Down
14 changes: 8 additions & 6 deletions src/test/soundproxy_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ TEST_F(SoundSourceProxyTest, openEmptyFile) {
TEST_F(SoundSourceProxyTest, readArtist) {
auto pTrack = Track::newTemporary(kTestDir, "artist.mp3");
SoundSourceProxy proxy(pTrack);
proxy.updateTrackFromSource();
EXPECT_TRUE(proxy.updateTrackFromSource());
EXPECT_EQ("Test Artist", pTrack->getArtist());
}

Expand All @@ -223,31 +223,33 @@ TEST_F(SoundSourceProxyTest, readNoTitle) {
auto pTrack1 = Track::newTemporary(
kTestDir, "empty.mp3");
SoundSourceProxy proxy1(pTrack1);
proxy1.updateTrackFromSource();
EXPECT_TRUE(proxy1.updateTrackFromSource());
EXPECT_EQ("empty", pTrack1->getTitle());

// Test a reload also works
pTrack1->setTitle("");
proxy1.updateTrackFromSource(SoundSourceProxy::ImportTrackMetadataMode::Again);
EXPECT_TRUE(proxy1.updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Again));
EXPECT_EQ("empty", pTrack1->getTitle());

// Test a file with other metadata but no title
auto pTrack2 = Track::newTemporary(
kTestDir, "cover-test-png.mp3");
SoundSourceProxy proxy2(pTrack2);
proxy2.updateTrackFromSource();
EXPECT_TRUE(proxy2.updateTrackFromSource());
EXPECT_EQ("cover-test-png", pTrack2->getTitle());

// Test a reload also works
pTrack2->setTitle("");
proxy2.updateTrackFromSource(SoundSourceProxy::ImportTrackMetadataMode::Again);
EXPECT_TRUE(proxy2.updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Again));
EXPECT_EQ("cover-test-png", pTrack2->getTitle());

// Test a file with a title
auto pTrack3 = Track::newTemporary(
kTestDir, "cover-test-jpg.mp3");
SoundSourceProxy proxy3(pTrack3);
proxy3.updateTrackFromSource();
EXPECT_TRUE(proxy3.updateTrackFromSource());
EXPECT_EQ("test22kMono", pTrack3->getTitle());
}

Expand Down
29 changes: 15 additions & 14 deletions src/test/trackupdate_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class TrackUpdateTest: public MixxxTest {

static TrackPointer newTestTrackParsed() {
auto pTrack = newTestTrack();
SoundSourceProxy(pTrack).updateTrackFromSource();
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource());
EXPECT_TRUE(pTrack->isMetadataSynchronized());
EXPECT_TRUE(hasTrackMetadata(pTrack));
EXPECT_TRUE(hasCoverArt(pTrack));
Expand Down Expand Up @@ -59,18 +59,19 @@ TEST_F(TrackUpdateTest, parseModifiedCleanOnce) {
pTrack->readTrackMetadata(&trackMetadataBefore);
auto coverInfoBefore = pTrack->getCoverInfo();

SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::ImportTrackMetadataMode::Once);
// Re-update from source should have no effect
ASSERT_FALSE(SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Once));

mixxx::TrackMetadata trackMetadataAfter;
pTrack->readTrackMetadata(&trackMetadataAfter);
auto coverInfoAfter = pTrack->getCoverInfo();

// Not updated
EXPECT_TRUE(pTrack->isMetadataSynchronized());
EXPECT_FALSE(pTrack->isDirty());
EXPECT_EQ(trackMetadataBefore, trackMetadataAfter);
EXPECT_EQ(coverInfoBefore, coverInfoAfter);
// Verify that the track has not been modified
ASSERT_TRUE(pTrack->isMetadataSynchronized());
ASSERT_FALSE(pTrack->isDirty());
ASSERT_EQ(trackMetadataBefore, trackMetadataAfter);
ASSERT_EQ(coverInfoBefore, coverInfoAfter);
}

TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
Expand All @@ -81,8 +82,8 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainSkipCover) {
pTrack->readTrackMetadata(&trackMetadataBefore);
auto coverInfoBefore = pTrack->getCoverInfo();

SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::ImportTrackMetadataMode::Again);
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Again));

mixxx::TrackMetadata trackMetadataAfter;
pTrack->readTrackMetadata(&trackMetadataAfter);
Expand All @@ -107,8 +108,8 @@ TEST_F(TrackUpdateTest, parseModifiedCleanAgainUpdateCover) {
pTrack->readTrackMetadata(&trackMetadataBefore);
auto coverInfoBefore = pTrack->getCoverInfo();

SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::ImportTrackMetadataMode::Again);
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Again));

mixxx::TrackMetadata trackMetadataAfter;
pTrack->readTrackMetadata(&trackMetadataAfter);
Expand All @@ -128,8 +129,8 @@ TEST_F(TrackUpdateTest, parseModifiedDirtyAgain) {
pTrack->readTrackMetadata(&trackMetadataBefore);
auto coverInfoBefore = pTrack->getCoverInfo();

SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::ImportTrackMetadataMode::Again);
EXPECT_TRUE(SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::UpdateTrackFromSourceMode::Again));

mixxx::TrackMetadata trackMetadataAfter;
pTrack->readTrackMetadata(&trackMetadataAfter);
Expand Down
10 changes: 7 additions & 3 deletions src/track/track.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,16 @@ void Track::importMetadata(
}
}

void Track::mergeImportedMetadata(
bool Track::mergeImportedMetadata(
const mixxx::TrackMetadata& importedMetadata) {
QMutexLocker lock(&m_qMutex);
if (m_record.mergeImportedMetadata(importedMetadata)) {
markDirtyAndUnlock(&lock);
if (!m_record.mergeImportedMetadata(importedMetadata)) {
// Not modified
return false;
}
markDirtyAndUnlock(&lock);
// Modified
return true;
}

void Track::readTrackMetadata(
Expand Down
9 changes: 6 additions & 3 deletions src/track/track.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,9 +330,12 @@ class Track : public QObject {
void importMetadata(
mixxx::TrackMetadata importedMetadata,
const QDateTime& metadataSynchronized = QDateTime());
// Merge additional metadata that is not (yet) stored in the database
// and only available from file tags.
void mergeImportedMetadata(

/// Merge additional metadata that is not (yet) stored in the database
/// and only available from file tags.
///
/// Returns true if the track has been modified and false otherwise.
bool mergeImportedMetadata(
const mixxx::TrackMetadata& importedMetadata);

void readTrackMetadata(
Expand Down
2 changes: 1 addition & 1 deletion src/widget/wtrackmenu.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -908,7 +908,7 @@ class ImportMetadataFromFileTagsTrackPointerOperation : public mixxx::TrackPoint
// to override the information within Mixxx! Custom cover art must be
// reloaded separately.
SoundSourceProxy(pTrack).updateTrackFromSource(
SoundSourceProxy::ImportTrackMetadataMode::Again);
SoundSourceProxy::UpdateTrackFromSourceMode::Again);
}
};

Expand Down