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 regressions in MusicKeyboard widget #2863

Merged
merged 2 commits into from
Mar 12, 2021

Conversation

daksh4469
Copy link
Member

@daksh4469 daksh4469 commented Mar 1, 2021

After #2849, the Music Keyboard widget has two regressions:

  • The lower 8 notes from G4 to C4) in the column of the grid were unclickable.
    This is happening as the for loop (starting from line 1057 in the makeClickable function), the parameter this.layout.length is getting set to 5, after Multiple Bug Fixes and Enhancements to Music Keyboard #2849, instead of 12. Now, the error arises in the _keysLayout function where the notes are pushed in the layout array. So, sortedList is replaced by newList . Here,
    newList = fillChromaticGaps(sortedNotesList)
    Thus, newList contains all the 12 notes whereas sortedList contains only the first 5 notes.

  • In case when multiple notes were selected in a column, the music would stop playing and not move any further.
    This regression arises as when multiple notes are selected, the playOne could not fetch the objId of the notes from selectedNotes . Now, this happens when the setNoteCell method is called in the setNotes function. In this setNoteCell method, when the note is being pushed in the selectedNotes array, objId should be assigned to that in the displayLayout (introduced in Multiple Bug Fixes and Enhancements to Music Keyboard #2849) array and not of the layout array.

Below is the video showing both these regressions:

mk.3.mp4

This patch fixes these regressions and the widget functions as expected.

mk.1.mp4

Please have a look at this @walterbender @meganindya and let me know if it has any issues.
Thanks.

@walterbender
Copy link
Member

Is #2857 ready to review too? Are these compatible?

@walterbender walterbender merged commit a99c19d into sugarlabs:master Mar 12, 2021
@daksh4469
Copy link
Member Author

Is #2857 ready to review too? Are these compatible?

Yes, can you please check if #2857 is now working? @walterbender
It is working on my environment (Chrome and Firefox).

@daksh4469
Copy link
Member Author

About compatibility, I might have to do a rebase once you review and approve those changes.

@daksh4469 daksh4469 deleted the daksh4469-main5 branch March 13, 2021 08:43
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