-
Notifications
You must be signed in to change notification settings - Fork 3
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
CT: sound generation does not support adding new items #244
Comments
This was noted in #243 (comment), and may be related to that issue. But I'll leave this open as a separate issue. |
In the Discrete screen, the value of the "Function of" (domain) combo box determines which items are visible in the "Equation" combo box: ![]() The problem here occurs with multitouch. It's possible for the selected item to become invisible before its sound plays. That results in an assertion failure in ComboBoxListBox. Here's the assertion, which @jbphet and I added recently in phetsims/sun@9dffc89 for phetsims/sun#862: // The selected item didn't provide a sound player, so use a default based on its position within the list
// of visible selections.
const selectionIndex = this.getVisibleListItemNodes().indexOf( this.selectionOnFireAction );
assert && assert( selectionIndex !== -1, 'sound generation does not support adding new items' );
defaultItemSelectedSoundPlayers[ selectionIndex ].play(); My inclination is to change this to something like the following. But I'll discuss with @jbphet first. // The selected item didn't provide a sound player, so use a default based on its position within the list
// of visible selections. With multitouch, it's possible that the selected item may become invisible before
// we attempt to play its sound, so play only if it's still visible.
// See https://github.com/phetsims/fourier-making-waves/issues/244
const selectionIndex = this.getVisibleListItemNodes().indexOf( this.selectionOnFireAction );
if ( selectionIndex !== -1 ) {
defaultItemSelectedSoundPlayers[ selectionIndex ].play();
} |
I discussed with @jbphet, and he supported the change in phetsims/sun@ab87ee8. Since the failing assertion no longer exists, I feel comfortable closing this without waiting a few CT cycles. |
The text was updated successfully, but these errors were encountered: