Skip to content

Conversation

@Commanderk3
Copy link
Member

Issue #4128

With this PR, the music keyboard will be able to add consistent white and black keys throughout its addition until the end of pitch. Previously, after a certain number of addition of Pitch notes the black notes started to break there sequence.

@walterbender Sir, kindly check this PR and give a feedback if this is relevant or not. Thank you.

@walterbender
Copy link
Member

It took me a minute to find the changes related to the problem you set out to fix as there are multiple formatting changes mixed in. Please put the formatting changes should be in a separate commit in the future.

How does this change work? Why is only necessary to add a second octave worth of black note positions? Why not more? Should we calculate this array instead of hard-code it?

@Commanderk3
Copy link
Member Author

Commanderk3 commented Dec 14, 2024

Yes sir, I will keep this in mind and won't do it next time. I will disable my formatting extension.

In case of octaves, on inspecting the code what I have understood is that there is a limit up to which the keys can be generated. I believe the last note name is C9.

The createKeyboard function accounts a seperate array for black notes. This particular array (where I have made the changes) holds the excluded black notes). That's how it is able to know which black note to render.

Of course I also thought about making a function which will make a list of excluded black notes but that would be unnecessary I suppose.

@walterbender
Copy link
Member

Just keep the formatting in a separate commit.

Meanwhile, I'll try to better understand what is going on here.

@walterbender
Copy link
Member

Screenshot From 2024-12-14 11-22-19

Your analysis holds true if you launch the widget from C4. But if you start at C1, eventually you run off the end of your array and end up with the same problem.

@walterbender
Copy link
Member

Screenshot From 2024-12-14 11-24-04

@Commanderk3
Copy link
Member Author

Okay I unerstood now. But it is still limited so I suppose we can just estend the 'excluded black notes' array. What do you think sir ? If you agree then I will make the necessary changes.

@walterbender
Copy link
Member

Extending the array is fine.

@Commanderk3
Copy link
Member Author

@walterbender Sir the fix is here. Also their is no formating.

image

@walterbender
Copy link
Member

Please make a separate PR with the formatting update.

@walterbender walterbender merged commit 02412af into sugarlabs:master Dec 14, 2024
4 checks passed
@Commanderk3
Copy link
Member Author

Please make a separate PR with the formatting update.

Is there any standard formatting style for music blocks, that I can follow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants