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

Multiple Bug Fixes and Enhancements to Music Keyboard #2849

Merged

Conversation

ksraj123
Copy link
Member

@ksraj123 ksraj123 commented Feb 17, 2021

Updated

Bugs

Bug 1

When hertz blocks are added to the music keyboard widget then the hertz blocks get mixed up with notes. Ideally all heartz blocks should be present in the right most side arranged in increasing order of frequency. Relaunching the widget fixes this issue but that leads to bad user experience.

Screen.Recording.2021-02-17.at.10.43.33.PM.mov

Bug 2

When adding pitches, leftover pitches in the current octave are skipped and pitches from the next octave are included. Ideally it should move to pitches in the next octave after exhausting the current octave.

Screen.Recording.2021-02-18.at.5.59.05.AM.mov

In the video above we already had C, D, E, F and G in the 4th octave. If a next pitch is to be added then it should be A4 but we get G5 instead. We can see that everytime we close and reopen the widgetWindow and try to add a pitch then instead of the next pitch in the same octave, a pitch from the next octave is added.

Bug 3

There seems to be some issue with the fillChromaticGap function. When the noteList spans over more than 1 octave then C# is missing. This leads to some strange bugs when there are more than one octaves.

Bug 4

On adding pitches consecutively through the add menu of music keyboard, regression is encountered.

mbregression.mp4

Bug 5

On hitting corresponding key form the keyboard, non-existent accidental key appears.

Enhancements

Enhancement 1

When new pitches are added then the on screen keyboard and the matrix does not update. Ideally both the keyboard and table should update to include the newly added pitch.

Screen.Recording.2021-02-18.at.5.59.05.AM.mov

Enhancement 2

Key mapping between virtual and physical keyboard should be improved. QWERTY should be mapped to black keys and ASDF to white and numbers to hertz keys. Since the bindings are updated, corresponding on the physical keyboard are now displayed on the black keys as well.

@walterbender
Copy link
Member

Whoa. Why is the hertz getting mixed with the letter names? That is a regression.

@ksraj123
Copy link
Member Author

@walterbender could you please share some more details? Do you mean for the hertz block in the clamp? Is it supposed to stay at top?

@walterbender
Copy link
Member

Nothing to do with the clamp... On the keyboard itself, the D key was shifted over to the right to make room for the 262 hz. This is a serious regression. The keyboard layout is fixed. D has to always be next to C...

@ksraj123
Copy link
Member Author

@walterbender oh I see it now, apparently this was present from the start but somehow got unnoticed. Looking into it. Thanks

Screen.Recording.2021-02-17.at.10.43.33.PM.mov

@walterbender
Copy link
Member

Screenshot from 2021-02-17 12-14-48
^^ this is what we should be seeing.

@walterbender
Copy link
Member

I suspect that I only fixed it for when you launch, not when you add. But I suspect it is an easy fix.

@ksraj123
Copy link
Member Author

@walterbender Yes, that seems to be the case, works fine when relaunched but not updates properly. Apparently the issue was how the layout was being sorted. It seems to be fixed. But there's a similar regression when adding pitches too. Working on fixing that as well. Thanks

Screen.Recording.2021-02-17.at.11.14.15.PM.mov

@ricknjacky
Copy link
Member

@ksraj123 notice that you have introduced a minor regression, now new hertz keys added don't have keyboard keys designated to them after the first one is added.

@ksraj123
Copy link
Member Author

ksraj123 commented Feb 17, 2021

@ricknjacky I don't think this is a regression introduced by recent changes. Please see the WHITEKEYS and BLACKKEYS arrays. It contains the value for keys which are mapped onto the keyboard on the screen. Notice that there are only 9 values in the WHITEKEYS array? That is why we could get a corresponding key mapped upto the 9th white key only if the corresponding block is in the clamp. We could simply expand that array and more keys would be mapped. But the question is what keys should be mapped? There are only so many keys on the computer keyboard and usability has to be kept in mind as well.
Right now the white keys are madded to A S D F G H J K L which are letter in the middle row of a standard computer keyboard.
@walterbender what do you think about this, what keys should be choose to map after the 9th white key and 8th black key?

@ricknjacky
Copy link
Member

ricknjacky commented Feb 17, 2021

@ricknjacky I don't think this is a regression introduced by recent changes. Please see the WHITEKEYS and BLACKKEYS arrays. It contains the value for keys which are mapped onto the keyboard on the screen. Notice that there are only 9 values in the WHITEKEYS array? That is why we could get a corresponding key mapped upto the 9th white key only the corresponding pitch is in the clamp. We could simply expand that array and more keys would be mapped. But the question is what keys should be mapped? There are only so many keys on the computer keyboard and usability has to be kept in mind as well.

