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 pitch slider arrow key interaction #4547

Merged
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update pitch slider to control active slider with arrow keys
Anvita Prasad committed Mar 21, 2025
commit e9db124aa0dcf5e8c4397e098223ebc13b612e89
6 changes: 4 additions & 2 deletions js/activity.js
Original file line number Diff line number Diff line change
@@ -2994,9 +2994,11 @@ class Activity {
this.__keyPressed = (event) => {
// First, check if the pitch slider is open
if (window.widgetWindows.isOpen("slider") === true) {
// If the event is an arrow key, don't process it at all
// If the event is an arrow key, let the PitchSlider handle it
if (event.keyCode === 37 || event.keyCode === 38 ||
event.keyCode === 39 || event.keyCode === 40) {
// Simply prevent default behavior here
// The actual pitch slider handling is done in the PitchSlider class
event.preventDefault();
event.stopPropagation();
return false;
@@ -3093,7 +3095,7 @@ class Activity {
break;
}
}

if ((event.altKey && !disableKeys) || (event.keyCode == 13) || (event.key == "/") || (event.key == "\\") ) {
switch (event.keyCode) {
case 66: // 'B'
68 changes: 46 additions & 22 deletions js/widgets/pitchslider.js
Original file line number Diff line number Diff line change
@@ -33,7 +33,8 @@ class PitchSlider {
this._delta = 0;
this.sliders = {};
this._cellScale = 0;
this.isActive = false; // Flag to indicate if the slider is active
this.isActive = false;
this.activeSlider = null;
}

/**
@@ -54,33 +55,56 @@ class PitchSlider {
this._cellScale = 1.0;
this.widgetWindow = window.widgetWindows.windowFor(this, "pitch slider", "slider", true);

// Set global flag that pitch slider is active

this.isActive = true;

// Store reference to this pitch slider in the logo object for global access

activity.logo.pitchSlider = this;

this.widgetWindow.onclose = () => {
for (const osc of oscillators) osc.triggerRelease();
this.isActive = false; // Reset flag when widget is closed
activity.logo.pitchSlider = null; // Remove reference
this.isActive = false;
activity.logo.pitchSlider = null;
this.widgetWindow.destroy();
};

// Add global keyboard event listener with the highest capture priority

const keyHandler = (event) => {
if (!this.isActive) return;

if (event.key === 'ArrowUp' || event.key === 'ArrowDown' ||
event.key === 'ArrowLeft' || event.key === 'ArrowRight') {

event.preventDefault();
event.stopPropagation();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe this can be double space instead of triple space?


const sliderToUse = this.activeSlider !== null ? this.activeSlider : 0;

if (this.sliders[sliderToUse]) {
const slider = this.sliders[sliderToUse];
const min = parseFloat(slider.min);
const max = parseFloat(slider.max);
const currentValue = parseFloat(slider.value);

if (event.key === 'ArrowUp' || event.key === 'ArrowRight') {
// Move up by a semitone
slider.value = Math.min(currentValue * PitchSlider.SEMITONE, max);
} else if (event.key === 'ArrowDown' || event.key === 'ArrowLeft') {
// Move down by a semitone
slider.value = Math.max(currentValue / PitchSlider.SEMITONE, min);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here... no need for so many empty lines between code blocks.


const inputEvent = new Event('input', { bubbles: true });
slider.dispatchEvent(inputEvent);
}

return false;
}
};

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here.

// Add the handler to various elements to ensure it's captured
this.widgetWindow.getWidgetBody().addEventListener('keydown', keyHandler, true);

document.addEventListener('keydown', keyHandler, true);

const MakeToolbar = (id) => {
@@ -98,25 +122,25 @@ class PitchSlider {
"pitchSlider"
);

// Specifically prevent arrow keys from affecting the slider
slider.addEventListener('keydown', (event) => {
if (event.key === 'ArrowUp' || event.key === 'ArrowDown' ||
event.key === 'ArrowLeft' || event.key === 'ArrowRight') {
event.preventDefault();
event.stopPropagation();
return false;
}
}, true);
// Set the leftmost slider as active by default
if (id == 0) {
this.activeSlider = id;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here and...

// label for frequency

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here

slider.addEventListener('mousedown', () => {
this.activeSlider = id;
});

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too


const freqLabel = document.createElement("div");
freqLabel.className = "wfbtItem";
toolBarDiv.appendChild(freqLabel);
freqLabel.innerHTML = `<label>${this.frequencies[id]}</label>`;

this.sliders[id] = slider;
const changeFreq = () => {
this.frequencies[id] = this.sliders[id].value;
this.frequencies[id] = parseFloat(this.sliders[id].value);
oscillators[id].frequency.linearRampToValueAtTime(
this.frequencies[id],
Tone.now() + 0.05
@@ -139,7 +163,7 @@ class PitchSlider {
_("Move up"),
toolBarDiv
).onclick = () => {
slider.value = Math.min(slider.value * PitchSlider.SEMITONE, max); //value is a string
slider.value = Math.min(parseFloat(slider.value) * PitchSlider.SEMITONE, max);
changeFreq();
oscillators[id].triggerAttackRelease(this.frequencies[id], "4n");
};
@@ -150,7 +174,7 @@ class PitchSlider {
_("Move down"),
toolBarDiv
).onclick = () => {
slider.value = Math.max(slider.value / PitchSlider.SEMITONE, min); //value is a string
slider.value = Math.max(parseFloat(slider.value) / PitchSlider.SEMITONE, min);
changeFreq();
oscillators[id].triggerAttackRelease(this.frequencies[id], "4n");
};
@@ -169,7 +193,7 @@ class PitchSlider {
MakeToolbar(id);
}

activity.textMsg(_("Use the up/down buttons to change pitch. Arrow keys are disabled."), 3000);
activity.textMsg(_("Use the up/down buttons or arrow keys to change pitch."), 3000);
activity.textMsg(_("Click on the slider to create a note block."), 3000);
setTimeout(this.widgetWindow.sendToCenter, 0);
}