fix: random not able to exit piano roll hybrid mode issue when switching between different MIDI regions (introduced a region dragging start threshold)
This commit is contained in:
@@ -439,8 +439,9 @@ const MainContent: React.FC<MainContentProps> = ({
|
||||
}
|
||||
}
|
||||
|
||||
// If piano roll is visible, set this region as the active region
|
||||
if (showPianoRoll) {
|
||||
// Keep the active piano roll region in sync when that same region is updated.
|
||||
// Do not switch the editor to an unrelated region from generic move/resize updates.
|
||||
if (showPianoRoll && activeRegionId === regionId) {
|
||||
setActiveRegionId(regionId);
|
||||
|
||||
if (DEBUG_MODE.MAIN_CONTENT) {
|
||||
@@ -908,4 +909,4 @@ const MainContent: React.FC<MainContentProps> = ({
|
||||
);
|
||||
};
|
||||
|
||||
export default MainContent;
|
||||
export default MainContent;
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
import React from 'react';
|
||||
import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest';
|
||||
import { render, fireEvent } from '@testing-library/react';
|
||||
import RegionItem from './RegionItem';
|
||||
import { KGMidiRegion } from '../../core/region/KGMidiRegion';
|
||||
import { KGMainContentState } from '../../core/state/KGMainContentState';
|
||||
|
||||
vi.mock('../../stores/projectStore', () => ({
|
||||
useProjectStore: () => ({
|
||||
selectedRegionIds: [],
|
||||
timeSignature: { numerator: 4, denominator: 4 },
|
||||
bpm: 120,
|
||||
}),
|
||||
}));
|
||||
|
||||
describe('RegionItem', () => {
|
||||
beforeAll(() => {
|
||||
Object.defineProperty(HTMLCanvasElement.prototype, 'getContext', {
|
||||
value: vi.fn(() => ({
|
||||
clearRect: vi.fn(),
|
||||
fillRect: vi.fn(),
|
||||
})),
|
||||
});
|
||||
|
||||
class ResizeObserverMock {
|
||||
observe() {}
|
||||
unobserve() {}
|
||||
disconnect() {}
|
||||
}
|
||||
|
||||
vi.stubGlobal('ResizeObserver', ResizeObserverMock);
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
KGMainContentState.instance().setActiveTool('pointer');
|
||||
});
|
||||
|
||||
const renderRegion = (props: Partial<React.ComponentProps<typeof RegionItem>> = {}) => {
|
||||
const midiRegion = new KGMidiRegion('midi-1', 'track-1', 0, 'Test Region', 0, 4);
|
||||
|
||||
return render(
|
||||
<RegionItem
|
||||
id="midi-1"
|
||||
name="Test Region"
|
||||
style={{ left: '0px', width: '120px', position: 'absolute' }}
|
||||
onClick={vi.fn()}
|
||||
onDragStart={vi.fn()}
|
||||
onDrag={vi.fn()}
|
||||
onDragEnd={vi.fn()}
|
||||
midiRegion={midiRegion}
|
||||
{...props}
|
||||
/>
|
||||
);
|
||||
};
|
||||
|
||||
it('treats small pointer jitter as a click', () => {
|
||||
const onClick = vi.fn();
|
||||
const onDragStart = vi.fn();
|
||||
const onDrag = vi.fn();
|
||||
const onDragEnd = vi.fn();
|
||||
|
||||
const { container } = renderRegion({ onClick, onDragStart, onDrag, onDragEnd });
|
||||
const region = container.querySelector('.track-region');
|
||||
|
||||
expect(region).toBeTruthy();
|
||||
|
||||
fireEvent.mouseDown(region!, { clientX: 100, clientY: 100 });
|
||||
fireEvent.mouseMove(document, { clientX: 102, clientY: 102 });
|
||||
fireEvent.mouseUp(document, { clientX: 102, clientY: 102 });
|
||||
|
||||
expect(onClick).toHaveBeenCalledWith('midi-1');
|
||||
expect(onDragStart).not.toHaveBeenCalled();
|
||||
expect(onDrag).not.toHaveBeenCalled();
|
||||
expect(onDragEnd).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('starts a drag after crossing the movement threshold', () => {
|
||||
const onClick = vi.fn();
|
||||
const onDragStart = vi.fn();
|
||||
const onDrag = vi.fn();
|
||||
const onDragEnd = vi.fn();
|
||||
|
||||
const { container } = renderRegion({ onClick, onDragStart, onDrag, onDragEnd });
|
||||
const region = container.querySelector('.track-region');
|
||||
|
||||
expect(region).toBeTruthy();
|
||||
|
||||
fireEvent.mouseDown(region!, { clientX: 100, clientY: 100 });
|
||||
fireEvent.mouseMove(document, { clientX: 110, clientY: 100 });
|
||||
fireEvent.mouseUp(document, { clientX: 110, clientY: 100 });
|
||||
|
||||
expect(onClick).not.toHaveBeenCalled();
|
||||
expect(onDragStart).toHaveBeenCalledWith('midi-1', 100, 100);
|
||||
expect(onDrag).toHaveBeenCalledWith('midi-1', 10, 0);
|
||||
expect(onDragEnd).toHaveBeenCalledWith('midi-1');
|
||||
});
|
||||
});
|
||||
@@ -9,6 +9,8 @@ import { KGAudioRegion } from '../../core/region/KGAudioRegion';
|
||||
import { useProjectStore } from '../../stores/projectStore';
|
||||
import { KGMainContentState } from '../../core/state/KGMainContentState';
|
||||
|
||||
const DRAG_START_THRESHOLD_PX = 4;
|
||||
|
||||
interface RegionItemProps {
|
||||
id: string;
|
||||
name: string;
|
||||
@@ -73,7 +75,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
// Use refs to track states for immediate access
|
||||
const isResizingRef = useRef<boolean>(false);
|
||||
const isDraggingRef = useRef<boolean>(false);
|
||||
const hasMovedRef = useRef<boolean>(false);
|
||||
const isPendingDragRef = useRef<boolean>(false);
|
||||
|
||||
// Canvas ref for note visualization
|
||||
const canvasRef = useRef<HTMLCanvasElement | null>(null);
|
||||
@@ -391,7 +393,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
const activeTool = KGMainContentState.instance().getActiveTool();
|
||||
if (activeTool === 'pencil') {
|
||||
// Still allow click events to pass through for region selection
|
||||
if (!hasMovedRef.current && onClick) {
|
||||
if (onClick) {
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`REGION CLICKED (pencil mode): regionId=${id}`);
|
||||
}
|
||||
@@ -404,7 +406,7 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
e.preventDefault();
|
||||
|
||||
// Reset movement tracking
|
||||
hasMovedRef.current = false;
|
||||
isPendingDragRef.current = false;
|
||||
|
||||
// Store initial mouse position
|
||||
initialMousePosRef.current = { x: e.clientX, y: e.clientY };
|
||||
@@ -423,21 +425,12 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
onResizeStart(id, resizeEdge, e.clientX);
|
||||
}
|
||||
} else {
|
||||
// Start dragging
|
||||
// Wait for actual pointer movement before promoting this gesture to a drag.
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`DRAG START: regionId=${id}`);
|
||||
console.log(`PENDING REGION INTERACTION: regionId=${id}`);
|
||||
}
|
||||
|
||||
setIsDragging(true);
|
||||
isDraggingRef.current = true;
|
||||
|
||||
// Change cursor to grabbing during drag
|
||||
setCursor('grabbing');
|
||||
|
||||
// Call the onDragStart callback if provided
|
||||
if (onDragStart) {
|
||||
onDragStart(id, e.clientX, e.clientY);
|
||||
}
|
||||
isPendingDragRef.current = true;
|
||||
}
|
||||
|
||||
// Add global event listeners for mouse move and up
|
||||
@@ -447,9 +440,6 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
|
||||
// Handle global mouse move for resize or drag
|
||||
const handleGlobalMouseMove = (e: MouseEvent) => {
|
||||
// Set the hasMovedRef to true as soon as there's movement
|
||||
hasMovedRef.current = true;
|
||||
|
||||
if (isResizingRef.current) {
|
||||
// Handle resize
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
@@ -463,16 +453,35 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
if (onResize) {
|
||||
onResize(id, resizeEdge, deltaX);
|
||||
}
|
||||
} else if (isDraggingRef.current) {
|
||||
} else if (isDraggingRef.current || isPendingDragRef.current) {
|
||||
const deltaX = e.clientX - initialMousePosRef.current.x;
|
||||
const deltaY = e.clientY - initialMousePosRef.current.y;
|
||||
const movedEnough = Math.hypot(deltaX, deltaY) >= DRAG_START_THRESHOLD_PX;
|
||||
|
||||
if (!isDraggingRef.current) {
|
||||
if (!movedEnough) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`DRAG START: regionId=${id}`);
|
||||
}
|
||||
|
||||
isPendingDragRef.current = false;
|
||||
setIsDragging(true);
|
||||
isDraggingRef.current = true;
|
||||
setCursor('grabbing');
|
||||
|
||||
if (onDragStart) {
|
||||
onDragStart(id, initialMousePosRef.current.x, initialMousePosRef.current.y);
|
||||
}
|
||||
}
|
||||
|
||||
// Handle drag
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`DRAG MOVE: regionId=${id}, trackIndex=${trackIndex}`);
|
||||
}
|
||||
|
||||
// Calculate delta from initial position
|
||||
const deltaX = e.clientX - initialMousePosRef.current.x;
|
||||
const deltaY = e.clientY - initialMousePosRef.current.y;
|
||||
|
||||
// Call the onDrag callback if provided
|
||||
if (onDrag) {
|
||||
onDrag(id, deltaX, deltaY);
|
||||
@@ -511,16 +520,15 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
if (onDragEnd) {
|
||||
onDragEnd(id);
|
||||
}
|
||||
|
||||
// If there was no movement, treat it as a click
|
||||
if (!hasMovedRef.current && onClick) {
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`REGION CLICKED: regionId=${id}`);
|
||||
}
|
||||
onClick(id);
|
||||
} else if (isPendingDragRef.current && onClick) {
|
||||
if (DEBUG_MODE.REGION_ITEM) {
|
||||
console.log(`REGION CLICKED: regionId=${id}`);
|
||||
}
|
||||
onClick(id);
|
||||
}
|
||||
|
||||
isPendingDragRef.current = false;
|
||||
|
||||
// Remove global event listeners
|
||||
document.removeEventListener('mousemove', handleGlobalMouseMove);
|
||||
document.removeEventListener('mouseup', handleGlobalMouseUp);
|
||||
@@ -652,4 +660,4 @@ const RegionItem: React.FC<RegionItemProps> = ({
|
||||
);
|
||||
};
|
||||
|
||||
export default RegionItem;
|
||||
export default RegionItem;
|
||||
|
||||
@@ -492,6 +492,13 @@ const TrackGridItem: React.FC<TrackGridItemProps> = ({
|
||||
currentDragTop.current = null;
|
||||
currentDragRegion.current = null;
|
||||
|
||||
if (finalBarNumber === region.barNumber && finalTrackIndex === region.trackIndex) {
|
||||
if (DEBUG_MODE.TRACK_GRID_ITEM) {
|
||||
console.log(`Skipping no-op drag update for region ${regionId}`);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// Notify parent about drag end with final values
|
||||
if (onRegionDragEnd) {
|
||||
onRegionDragEnd(regionId, finalBarNumber, finalTrackIndex);
|
||||
@@ -589,4 +596,4 @@ const TrackGridItem: React.FC<TrackGridItemProps> = ({
|
||||
);
|
||||
};
|
||||
|
||||
export default TrackGridItem;
|
||||
export default TrackGridItem;
|
||||
|
||||
@@ -386,6 +386,13 @@ const TrackGridPanel: React.FC<TrackGridPanelProps> = ({
|
||||
// Find the region
|
||||
const region = regions.find(r => r.id === regionId);
|
||||
if (!region) return;
|
||||
|
||||
if (finalBarNumber === region.barNumber && finalTrackIndex === region.trackIndex) {
|
||||
if (DEBUG_MODE.TRACK_GRID_PANEL) {
|
||||
console.log(`Skipping no-op move for region ${regionId}`);
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
// Get the target track
|
||||
const targetTrack = tracks[finalTrackIndex];
|
||||
@@ -680,4 +687,4 @@ const TrackGridPanel: React.FC<TrackGridPanelProps> = ({
|
||||
);
|
||||
};
|
||||
|
||||
export default TrackGridPanel;
|
||||
export default TrackGridPanel;
|
||||
|
||||
@@ -0,0 +1,78 @@
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import { act } from '@testing-library/react';
|
||||
import { KGMidiTrack } from '../core/track/KGMidiTrack';
|
||||
|
||||
const mockProject = {
|
||||
getTimeSignature: () => ({ numerator: 4, denominator: 4 }),
|
||||
getMaxBars: () => 32,
|
||||
getBarWidthMultiplier: () => 1,
|
||||
getTracks: () => [new KGMidiTrack('Track 1', 0, 'acoustic_grand_piano')],
|
||||
getBpm: () => 120,
|
||||
getKeySignature: () => 'C major',
|
||||
getName: () => 'Test Project',
|
||||
getSelectedMode: () => 'major',
|
||||
getIsLooping: () => false,
|
||||
getLoopingRange: () => null,
|
||||
};
|
||||
|
||||
const mockCore = {
|
||||
getCurrentProject: () => mockProject,
|
||||
setPlayheadUpdateCallback: vi.fn(),
|
||||
setPlaybackStateChangeCallback: vi.fn(),
|
||||
getSelectedItems: () => [],
|
||||
onSelectionChanged: vi.fn(),
|
||||
canUndo: () => false,
|
||||
canRedo: () => false,
|
||||
getUndoDescription: () => '',
|
||||
getRedoDescription: () => '',
|
||||
setOnCommandHistoryChanged: vi.fn(),
|
||||
executeCommand: vi.fn(),
|
||||
clearSelectedItems: vi.fn(),
|
||||
getStatus: () => 'Ready',
|
||||
getPlayheadPosition: () => 0,
|
||||
getIsPlaying: () => false,
|
||||
};
|
||||
|
||||
vi.mock('../core/KGCore', () => ({
|
||||
KGCore: {
|
||||
instance: () => mockCore,
|
||||
},
|
||||
}));
|
||||
|
||||
vi.mock('../core/config/ConfigManager', () => ({
|
||||
ConfigManager: {
|
||||
instance: () => ({
|
||||
getIsInitialized: () => true,
|
||||
get: () => false,
|
||||
}),
|
||||
},
|
||||
}));
|
||||
|
||||
describe('projectStore piano roll state', () => {
|
||||
beforeEach(() => {
|
||||
vi.resetModules();
|
||||
});
|
||||
|
||||
it('clears hybrid state when opening a MIDI region', async () => {
|
||||
const { useProjectStore } = await import('./projectStore');
|
||||
|
||||
act(() => {
|
||||
useProjectStore.getState().openHybridMode('midi-a', 'audio-a');
|
||||
});
|
||||
|
||||
let state = useProjectStore.getState();
|
||||
expect(state.pianoRollMode).toBe('hybrid');
|
||||
expect(state.activeRegionId).toBe('midi-a');
|
||||
expect(state.hybridAudioRegionId).toBe('audio-a');
|
||||
|
||||
act(() => {
|
||||
useProjectStore.getState().openMidiPianoRoll('midi-b');
|
||||
});
|
||||
|
||||
state = useProjectStore.getState();
|
||||
expect(state.showPianoRoll).toBe(true);
|
||||
expect(state.pianoRollMode).toBe('midi-edit');
|
||||
expect(state.activeRegionId).toBe('midi-b');
|
||||
expect(state.hybridAudioRegionId).toBeNull();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user