Problem with Classical Extras plugin/script with recent Picard versions

hiccup · February 11, 2024, 6:40pm

I have been noticing a problem with recent versions of Picard, regarding a script that I am using with the Classical Extras plugin.

This scripts writes a tag that is constructed of the different levels that a classical piece may be a part of.
The problem is that with the latest version(s) of Picard, that tag is not being written anymore for some subsequent tracks of a release, or even the whole release.

So if the tracks of that release all have that tag written by an earlier version of Picard, and you then run the same release through v2.11, those tags will not get updated, or will be deleted. (if you have set ‘clear existing tags’)

This gif should clearly show the problem.
Look for the fouth tag named ‘picardtitlefull’.
It was written using an earlier version of Picard, and a recent version is trying to delete it:

I have tested this with several previous versions of Picard to find out when this degradation may have been introduced, and it clearly showed that Picard 2.8.5 is the last one where it worked fine
All releases after that, starting from 2.9 have this issue.

Let me know what more information is required in case this is something worth investigating?

outsidecontext · February 11, 2024, 8:20pm

I don’t know the classical extras plugin well. From looking over the changes in Picard 2.9 I couldn’t spot something suspicious there.

What exactly is the script that sets the picardtitlefull tag?

hiccup · February 11, 2024, 8:54pm

Since there may be more things involved than just the script itself (e.g. how I have configured Classical Extras), I have send you a link for a download that contains two Picard portable installs (old and new), and two releases that should show exactly what you see in the gif.

No hurries, 2.8.5 is working great for my classical library, and I wouldn’t mind sticking with it.

outsidecontext · February 12, 2024, 1:14pm

Found the issue: The reason is a change to how the comparison functions in Picard 2.9 and later operate. In prior versions the functions only worked on integer values, so something like $gt(2,1) worked. But things like $gt(2022-10-13,2022-12-24), $gt(a,b) or even $gt(5.3,6.7) had no defined outcome.

With Picard 2.9 the functions can also be used for string or float number comparison. Most of the time the automatic detection of the comparison to performs works well and scripts function as before. But in your case many variables are complete unset and seem to cause the comparison to behave differently then before. The trick is to explicitly set the type of comparison, e.g. instead of $lte($get(_cwp_title_part_levels),1) use $lte($get(_cwp_title_part_levels),1,int). See also $lte — MusicBrainz Picard v2.11 documentation

The below updated script adds the types to the $lte and $gte calls and causes the script to work again for me:

$set(_work_top,$if($gte($get(_cwp_work_part_levels),1,int),$get(_cwp_work_top)))

$if($gte($get(_cwp_work_part_levels),3,int),$set(_temp,$find(%_cwp_work_1%,:))$set(_work_sub,$substr(%_cwp_work_1%,,$if($lt(%_temp%,0,int),,%_temp%)))))

$if($eq($get(_cwp_work_part_levels),2),$set(_part,$get(_cwp_inter_work)))
$if($eq($get(_cwp_work_part_levels),3),$set(_part,$get(_cwp_part_1)))
$if($eq($get(_cwp_work_part_levels),4),$if($eq($get(_cwp_title_work_levels),4),$set(_part,$trim($substr(%_cwp_work_2%,$find(%_cwp_work_2%,:),),: )),$set(_part,%_cwp_inter_work%)),)

$if($get(_cwp_work),$if($get(_cwp_title_part_levels),,$set(_title_piece,$get(_cwp_extended_part))),$set(_title_piece,$get(_recordingtitle)))
$if($lte($get(_cwp_title_part_levels),1,int),$set(_title_piece,$get(_cwp_extended_part)))
$if($eq($get(_cwp_title_part_levels),2),$set(_title_piece,$get(ce_movement_inc_nr)))
$if($eq($get(_cwp_title_part_levels),3),$set(_title_piece,$get(_cwp_title_part_0)))

$set(picardtitlefull,$if($get(_work_top),₁$get(_work_top)¹,)$if($get(_work_sub),₂$get(_work_sub)²,)$if($get(_part),₃$get(_part)³,)$if($get(_title_piece),₄$get(_title_piece)⁴,))

outsidecontext · February 12, 2024, 1:26pm

To illustrate the problem a bit, take this small script:

$set(a_lte,$if($lte(,1),true,false))
$set(a_gte,$if($gte(,1),true,false))

It sets the two variables a_lte and a_gte by comparing an empty value with 1 with both $gte (greater than or equal) and $lte (lower than or equal).

The maybe surprising result in Picard 2.8.5 and lower is that both variables are set to “false”, that is the empty value is neither smaller nor larger (nor equal) than 1. That’s because it can only compare integers, an empty value is not a valid integer, the comparison fails and as with all scripting functions which fail it returns an empty value (= false) in case of error.

Since Picard 2.9 a_lte will be set to true and a_gte will be set to false. That’s because the function now tries to find a comparison that works. The first empty value is not a number, so number comparison fails. But everything in Picard’s scripting is a string, so it does string comparison. And the empty string sorts lower then the “1”.

By calling $lte(,1,int) instead the function gets forced to use integer comparison and the result will be the same as in older versions.

Sophist · February 12, 2024, 4:13pm

I would classify this as potentially a bug. The outcome of the comparison functions should be determinate in all circumstances and the outcomes for e.g. null parameters should make logical sense.

outsidecontext · February 12, 2024, 4:31pm

It is deterministic, and I think it makes logical sense. Both $lt(,1) being true and $gt(,1) being false seems fine.

hiccup · February 12, 2024, 4:39pm

Thank you for figuring this out so quickly!
That indeed solved the problem. Great!

I am only wondering if this change may affect other (older) scripts too?
And the users not being aware (yet) of results being different than expected?

Perhaps some mitigation could be implemented so that older scripts such as mine will output the same results as with pre-2.9 versions?
But if the current behaviour is considered correct, and the previous one to have been incorrect, I can understand that not being an option.

Sophist · February 12, 2024, 4:45pm

I would agree. Although a break in backwards compatibility is always regrettable, sometimes it is the price of moving forwards. (Not that it was an official bug request, but I withdraw my previous statement about this being a bug.)