FIX #2645 Better Faux Pickup Support

[arth-1]

Changes Made-

Added logic to handle non-negative pickup values and support "faux pickups" like 1/3. Ensured notationPickup is called after setting the pickup value.

Validated the pickup value before passing it to MeterActions.setPickup to prevent errors.

Added input validation for pickup values in flow, ensuring only valid values are processed.

[walterbender]

Maybe print factor to console so we can diagnose why this is happening? Or maybe fix rationalToFraction?

[arth-1]

Hello, even though i print factor in console, there seems to be some issue in the initial function as while doing so it throws errors, also i did find some potential problems in error handling in rationalToFraction function which are as follows-

1. Infinite loop risks are there in the while loop in edge cases
2. there are floating point inaccuracies which might cause very large denominators due to very small values of d
3. also there could be more improvements in error handling of edge cases such as limiting the maximum number of cases or the number possible

but if i do these changes in rationalToFraction function in js\utils\utils.js file can it cause issues in the software at other places as it is used in a lot of places, please guide me for the same @walterbender

[walterbender]

I think all of the changes you recommend for rationalToFraction make sense regardless of where it is used. Best to address the problem at its source.

[arth-1]

I have updated the rationalToFraction function kindly review it and please let me know if this is good or require some additional changes as after testing this it seems to work for me with no error in edge cases in the software.

[walterbender]

looks good.
how have you tested it?

[arth-1]

At first to test the rationalToFraction function i first just tested it by doing console.log for edge cases and it seemed giving correct outputs so the logic seems correct to me and i also ran the musicblocks software locally and tried testing the changes there and...there i didnt seem to find any unexpected thing regarding this hence, it looked fine to me.

[walterbender]

I am seeing:
Uncaught ReferenceError: iterations is not defined at rationalToFraction (utils.js:1163:45) at Notation.notationPickup (notation.js:357:19) at Singer.MeterActions.setPickup (MeterActions.js:87:36) at PickupBlock.flow (MeterBlocks.js:1327:33) at Logo.runFromBlockNow (logo.js:1453:54) at logo.js:1300:32

[walterbender]

One last thing. Since we can do a pick up of 1/7 now with Lilypond, the warning message should be removed.

[walterbender]

[arth-1]

i am really sorry for the previous mistake of forgetting to commit i did remove the warning message, but based on the picture you sent i am unsure that if it is correct or not as i am not having in depth knowledge of sheet music. But let me know.

[walterbender]

I attached the picture to demonstrate that everything is working well. I'll try to capture a picture of the warning message.

[walterbender]

I think we can eliminate this entire block in notation.js

        if (this.activity.logo.runningLilypond) {
            obj = rationalToFraction(factor);
            if (!obj) return; // Prevent undefined obj error
            this.activity.errorMsg(
                _("Lilypond cannot process pickup of ") + obj[0] + "/" + obj[1]
            );
        }

[arth-1]

Ok thankyou for clarification, I am now getting to know better how this works. Yes it indeed seems unnecesarry for this block to be present there, I have removed it and the software seems to work fine.