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

Prepare multi-valued genre and mood tags #4101

Merged
merged 7 commits into from
Aug 16, 2021
Merged

Prepare multi-valued genre and mood tags #4101

merged 7 commits into from
Aug 16, 2021

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Jul 12, 2021

The singular genre (and mood) track properties will become derived text fields that are split by a separator into tags when set and that are joined with a separator from multiple tags when read.

The configuration including the separator will be stored in and managed by TrackCollectionManager.

@uklotzde uklotzde added this to the 2.4.0 milestone Jul 12, 2021
@uklotzde uklotzde requested a review from Holzhaus July 12, 2021 14:46
@uklotzde uklotzde changed the title [WiP] Pepare multi-valued genre and mood tags Pepare multi-valued genre and mood tags Jul 12, 2021
@coveralls
Copy link

coveralls commented Jul 12, 2021

Pull Request Test Coverage Report for Build 1132945886

  • 46 of 82 (56.1%) changed or added relevant lines in 12 files are covered.
  • 3 unchanged lines in 3 files lost coverage.
  • Overall coverage increased (+0.01%) to 25.993%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/library/banshee/bansheeplaylistmodel.cpp 0 1 0.0%
src/library/baseexternalplaylistmodel.cpp 0 1 0.0%
src/library/baseexternaltrackmodel.cpp 0 1 0.0%
src/library/basesqltablemodel.cpp 0 1 0.0%
src/library/dao/trackdao.cpp 34 35 97.14%
src/skin/qml/qmlplayerproxy.cpp 0 1 0.0%
src/library/basetracktablemodel.cpp 0 2 0.0%
src/library/proxytrackmodel.cpp 0 2 0.0%
src/library/browse/browsetablemodel.cpp 0 3 0.0%
src/library/trackcollectionmanager.cpp 0 3 0.0%
Files with Coverage Reduction New Missed Lines %
src/library/dao/trackdao.cpp 1 44.01%
src/library/proxytrackmodel.cpp 1 0%
src/track/taglib/trackmetadata_id3v2.cpp 1 61.63%
Totals Coverage Status
Change from base Build 1127725238: 0.01%
Covered Lines: 20019
Relevant Lines: 77017

💛 - Coveralls

@Holzhaus Holzhaus changed the title Pepare multi-valued genre and mood tags Prepare multi-valued genre and mood tags Jul 13, 2021
/// making it private.
void setGenreTextInternal(
const QString& genreText);
bool updateGenreText(
Copy link
Member

Choose a reason for hiding this comment

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

Is this method is the replacement for setGenreText? Can you elaborate why it's named differently and not exposed as a setter in QmlPlayerProxy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...because we will need TrackCollectionManager for the custom tag mapping config. This context is not available in individual track objects.

@@ -1041,3 +1041,17 @@ TrackId BaseTrackTableModel::doGetTrackId(
const TrackPointer& pTrack) const {
return pTrack ? pTrack->getId() : TrackId();
}

bool BaseTrackTableModel::updateTrackGenreText(
Copy link
Member

Choose a reason for hiding this comment

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

This code is duplicated multiple times, but I guess we can't add it to trackmodel.h because it don't have a reference to the track collection manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TrackCollectionManager stores the custom tag mapping config and is only available in implementations.

@uklotzde uklotzde changed the title Prepare multi-valued genre and mood tags [WiP] Prepare multi-valued genre and mood tags Jul 14, 2021
@uklotzde uklotzde marked this pull request as draft July 14, 2021 09:06
The singular genre (and mood) track properties will become derived text
fields that are split by a separator into tags when set and that are
joined with a separator from multiple tags when read.

The configuration including the separator will be stored in and managed
by TrackCollection.
@uklotzde
Copy link
Contributor Author

I have reverted the renaming by removing the "Text" suffix from genre and mood properties.

TODO: We need to find a way to submit new genre/mood property values from QML after editing tracks. These properties cannot be set directly on a single Track object. Maybe we can store the context (= TrackCollectionManager) in the corresponding QML proxy?

That's a typical use case that demonstrates why bidirectional data bindings only work for trivial use cases.

@uklotzde uklotzde changed the title [WiP] Prepare multi-valued genre and mood tags Prepare multi-valued genre and mood tags Jul 31, 2021
@uklotzde uklotzde marked this pull request as ready for review July 31, 2021 18:26
@uklotzde uklotzde mentioned this pull request Aug 7, 2021
11 tasks
Comment on lines 221 to 230
/// !!!DO NOT USE!!!
/// Set the genre text WITHOUT updating the corresponding custom tags.
///
/// Only allowed to be used by TrackDAO!!! Unfortunately, the
/// design of TrackDAO does not allow to hide this method by
/// making it private.
void setGenreInternal(
const QString& genre);
bool updateGenre(
const QString& genre);
Copy link
Member

Choose a reason for hiding this comment

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

The comment doesn't make it clear to me if it only applies to setGenreInternal or to updateGenre as well. Maybe add a blank line between them and a separate doc comment.

Should we make the interal method private/protected and make TrackDAO a friend class to prevent wrong usage?

Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

@Holzhaus Holzhaus merged commit 2f43bb8 into mixxxdj:main Aug 16, 2021
@uklotzde uklotzde deleted the genre-and-mood-text branch August 16, 2021 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants