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

[Don't merge / Obsolete] Custom Tags #2656

Closed
wants to merge 15 commits into from
Closed

[Don't merge / Obsolete] Custom Tags #2656

wants to merge 15 commits into from

Conversation

uklotzde
Copy link
Contributor

@uklotzde uklotzde commented Apr 12, 2020

2021-05-13 I have included another preliminary database update that adds a time stamp for metadata synchronization (import/export). This requires to reapply schema update 37 and 38, either by resetting the version number manually or by starting Mixxx 2.3 at least once. The next restart will then reapply all preliminary schema updates and add the new column.

This PR includes a new approach on how to organize the code for individual features. It isolates (as far as possible) all the required pieces including database access and UI components/utilities in a separate folder instead of scattering them over the whole code base.

TODO

  • Add last_played_at column to database (Library: Add last_played_at column #3140, database schema v34)
  • Integrate preliminary database schema changes for cover art (CoverArt: SHA-256 digest and (background) color #2524)
  • Modal progress dialog for batch processing (WTrackMenu: Display a modal progress dialog #2899)
  • Switch build from SCons to CMake (Remove SCons #2777)
  • Copy&Paste custom tags between tracks
  • Map and synchronize genre tags with genre text (also mood but not yet displayed or used)
  • Store star rating in a custom tag with the facet rating and the label mixxx.org as a score value
  • Edit tag score in track property dialog
  • Add UI support for unlabeled, faceted tags that only have a score (Valence, Energy, ...) Not in the first step, maybe later
  • Use a custom JSON format for the menu entries that allow extensions for faceted tags, i.e. enable scoring and a custom display name instead of the facet identifier
  • BUG: Modifications in DlgTrackInfo are not saved. Probably caused by a previous merge conflict resolution, no time to investigate yet.

Related issues

https://bugs.launchpad.net/mixxx/+bug/1831227

Custom Tags

Support for aoide-like (faceted) tags in Mixxx:

Screenshot from 2020-09-13 21-57-04

Disclaimer: This PR is intended to provide

  • rudimentary editing capabilities,
  • database synchronization, and
  • file tag synchronization (import/export)!

No fancy UI and no query support or further integration into Mixxx. Just the foundation to manage and synchronize custom tags, compatible with the aoide data model.

Build

Requires CMake, SCons is no longer supported.

Configuration

The assignment of tags to tracks is currently handled by an additional context menu, adopted from the crates selection menu. This menu is populated with the contents of the file custom_tags.json, loaded from the Mixxx settings folder. An example set of predefined tags is provided in preferences/custom_tags_example.json. Use this as a starting point for your own tagging system.

Update: No need to edit the JSON file manually. It will be created and both facets and labels can be added from the context menu.
edit_custom_tags

Tagging

A custom tag is a triple with 3 elements:

(facet, label, score)
  • facet:
    • Type: String (Identifier)
    • Default value: None
    • Optional: Yes, if a label is present
    • Value constraints: Lowercase ASCII without spaces QRegularExpression("[\\x{0021}-\\x{0040}\\x{005B}-\\x{007E}])"
  • label:
    • Type: String (Text)
    • Default value: None
    • Optional: Yes
    • Value constraints: No leading/trailing whitespace
  • score:
    • Type: Double
    • Default value: 1.0
    • Optional: No
    • Value constraints: Normalized to the unit interval [0.0, 1.0]

Tags are composed of a facet identifier (optional) and a label text. Tags without a facet are called plain tags in contrast to faceted tags. The facet associates a tag with a predefined category, while the label assigns the actual content.

Facet identifiers are restricted to lowercase strings without whitespaces. These identifiers could be translated into human-readable strings for display purposes. How and where this is done needs to be defined. These display strings belong to the presentation layer and will not become part of the data model.

The optional score value in the range [0.0, 1.0] defines a weight or strength of the relationship. It defaults to 1.0 when unspecified.

Faceted tags may only have a score and no label. This is useful for encoding anonymous analysis features like energy = 0.75 that are represented by a facet and a score.

Searching

It works, sort of. It is just a temporary hack.

Only tag labels are filtered, facets and scores are ignored. The resulting subselect (if not empty) is added to the main query with OR if the query contains no tag-specific search terms. If the query contains at least one token "label:" then AND will be used.

File Tags

Custom tags are permanently saved (import/export) as UTF-8 JSON and encoded into custom string fields in ID3v2, MP4, and Xiph/VorbisComment (FLAG, OGG, Opus) file tags. This allows to restore the newly added track_custom_tags table from file tags whenever needed.

ID3v2: GEOB frame

Description: "Mixxx CustomTags"
MIME type: "application/json"
Filename: empty
Data: UTF-8

$ eye3D <filename.mp3>

GEOB: [Size: 101 bytes] [Type: application/json]
Description: Mixxx CustomTags
Filename: 

MP4: Atom

Key "----:<mean>:<name>": "----:org.mixxx.dj:CustomTags"

$ exiftool -l <filename.m4a>

CustomTags
      {"occasion":["Cruise"]}

$ AtomicParsley <filename.m4a> -t

Atom "----" [org.mixxx.dj;CustomTags] contains: {"occasion":["Cruise"]}

Xiph/VorbisComment

Field name: "MIXXX_CUSTOM_TAGS"

$ exiftool -l <filename.flac/.ogg/.opus>

Mixxx Custom Tags
      {"dj-set":[PeakTime"],"":["Top40"]}

$ metaflac --list <filename.flac>

    comment[16]: MIXXX_CUSTOM_TAGS={"dj-set":["PeakTime"],"":["Top40"]}

@uklotzde uklotzde added this to the 2.4.0 milestone Apr 12, 2020
@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

The Custom Tags right click menu closes when a tag is clicked. This is inconvenient. Maybe this happens because you are popping up a modal dialog? I don't understand the need for this modal. It is shown so briefly that it just looks like a graphical glitch.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 12, 2020

The Custom Tags right click menu closes when a tag is clicked. This is inconvenient. Maybe this happens because you are popping up a modal dialog? I don't understand the need for this modal. It is shown so briefly that it just looks like a graphical glitch.

I noticed this, too. Don't know if QWidgetAction is used correctly here and how to batch changes before they are applied.

The modal dialog is necessary to monitor progress when updating multiple selected tracks. This takes some time. We need some kind of feedback that Mixxx is currently missing for all selected track actions. This was a first try to achieve this.

@uklotzde
Copy link
Contributor Author

Select >= 20-30 tracks at once and you will see the progress bar.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I don't understand why we need to give feedback to the user on the progress of this even if it takes more than a second. What does it matter to the user? Can't they just keep going using Mixxx? The task will be done soon enough in the background, right?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I don't think a modal dialog is appropriate unless the task in progress should prevent doing anything else in Mixxx. We sure could use a progress bar somewhere in the library GUI which we could also use for batch analysis, but that's a separate topic from this PR.

@uklotzde
Copy link
Contributor Author

Freezing the UI is inconvenient. Remember, you could easily select all/thousands of tracks and accidentally trigger this action.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

Why would the GUI freeze? Is that because of the current database architecture with DB queries in the GUI thread? If I understand correctly, that problem will be obsolete with Aoide?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

you could easily select all/thousands of tracks and accidentally trigger this action.

Not really. Sure, you could accidentally right click. But accidentally right click then navigate multiple levels through a nested menu? Unlikely.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I used the menu to add a custom tag. I did not see a way to add a faceted tag through this menu as I wanted. I expected to see my new custom tag appear the next time I opened the menu, but it did not.

@uklotzde
Copy link
Contributor Author

The UI stalls will not disappear magically. We still need to decouple UI interaction from executed tasks.

The menu is currently read-only and populated solely from the JSON file. Another option would be to populate it from the database from all existing tracks, but the number of tags could be come huge.

I think about adding tags entered manually with "Custom..." permanently to the menu by saving them into the JSON file. This is a simple and effective extension that does not involve any database interaction. If you want to delete them from the menu, you still need to edit the JSON file manually.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 12, 2020

Still build errors. I had to include one commit from @Holzhaus Serato PR to avoid merge conflicts. Not your fault, Jan ;) I messed around.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I think about adding tags entered manually with "Custom..." permanently to the menu by saving them into the JSON file. This is a simple and effective extension that does not involve any database interaction. If you want to delete them from the menu, you still need to edit the JSON file manually.

👍

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 12, 2020

@Be-ing Done. Manually entered tag labels ("Custom...") are now permanently saved into your configuration file.

Faceted tags without a score like "energy" or "valence" need to be handled differently. Probably by configuring which facets only have a score without a label. How do we add/remove those tags and select/assign a score to them?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I'm unclear what the long term plan is for this JSON file. Is this JSON file the same format that Aoide will provide when asked for the list of all tags?

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

How do we add/remove those tags and select/assign a score to them?

Can you hack a QSlider into a QMenu item?

@uklotzde
Copy link
Contributor Author

I'm unclear what the long term plan is for this JSON file. Is this JSON file the same format that Aoide will provide when asked for the list of all tags?

No. This is just the Mixxx tagging format that is also written into file tags. A JSON representation of the CustomTags class. I used it as a starting point. Basically the menu contains just a selection of all tags assigned at once, a hack. This format does not allow to apply a custom ordering among the available tags, nor does it allow to specify a list of multiple, predefined score values for a single faceted tag like energy or valence. Different requirements require different formats.

@uklotzde
Copy link
Contributor Author

The menu is only a quick proof-of-concept to access and modify the underlying data. It is supposed to be adapted and extended in the future. Maybe we exchange it with a more suitable UI paradigm when available. But at least we have some tools to start experimenting now.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

Why are faceted versus plain tags explicitly separated in the data format? Couldn't you implicitly figure out which tags are "plain" by them not having a label? I'm also unclear if the data model supports assigning a faceted tag without picking a label for it. This is something I would want. For example, I may want to apply a "house" tag, but not know what subgenres to assign it.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

The menu is only a quick proof-of-concept to access and modify the underlying data. It is supposed to be adapted and extended in the future. Maybe we exchange it with a more suitable UI paradigm when available. But at least we have some tools to start experimenting now.

Well, the proof-of-concept seems to work. If you don't want to improve the proof-of-concept GUI to fully implement all features, I think it would make sense to start working on the actual GUI. As discussed on Zulip, I think imitating Rekordbox's example would be good here. We can have a QTreeView popout panel activated by a button to the right of the library that would be independent of both WTrackTable and WLibrarySidebar. Cramming this into the already conceptually confused WLibrarySidebar would exacerbate the flaws of the current design. We don't need to overhaul the library GUI design all at once if we add new widgets separate from the current ones. To make it functional we just need to get WTrackTable to communicate the currently selected tracks to the new widget.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 12, 2020

Why are faceted versus plain tags explicitly separated in the data format? Couldn't you implicitly figure out which tags are "plain" by them not having a label? I'm also unclear if the data model supports assigning a faceted tag without picking a label for it. This is something I would want. For example, I may want to apply a "house" tag, but not know what subgenres to assign it.

I guess an empty key "" in a JSON object map is illegal and it would be awkward and error prone. That's why plain tags (without a faceted) need to be stored separately. Internally we store everything in a single map, where the entry with the empty facet contains the plain tags.

Plain tag

  • Has no facet (for the internal representation in a single map an empty facet is used as the key)
  • Must have a non-empty label
  • May have a score (defaults to 1.0)

Faceted tag

  • Must have a non-empty facet
  • May have a label (optional, i.e. might be empty)
  • May have a score (defaults to 1.0)

Each combination of the tuple (facet, label) may appear only once in CustomTags. That's why faceted tags without a label and only a score are not supported by the menu. A faceted tag with an empty label could appear only once in CustomTags, due to this uniqueness constraints. You cannot show multiple score values for the same facet. That's why CustomTags is not sufficient if we need to represent more entries in the menu.

You have 3 options for a faceted tag:

No label, explicit score (mandatory if no label is specified):

"facets": {
    "house": <score>
}

Very special case: "house": 1.0 is equivalent to "house: []". The empty array is parsed as a default plain tag with an empty label and default score of 1.0.

With label, implicit score (= 1.0):

"facets": {
    "house": "My label"
}

With label and explicit score:

"facets": {
    "house": ["My label", <score>]
}

You could use 1-element arrays even for the first two variants. But the parser is tolerant and allows to omit the array structure in this case. The general structure for the plain tag part is [<label>, <score>] where both array elements are optional and for 1-element arrays even the array itself could be omitted.

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

I guess an empty key "" in a JSON object map is illegal

Doesn't seem so. I ran this on a Node REPL:

> JSON.parse('{ "whatever": "" }')
{ whatever: '' }

Couldn't you have:

facet: {
    "house": ""
}

for a faceted tag without a label and implicit score? I don't know if this concept could fit well into a QMenu. Here is a mockup of how I think this could look in the tagging panel, with some cutting and pasting done to a screenshot from Rekordbox. "Components" is checked, but none of the subcategories are. Of course, the meaning in this particular example is nonsensical, but if the the tag name was "house" or "jazz" it could make sense:
tag-panel-mockup

Each combination of the tuple (facet, label) may appear only once in CustomTags. That's why faceted tags without a label and only a score are not supported by the menu. A faceted tag with an empty label could appear only once in CustomTags, due to this uniqueness constraints. You cannot show multiple score values for the same facet. That's why CustomTags is not sufficient if we need to represent more entries in the menu.

So is the problem distinguishing between:

facet: {
    "house": ["", 0.5]
}

and

facet: {
   "house": ["deep", 0.75]
}

@Be-ing
Copy link
Contributor

Be-ing commented Apr 12, 2020

Perhaps one way to handle this would be to treat an empty string as a special label.

@uklotzde
Copy link
Contributor Author

uklotzde commented Apr 12, 2020

Valid JSON:

{
	"": [
		"Top40",
		["A plain tag label with a score", 0.45]
	],
	"genre": [
		["House", 0.5],
		["Deep House", 0.75]
	],
	"house": [
		0.5,
		["Deep", 0.75]
	]
}

In this example I have included two different options how to organize genre tags. I would prefer the first variant, but you are free to use whatever works for you.

Ok, let's think about removing the "plain"/"facets" distinction if this is more intuitive. Thanks for the suggestions, @Be-ing 👍

@uklotzde
Copy link
Contributor Author

If it loads it is a valid set of tags that could also be assigned to a single track.

@uklotzde
Copy link
Contributor Author

Remark: aoide uses "genre" facets and doesn't have a dedicated genre field. In Mixxx these will be mapped (= concatenated) into a single string. This is a special case we might need to think about. Since there is a genre field in file tags we need to provide a bridge anyway. This is what the MultiGenreTagger in #2282 accomplishes.

@uklotzde
Copy link
Contributor Author

Perhaps one way to handle this would be to treat an empty string as a special label.

Empty strings should already be parsed as an empty facet or label. I will add a test case.

@uklotzde
Copy link
Contributor Author

I have modified the JSON structure as suggested and transformed it into a single object without a syntactical distinction between plain and faceted tags. Plain tags are stored in the array behind the empty string key. This also helped to remove some special case handling in the code. See the example file in res/preferences how it looks like.

@uklotzde
Copy link
Contributor Author

I found a clever solution for suppressing the progress dialog during short processing times! You can now pass a grace period (default = 1000ms) to the track processor. The modal progress dialog is only shown if processing time exceeds this period. Works great.

@uklotzde uklotzde modified the milestones: 2.4.0, 2.5.0 Jul 5, 2021
@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2021

This feature won't be ready for 2.4.0 without a decent UI.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 5, 2021

I think it would be fine for its initial release to have a clunky proof-of-concept GUI that doesn't utilize its full capabilities.

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2021

I consider both the database backend and file tag import/export as finished and stable while the stored configuration data depends on the yet unfinished UI.

Let's aim for more frequent releases and avoid the mistakes from the past. We should probably focus on other, more important features to enable a quick 2.4 release with the new QJSEngine. Releasing a half-baked feature that then requires to migrate configuration data would cause more work in the future.

@Holzhaus
Copy link
Member

Holzhaus commented Jul 5, 2021

Also, do you need the UI for testing? If not we could merge it without one and add the UI later. That way you won't run into merge conflicts and we make sure the code builds on all OSes.

@Holzhaus
Copy link
Member

Holzhaus commented Jul 5, 2021

Ah OK, commenting race-condition.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 5, 2021

If you split off the GUI changes, we can merge the application logic code and potentially experiment with QML GUIs instead.

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2021

I expect changes on how to store the user settings that are used for populating the track context menu. Currently this is just a reinterpretation of the JSON serialization instead of a dedicated format. It is sufficient for the simple use cases, but doesn't cover tags with an adjustable score or a discrete number of predefined score values.

@uklotzde
Copy link
Contributor Author

uklotzde commented Jul 5, 2021

I am not sure if it is a good idea to merge unused code before we finish the actual feature. If we then find out that we need substantial changes we have to do a full migration of what has already been released. Not worth it.

@Be-ing
Copy link
Contributor

Be-ing commented Jul 5, 2021

If you are willing to continue rebasing unmerged code for some indefinite time, sure, that's up to you. I would not do so myself.

facetsWithMandatoryTagMappingConfig[] = {
{CustomTags::kFacetGenre, kDefaultGenreTagMappingConfig},
{CustomTags::kFacetMood, kDefaultMoodTagMappingConfig}};
for (const auto [facet, defaultConfig] : facetsWithMandatoryTagMappingConfig) {
Copy link
Member

Choose a reason for hiding this comment

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

loop variable '[facet, defaultConfig]' of type 'const std::__1::pair<const mixxx::TagFacet &, const mixxx::TagMappingConfig &>' creates a copy from type 'const std::__1::pair<const mixxx::TagFacet &, const mixxx::TagMappingConfig &>' [-Wrange-loop-analysis]

@@ -454,7 +458,7 @@ void Track::emitChangedSignalsForAllMetadata() {
emit titleChanged(getTitle());
emit albumChanged(getAlbum());
emit albumArtistChanged(getAlbumArtist());
emit genreChanged(getGenre());
emit genreTextChanged(getGenreText());
Copy link
Member

Choose a reason for hiding this comment

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

Could we split off the genre text renamings and already merge them or is this too much work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could probably reduce the change set. I will check that.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1019601650

  • 615 of 2297 (26.77%) changed or added relevant lines in 36 files are covered.
  • 25 unchanged lines in 11 files lost coverage.
  • Overall coverage decreased (-0.1%) to 28.543%

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/dlgtagfetcher.cpp 0 1 0.0%
src/library/export/engineprimeexportjob.cpp 0 1 0.0%
src/library/searchquery.cpp 0 1 0.0%
src/sources/soundsourceproxy.cpp 2 3 66.67%
src/widget/wsearchrelatedtracksmenu.cpp 0 1 0.0%
src/library/basetrackcache.cpp 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
src/engine/enginevumeter.cpp 1 90.24%
src/library/proxytrackmodel.cpp 1 0%
src/library/scanner/libraryscanner.cpp 1 23.55%
src/track/taglib/trackmetadata_common.cpp 1 29.86%
src/track/taglib/trackmetadata_id3v2.cpp 1 62.14%
src/library/dao/trackdao.cpp 2 46.65%
src/library/dlgtagfetcher.cpp 2 0%
src/library/trackcollectionmanager.cpp 2 12.42%
src/widget/wtrackmenu.cpp 2 0%
src/library/dlgtrackinfo.cpp 3 0%
Totals Coverage Status
Change from base Build 1019395588: -0.1%
Covered Lines: 20662
Relevant Lines: 72389

💛 - Coveralls

@uklotzde uklotzde changed the title Custom Tags [Don't merge] Custom Tags Aug 7, 2021
@uklotzde
Copy link
Contributor Author

uklotzde commented Aug 7, 2021

After #4101 has been merged I will split this PR into smaller PRs. The main data structure (including serialization) and the mapping to file tags can safely be merged at any time.

Database and UI changes will be deferred.

@uklotzde uklotzde changed the title [Don't merge] Custom Tags [Don't merge / Obsolete] Custom Tags Aug 19, 2021
@uklotzde uklotzde marked this pull request as draft August 19, 2021 09:39
@uklotzde uklotzde closed this Sep 27, 2021
@uklotzde uklotzde deleted the custom_tags branch September 28, 2021 11:34
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.

7 participants