Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: #4128 White and Black key generation is consistent #4129

Merged
merged 3 commits into from
Dec 14, 2024

Conversation

Commanderk3
Copy link
Contributor

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
Contributor 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
Contributor 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
Contributor 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
Contributor 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