Yes you're right.. although i was talking about the behaviour that's there on master i.e. musicblocks website, over there each key added gets a letter assigned to it.

@walterbender what do you think about this, what keys should be choose to map after the 9th white key and 8th black key?

Looking forward to this.

@walterbender
Copy link
Member

The mapping of the computer keyboard to the virtual piano keyboard is never going to be perfect. But I suggest the following:
the primary goal is to map the notes in the clamp (the ones specified by the user as significant--the same ones we explicitly label) to the keyboard. There are three different collections of these: (1) letter-name notes that map to white keys; (2) letter-name notes that map to black keys; and (3) hertz, which are placed at the end of the virtual keyboard. I think it is adequate to just assign the asdfg row to the white keys, the qwerty row to the black keys and the number row to the hertz keys until we run out of available computer keys. We stop assigning once we run out.

An alternative approach could be to assume two octaves and use zxcvb and querty for white keys and asdfg and 12345 for black keys and ignore hertz. But either way, we'll eventually run out of keys. C'est la vie.

@ricknjacky
Copy link
Member

ricknjacky commented Feb 17, 2021

The mapping of the computer keyboard to the virtual piano keyboard is never going to be perfect. But I suggest the following:
the primary goal is to map the notes in the clamp (the ones specified by the user as significant--the same ones we explicitly label) to the keyboard. There are three different collections of these: (1) letter-name notes that map to white keys; (2) letter-name notes that map to black keys; and (3) hertz, which are placed at the end of the virtual keyboard. I think it is adequate to just assign the asdfg row to the white keys, the qwerty row to the black keys and the number row to the hertz keys until we run out of available computer keys. We stop assigning once we run out.

An alternative approach could be to assume two octaves and use zxcvb and querty for white keys and asdfg and 12345 for black keys and ignore hertz. But either way, we'll eventually run out of keys. C'est la vie.

Interesting, thanks for the insight.

I had a query though, if at a certain point of time we're gonna run out of keyboard shortcuts... wouldn't it be erratic user experience to have shortcuts for some and not for the rest. As we run out of them.. is addkeyboardshortcut method unavoidable?

Wouldn't we be better off without this method? speaking from my experience of working and using this widget... using mouse is fluent and intuitive. I presume thats what most users would prefer and use when using this widget. Also, most of add on functionalities like add button, modifying duration, change note type etc. are accessed using mouse.

Your thoughts on this?

@walterbender
Copy link
Member

We don't use short cuts for widget buttons right now, but we probably should. But I think that can live in parallel with the keyboard mapping described above. I think it is unusual for a user to have more than an octave or two but I do think being able to use the mapping is useful.

@ksraj123
Copy link
Member Author

ksraj123 commented Feb 18, 2021

More regressions

Please go through the short video below

Screen.Recording.2021-02-18.at.5.59.05.AM.mov

Here is a list -

  • In the video above we already had C, D, E, F and G in the 4th octave. If a next pitch is to be added then it should be A4 but we get G5 instead. We can see that everytime we close and reopen the widgetWindow and try to add a pitch then instead of the next pitch in the same octave, a pitch from the next octave is added.
  • sol5 is G5 in fixed solfege but when the widget window is relaunched the onscreen keyboard and the table does not update.
  • There seems to be some issue with the fillChromaticGap function. When the noteList spans over more than 1 octave then C# is missing. This leads to some strange bugs when there are more than one octaves.
  • Updates to key mapping, will follow qwerty for black, asdf for white and 1234 for hertz keys.
  • On hitting corresponding key form the keyboard, non-existent accidental key appears.
    non existent accidentals

All three of these issues are fixed in local, will push the commits soon. Will keep updating here as I find more regressions. Thanks

@ksraj123 ksraj123 changed the title fix bug on adding pitches consecutively in music keyboard Multiple Bug Fixes and Enhancements to Music Keyboard Feb 18, 2021
@ksraj123 ksraj123 force-pushed the music-keyboard-blocks-regression branch from 611d020 to d14d3d0 Compare February 18, 2021 05:53
@ksraj123
Copy link
Member Author

@walterbender Can you please have a look, most of the issues have been fixed. The PR's starting comment updated to document the regressions fixed in this. Please point out if any regressions still remain. Thanks

@walterbender walterbender merged commit 73770ff into sugarlabs:master Feb 20, 2021
@walterbender
Copy link
Member

Thanks for your persistence. I think it is all working much better now -- more predictable and intuitive behavior.

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.

3 participants