String breaking change

So with this commit the double to String behaviour has changed :

imo this is a breaking change. And the proof is that you had to remove a test from the UnitTest when you did it! :slight_smile:

hopefully not a lot of people have some critical functionalities expecting that things like String (10.003, 2) gives "10.00" (because now it is "10.0"), but it worths warning people about the change.

And that change is really annoying from a UI perspective, because if you have some parameters/sliders strings built up with String (value, 2) the string length is now changing according to the value, and thus the number is ā€˜bumpingā€™ in the UI while you drag the slider. :woozy_face:

Any chance to get the old behaviour back by default?
perhaps you could add a bool parameter ā€˜relaxNumberOfDecimalPlacesConstraintā€™ or something, but I would love to to get the old behaviour as the default one.

8 Likes

I agree, Iā€™ve used the decimal places String constructor a lot in my look and feel methods. It doesnā€™t exactly cause errors but does cause some visual bugs!

1 Like

Yep. weā€™ve also used it for formatting through faders/meters.

wouldā€™ve been worth adding additional default bool for removeTrailingZeros / relaxNumberOfDecimalPlacesConstraint (as @lalala suggestedā€¦)

2 Likes

canā€™t you just check out a previous commit?

But then I miss out on everything added since that commitā€¦ or do I not understand git? :thinking:

2 Likes

I guess Iā€™ll have to temporarily remove the constructor to catch all of its usages to replace them with a new function which emulates the old behaviourā€¦ :confused:

There is a way around:

  • checkout the commit after the breaking change
  • call git revert HEAD~1 to undo the last commit
  • Instead of commiting, call git diff > undoString.patch
  • Go back to the head using git checkout HEAD
  • apply the patch git apply undoString.patch

Now either you do this each time after checking out, or you create you own branch, where you always pull new commits from develop. It will often create merge commits, donā€™t worry about them, if you look into them using git diff, most of the time they are just empty.

Jules, what do you think about that?
Could you confirm if this change is going to stay or not? are you considering a revert?

Yeah, apologies for this one being a bit of a messy change. The problem was that it was never intended to work the way it did, but a test had somehow been added that checked for the wrong result, which made it all look legit. The function as it stood was full of edge-cases where its behaviour would be ambiguous or problematic - it really wasnā€™t very good, and IIRC the reason I changed it was because it caused bizarre problems in something else I was working on.

We discussed the fact that it would be a breaking change but figured that surely nobody would actually be relying on the not-quite-as-expected behaviourā€¦ so we decided to change itā€¦ and clearly itā€™s a mistake to make assumptions about how people might be using your code!

I donā€™t want to revert it though. We donā€™t want the old behaviour back, and we donā€™t want to get into a feature-creep situation by tagging on extra parameters to this method for more formatting options.

What we all really want is a proper, comprehensive system for formatting numeric strings with lots of nice, clear options, and thatā€™s not something that belongs in the String class as a method, it needs to be done in a dedicated class devoted to number->string conversion that can cover lots of use-cases.

Iā€™d really like to see that added (have wanted to do it for years), but itā€™d take a bit of planning and time to implement, so in the meantime, perhaps just add a free function to your own project like this:

static String doubleToStringWithMinDecimalPlaces (double value, int minDecimalPlacesAfterPoint)
{
    String s (value, minDecimalPlacesAfterPoint);
    auto dot = s.indexOfChar ('.');
    auto len = s.length();
    auto paddingNeeded = minDecimalPlacesAfterPoint;

    if (dot < 0)
        s << '.';
    else
        paddingNeeded -= (len - dot - 1);

    return paddingNeeded > 0 ? s + String::repeatedString ("0", paddingNeeded) : s;
}

Having your own free function also means you have more control over what happens in edge-cases, e.g. if the number is so large that scientific notation is needed, or what happens if itā€™s an NaN, or

1 Like

!!!
nothing wrong with the way people used it imho. The doc was stating "if this is > 0, it will format the number using that many decimal places". Iā€™m not native english, but I think we were all expecting it to do what it was supposed to do.

2 Likes

Yes, not criticising anyone for misunderstanding - the comment was as bad as the implementation!

Yes, the function was doing what the doc was saying but both were wrong. cool :slight_smile:

I will stop using the tip for now and stick with 5.3.2

Iā€™m sure itā€™s going to be a really annoying change (mainly for UI reason as stated above) for many people (note that the thread was liked 7 times in a couple of hours), so if you wonā€™t change your mind, donā€™t forget to add a paragraph in the ā€œbreakingChangesā€ txt file.

3 Likes

I would have expected Slider:: setNumDecimalPlacesToDisplay() to always display the requested number of decimal placesā€¦ although the comment does use the wording: ā€œModifies the best number of decimal places to use when displaying this sliderā€™s value.ā€

Rail

1 Like

I would have guessed almost everyone expects String(value, 2) to be equivalent to snprintf(buffer, bufferSize, "%.2f", value).

For an XML or JSON dump, you donā€™t use a fixed number of decimal places anyway, but a fixed number of significant digits: eg. you write 12345678 with 0 decimal places, 123.45678 with 5 and 0.012345678 with 9.