PICARD-2032: Set albumsort/titlesort tags #369
Conversation
plugins/enhanced_titles/__init__.py
Outdated
| self._swapprefix(metadata, "album") | ||
|
|
||
|
|
||
| def _create_prefixes_list(languages = None, is_title = False): |
There was a problem hiding this comment.
IMO, this is coded incorrectly. This function is called each and every time a script function is called and simply repeats the exact same code each time.
IMO the set of words for all languages should be calculated once, at load time rather than each time a script function is called. Personally I would store it in the same dict as the separate languages with an empty string as the key.
|
P.S. @twodoorcoupe Giorgio - Many thanks for coding this when I don't have the time. 👍 |
|
Thank you @Sophist-UK for taking the time to review this. I made the changes you requested and added some unit tests. Now, when a new combination of languages is used, the resulting list of prefixes or minor words is stored so as not to recalculate it every time. |
| else: | ||
| self._swapprefix(metadata, field) | ||
|
|
||
| def _swapprefix(self, metadata, field): |
There was a problem hiding this comment.
This would IMO be better of as a function taking the field value and returning a sorted value, leaving the caller to get the unsorted value and store the sorted value i.e.
def _swapprefix(self, field):
....
return func_swapprefix(None, metadata[field], *prefixes)
metadata["titlesort"] = self._swapprefix(metadata["title"])Also, using return enables early return improved readability avoiding multiple nested if/else i.e.
if something:
...
return first_something
if something_else:
return second_something
return third something
plugins/enhanced_titles/__init__.py
Outdated
| def _format_languages(self, languages): | ||
| languages = [lang.lower()[:3] for lang in languages] | ||
| languages = [lang for lang in languages if lang in _articles.keys()] | ||
| return tuple(languages) |
There was a problem hiding this comment.
I am unclear what benefit there is to converting languages into a tuple rather than returning it as a list.
There was a problem hiding this comment.
This is so the tuple can be used as a key for the cache dictionary, since it's immutable.
plugins/enhanced_titles/__init__.py
Outdated
| 'the' | ||
| """ | ||
| prefixes = self.find_prefixes(languages) | ||
| return func_delprefix(parser, text, *prefixes) if text else "" |
plugins/enhanced_titles/__init__.py
Outdated
| if text.upper() == text and config.setting[KEEP_ALLCAPS]: | ||
| return text | ||
| minor_words = self.find_minor_words(languages) | ||
| return self._title_case(text, minor_words) if text else "" |
plugins/enhanced_titles/__init__.py
Outdated
| } | ||
| _articles[""] = [value for values in _articles.values() for value in values] | ||
|
|
||
| # Prepositions and conjunctions with 3 letters or less. |
There was a problem hiding this comment.
Might also be helpful say that these are words which aren't title-cased.
plugins/enhanced_titles/__init__.py
Outdated
| (str): The sort name of the first useful alias it finds. None if | ||
| none are found. | ||
|
|
||
| For example, "The Beatles" has alias "Le Double Blanc" with sort name |
There was a problem hiding this comment.
I got confused here as to why the 1960s' Hip Beat Combo "The Beatles" would be called "The Double White" in French, particularly considering that the group consisted of 4 white blokes not two. Then it clicked that you mean the nicknamed "White Album" which was officially entitled "The Beatles". To avoid anyone as stupid as me thinking the same, you might want to say 'For example, the album "The Beatles"...'.
|
@zas, can you enable workflow for @twodoorcoupe so that we can see his unit tests running? |
|
All the tweaks you suggested should be implemented now. Many thanks for all the tips. |
There was a problem hiding this comment.
Code and functionality look good to me, thanks a lot for this plugin.
I wonder though, whether the script functions should also try to use the language from the metadata instead of using essentially all if no language is explicitly given. The same as the swapprefix in SortTagger already does.
Or is there some specific reason not to do this?
|
Ah that makes sense and now I remembered it was actually proposed by @Sophist-UK in [PICARD-2779] as an enhancement of $swapprefix. I moved the logic for finding the languages from |
|
I think this looks good to go too. |
|
@twodoorcoupe Thank you again for all your efforts on this - including your willingness to rework it several times to satisfy the petty demands of grumpy old coders like me. |
This plugin attempts to fix [PICARD-2032]. For each track or album it first checks if there are any sort names already available in the aliases, otherwise it swaps the title's prefix.
Then it also partly fixes [PICARD-2779] by providing script functions:
So far, the languages available are English, Spanish, Italian, French, German, and Portuguese. For each language there's a list of articles that are used as prefixes and a list of short prepositions and conjunctions that are used in $title_lang. These I wrote myself by looking online, so they are prone to errors.
$swapprefix_lang and $delprefix_lang do the same thing as their original counterparts but take languages as inputs instead of prefixes. $title_lang returns the text in title case by keeping the small words of each language in lower case.
During tagging, if the title's prefix is swapped, only the track's or album's language are considered. If none are found then all of the available ones are considered.
Checking the aliases for each track and album can be turned off in the options, as well as ignoring titles in all caps for $title_lang.
It's my first time contributing so I apologize if I missed anything important.