fix: correct custom pitch pie menu order, rotation and preview - #7981
Open
abhijitdalal26 wants to merge 1 commit into
Open
fix: correct custom pitch pie menu order, rotation and preview#7981abhijitdalal26 wants to merge 1 commit into
abhijitdalal26 wants to merge 1 commit into
Conversation
The custom pitch pie menu had three problems compared to the regular pitch wheels: - note labels were built straight from the temperament's pitch number order (low to high), so they showed up reversed relative to every other pitch wheel, which lists high to low (e.g. solfege is "ti la sol fa mi re do") - clickModeRotate was explicitly disabled on the custom temperament wheel and the note wheel, so dragging to rotate never worked even though every other pitch wheel supports it - block.customID was only ever set once the user clicked the temperament tab, so with a single custom temperament defined (the common case) the first note click passed a null customID into getCustomFrequency, which fell through to returning the raw note name instead of a frequency, and triggerAttackRelease got a string it can't play Reverse the label order when building the note wheel, drop the two clickModeRotate=false assignments, and set customID from the selected temperament as soon as the menu opens instead of waiting for a tab click. Also fixed the initial wheel navigation index math to match the new reversed order. Fixes sugarlabs#2255
Contributor
|
@abhijitdalal26 |
Contributor
|
and for the code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the three custom pitch pie menu problems described in #2255:
Reversed order - the note wheel was built straight from the temperament's pitch-number order (low to high). Every other pitch wheel in the app lists notes high to low (solfege is built as "ti la sol fa mi re do"), so the custom pitch wheel looked backwards next to them. Fixed by reversing the label array when it's built, and adjusting the initial-selection index math to match.
No rotation -
clickModeRotatewas explicitly set tofalseon both the temperament wheel and the note wheel inpiemenuCustomNotes. Nothing else in the regular pitch pie menu does this (only the exit button disables rotation), so dragging to rotate the custom pitch wheel never worked. Removed both assignments.No pitch preview -
block.customIDwas only ever set inside the temperament tab'snavigateFunction, so it stayednulluntil the user clicked a temperament tab. With only one custom temperament defined (the common case), the first note click calledgetCustomFrequency(notes, null), which can't find a match and falls through to returning the raw note name (e.g."do") instead of a frequency. That string then got handed totriggerAttackRelease, which can't play it, so there was no preview sound. Fixed by settingblock.customIDfrom the selected temperament as soon as the menu opens.Changes
js/piemenus.js: reverse note label order inpiemenuCustomNotes, dropclickModeRotate = falsefor_customWheel/_cusNoteWheel, initializeblock.customIDup front, fix initial wheel navigation index to account for the reversed order.Testing
npx eslint js/piemenus.jsnpx jest js/__tests__/piemenus.test.js --runInBand --coverage=false(8/8 passing)npx jest js/blocks/__tests__/PitchBlocks.test.js js/utils/__tests__/synthutils.test.js --runInBand --coverage=false(199/199 passing)Fixes #2255