widgets/musickeyboard ES6 port, enhancements etc.

[ricknjacky]

Issue references:- #2767, #2630, #2609 #2629

In this PR, I have:-

• Ported widgets/musickeyboard.js file to ES6 class syntax.
• Made minor enhancements to UI of the keyboard
• Took care of linting and prettifying the file.

[ricknjacky]

@walterbender please review.

ps:- this is in regard to comment,
I had initially misinterpreted the behavior of keyboard. I presumed that all keys were supposed to have text irrespective of whether or not they were present in clamps blocks.
As, that's not the case bug described in #2796 is not technically a bug, it was a minor misinterpretation I did.

Minor UI enhancement is what I thought was relevant here and that's what I have done.

[ksraj123]

Please go through my comments. Thanks

[ksraj123]

would be better if we could remove the init function and do things in the constructor instead, please make the necessary changes where the class is instantiated as well. We could make another function to setup the UI elements and event listeners and call it from the constructor for making our code more organised. Thanks

[ricknjacky]

It's imperative to reach a consensus on this.

• phrasemaker
• pitchdrummatrix
• pitchslider
• pitchstaircase
• rhythmruler
• status
• temperament
• tempo
• timbre

all of these widgets have init function. If we're deciding to eradicate init function all together, it be done with all widgets and not just arbitrarily.

[ricknjacky]

@ksraj123 thanks for the review, I have committed your suggestions.

@meganindya Please read my comment on @ksraj123's suggestion for eradication of init() , pls update on the same, Thanks.

[meganindya]

Initially I thought putting all the init stuff in the constructor made a lot of sense because that's what a constructor is supposed to do: initialize. But, I noticed that the widget classes (or prototype functions) aren't consistent with their initialization. The object is created when the clamp block of the widget is visited, and the DOM is initialized when the hidden block at the end of the block is executed (i.e. when the clamp block is exited). The DOM initialization sometimes depends on the contents of the clamp block, other times it doesn't. Since there is inconsistency it needs refactoring to standardize the equation all across.

But, all that doesn't help the application on the user-end; it is merely a cosmetic operation on the codebase. And, since this application is going to be replaced soon, I don't see much value in doing that. So, I think it's better to keep it as is. Even the code refactoring isn't helping a lot right now, unless it makes the application more stable for archiving. Don't bother much with these.

[meganindya]

Something like #2632 is a higher priority requirement as it significantly helps with the stability of the application, which currently is very unstable and unpredictable when hosted remotely.

[ricknjacky]

Thanks for the insight @meganindya

I presume this PR is ready to be merged then, please review @walterbender

[walterbender]

I think that the behavior of the + button has changed. Could you please test?

[ricknjacky]

I think that the behavior of the + button has changed. Could you please test?

Sure

Behavior on my local (concurrent with this PR's branch MusicKeyB):

MB_local.mp4

• Hertz note of frequency 261 added.
• Note Re 5 added

Behavior on Master branch (in sync with MB's master):

MB_master.mp4

• Hertz note of frequency 261 added.
• Note Sol 5 added

They're identical, just the notes that were added with presets are different.
If I have missed something however, please do share the feedback.

[walterbender]

As you can see in your video, the pitch block created was not properly attached to the music keyboard clamp --- it stayed in the upper left behind the palette. There seems to be a bug with the connections when adding the new block.

[ricknjacky]

As you can see in your video, the pitch block created was not properly attached to the music keyboard clamp --- it stayed in the upper left behind the palette. There seems to be a bug with the connections when adding the new block.

Right, this is a regression

Also, this behavior is prevalent on both master branch and this PR.
Somehow, we've missed this all along.

[ricknjacky]

As you can see in your video, the pitch block created was not properly attached to the music keyboard clamp --- it stayed in the upper left behind the palette. There seems to be a bug with the connections when adding the new block.

Any addition be it hertz or pitch seems to have this bug in this widget,
Also, if we

• (i)add hertz first--then it is not added to the clamp.

But if I then [after(i)],

• (ii)add any other note or pitch then the hertz added in step (i) is now in the clamp connected as it was supposed to be initially.

I'm trying to find the cause of this bug.
Any suggestions as to where it can be or what needs to be done that can help me alleviate this bug?

[walterbender]

I've not looked, but the place to look is in the code that chains together the blocks after the new block is created, beginning at L 1397 in the old code. (The problem may be in _addNotesBlockBetween() as it seems the insertion is broken.)

[ricknjacky]

@walterbender please take a look at #2839
also, I investigated the error described above..error isn't in this file it is in blocks.js file i presume.

If we add a note, then drag the new added note around..you can see the notes that were present in clamp join the new added note to form a seperate new group. connections property seems ok to me in this file. Please correct me if I have missed something though.
Please do share your thoughts on this.

[ksraj123]

Hey @ricknjacky, I find this issue of improper alignment of blocks inside the clamp when new ones are added interesting. I would like to give this a try if you are facing some difficulties with it. Thanks
cc - @walterbender

[ricknjacky]

Hey @ricknjacky, I find this issue of improper alignment of blocks inside the clamp when new ones are added interesting. I would like to give this a try if you are facing some difficulties with it. Thanks
cc - @walterbender

Definitely, it'll be a great opportunity for me to work with you and learn a lot. Please tell me if I can help with anything..

also..there are a lot of other bugs I've tagged an issue above.. let's solve all these once and for all. 👍

[ksraj123]

Hey @ricknjacky, I find this issue of improper alignment of blocks inside the clamp when new ones are added interesting. I would like to give this a try if you are facing some difficulties with it. Thanks
cc - @walterbender

Definitely, it'll be a great opportunity for me to work with you and learn a lot. Please tell me if I can help with anything..

also..there are a lot of other bugs I've tagged an issue above.. let's solve all these once and for all. 👍

Sounds great. I have opened a PR #2844 fixing this issue.
@walterbender Please have a look. Thanks