-
-
Notifications
You must be signed in to change notification settings - Fork 227
Support for Artist Sort Tags #1779
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
base: master
Are you sure you want to change the base?
Conversation
The previous "skip articles" checkbox setting moves to one of the combobox items so that the current behavior is still supported.
A full rescan is need to utilize the new fields artistsort and albumartistsort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks great.
Could you also add "TITLESORT" and "ALBUMSORT" at the same time?
And optionally "COMPOSERSORT" and "PERFORMERSORT".
And if you could do me the favor of adding "bpm" (REAL), "mood" (TEXT) and "initial_key" (TEXT) to the same schema update. I can do the work and add support for those tags. But that way we don't have to do multiple schema updates before the next version. Once this get's merged some users will already be using it.
|
||
DROP INDEX IF EXISTS idx_albumartistsort; | ||
|
||
ALTER TABLE songs ADD COLUMN artistsort TEXT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use %allsongstables
to reduce the number of entries, and that will also update the device song tables.
See
strawberry/src/core/database.cpp
Line 415 in a47531d
if (command.contains(QLatin1String(kMagicAllSongsTables))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You also need to add the columns to device-schema.sql https://github.com/strawberrymusicplayer/strawberry/blob/master/data/schema/device-schema.sql identical to songs, otherwise devices will fail.
@@ -50,6 +50,13 @@ constexpr char kOverwriteRating[] = "overwrite_rating"; | |||
constexpr char kDeleteFiles[] = "delete_files"; | |||
constexpr char kLastPath[] = "last_path"; | |||
|
|||
enum class SortShowArtists { | |||
AsIs = 1, | |||
SortSkipArticleShowArtist = 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SortSkipArticleShowArtist = 2, | |
SkipArticles = 2, |
@@ -50,6 +50,13 @@ constexpr char kOverwriteRating[] = "overwrite_rating"; | |||
constexpr char kDeleteFiles[] = "delete_files"; | |||
constexpr char kLastPath[] = "last_path"; | |||
|
|||
enum class SortShowArtists { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest different names so we can use the same enum for all sort tags:
SortBehaviour
enum class SortShowArtists { | ||
AsIs = 1, | ||
SortSkipArticleShowArtist = 2, | ||
SortByTagShowArtist = 3, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name was a bit confusing to me, also don't limit the enum to artist.
SortByTagShowArtist = 3, | |
UseSortTagForSort = 3, |
AsIs = 1, | ||
SortSkipArticleShowArtist = 2, | ||
SortByTagShowArtist = 3, | ||
SortByTagShowTag = 4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SortByTagShowTag = 4 | |
UseSortTagForDisplayAndSort = 4 |
Thanks for reviewing. I agree with all your comments. Handling the other sort tags as well certainly makes sense (I just hope that the ui doesn't get too messy). What's the best way forward now? Since the scope changes quite a bit I propose the following:
That way we could get the db changes early into master and you can work on bpm, mood, initial_key without depending on me (I won't be able to work on the sort tags next week). What do you think? |
Upgrade from schema version 20 to 21. This includes: - six fields for sort tags - new fields bpm, mood, initial_key See strawberrymusicplayer#1779 (review)
You don't need to create a new PR, but you can add the schema update first if you want. |
Upgrade from schema version 20 to 21. This includes: - six fields for sort tags - new fields bpm, mood, initial_key See strawberrymusicplayer#1779 (review)
Upgrade from schema version 20 to 21. This includes: - six fields for sort tags - new fields bpm, mood, initial_key See #1779 (review)
Overview
This PR adds support for sorting a collection by sort names often included as special tags in song files. This allows, for example, to see "The Beatles" sorted as "Beatles, The".
Strawberry already has some support for filtering articles from names before sorting, but this has limitations like no support for languages other than English. A more reliable way is to use provided data since the rules for correct sort names can be quite complex. If you tag your files with MusicBrainz, for example, then sort information is stored in tags "artist sort" and "album artist sort".
Technically, the PR implements support for vorbis tags ARTISTSORT and ALBUMARTISTSORT, and id3v2 tags TSOP and TSO2 (see e.g. https://docs.mp3tag.de/mapping-table/ or https://picard-docs.musicbrainz.org/downloads/MusicBrainz_Picard_Tag_Map.html ).
While motivated mainly by my own needs, this PR also addresses requests in the following forum posts:
User Visible Changes
Collection
The Settings|Collection page now has a combo box for choosing the sort behavior. Previous versions of strawberry had a checkbox "Skip leading articles ("the", "a", "an") when sorting artist names". This has been removed, but the underlying behavior is still available as one of the combobox options.
There are four settings available:
Option 1 (Sort and show artist name as is) uses the artist name as is. It is equivalent to the unchecked state of the "Skip leading articles..." checkbox in previous versions of strawberry. Here is an example of the result:
Option 2 (Skip articles "The, A, An" and show "The Beatles") is the default and the same as the checkbox "Skip leading articles..." in earlier versions. Note that this option does not correctly handle non-English names as can be seen by the entry for "Die Ärzte".
Options 1 and 2 provide full backwards compatibility. They can be used, for example, if your collection does not have sort tags, you do not want to use them, or you are just happy with the current behavior.
Option 3 and 4 provide the new functionality by using sort tags. Option 3 (Use sort tag "Beatles, The" and show "The Beatles") uses the sort tag for sorting internally, but shows the artist name.
Option 4 (Use sort tag "Beatles, The" and show "Beatles, The") uses the sort tag internally and also for display.
Playlists
Two new columns Artist Sort and Album Artist Sort can be enabled in the context menu of playlists:
Smart Playlists
Smart playlists can use search terms with Artist Sort and Album Artist Sort fields.
Both new fields can then be used for sorting the results as well.
Edit Track Information
The Edit Track Information dialog allows to edit the two new fields for supported file formats (e.g. flac and mp3). Changes are written back to the song files and the collection database is updated as well.
Context View
The custom text settings for the context view can now make use of the new fields. The popup menu in the Settings|Context page has additional entries for that:
Here is an example setting using the artistsort field.
Migrating Existing Collections
When the new version of strawberry detects an older collection database on startup, it offers to rescan the collection.
This should be confirmed in order to read information for the new tags from song files.
Implementation Details
The steps necessary to implement this were:
Limitations
Tests
I have run the following manual tests with a flac and mp3 collection of about 1,000 albums / 16,000 songs:
Sorry, that this PR got a bit long. Initially, when I started to work on this, I thought it might be a rather small change. Then I learned how many places in strawberry actually are affected, although, I think most changes were rather straightforward. Let me know if there is anything else to do or if you find any issues with my changes. I will try to fix them.