From d5795c528142b45e28968c6239931dec3567282e Mon Sep 17 00:00:00 2001 From: Xiaohan-Tian <157918347+Xiaohan-Tian@users.noreply.github.com> Date: Fri, 1 May 2026 20:34:21 -0700 Subject: [PATCH] fix: random not able to exit piano roll hybrid mode issue when switching between different MIDI regions (introduced a region dragging start threshold) --- src/components/MainContent.tsx | 7 +- src/components/track/RegionItem.test.tsx | 97 ++++++++++++++++++++++++ src/components/track/RegionItem.tsx | 70 +++++++++-------- src/components/track/TrackGridItem.tsx | 9 ++- src/components/track/TrackGridPanel.tsx | 9 ++- src/stores/projectStore.test.ts | 78 +++++++++++++++++++ 6 files changed, 234 insertions(+), 36 deletions(-) create mode 100644 src/components/track/RegionItem.test.tsx create mode 100644 src/stores/projectStore.test.ts diff --git a/src/components/MainContent.tsx b/src/components/MainContent.tsx index 1003b19..0d9e6eb 100644 --- a/src/components/MainContent.tsx +++ b/src/components/MainContent.tsx @@ -439,8 +439,9 @@ const MainContent: React.FC = ({ } } - // 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 = ({ ); }; -export default MainContent; \ No newline at end of file +export default MainContent; diff --git a/src/components/track/RegionItem.test.tsx b/src/components/track/RegionItem.test.tsx new file mode 100644 index 0000000..7172df7 --- /dev/null +++ b/src/components/track/RegionItem.test.tsx @@ -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> = {}) => { + const midiRegion = new KGMidiRegion('midi-1', 'track-1', 0, 'Test Region', 0, 4); + + return render( + + ); + }; + + 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'); + }); +}); diff --git a/src/components/track/RegionItem.tsx b/src/components/track/RegionItem.tsx index 1a7b1ee..da5398c 100644 --- a/src/components/track/RegionItem.tsx +++ b/src/components/track/RegionItem.tsx @@ -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 = ({ // Use refs to track states for immediate access const isResizingRef = useRef(false); const isDraggingRef = useRef(false); - const hasMovedRef = useRef(false); + const isPendingDragRef = useRef(false); // Canvas ref for note visualization const canvasRef = useRef(null); @@ -391,7 +393,7 @@ const RegionItem: React.FC = ({ 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 = ({ 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 = ({ 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 = ({ // 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 = ({ 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 = ({ 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 = ({ ); }; -export default RegionItem; \ No newline at end of file +export default RegionItem; diff --git a/src/components/track/TrackGridItem.tsx b/src/components/track/TrackGridItem.tsx index 20e891c..f85e481 100644 --- a/src/components/track/TrackGridItem.tsx +++ b/src/components/track/TrackGridItem.tsx @@ -492,6 +492,13 @@ const TrackGridItem: React.FC = ({ 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 = ({ ); }; -export default TrackGridItem; \ No newline at end of file +export default TrackGridItem; diff --git a/src/components/track/TrackGridPanel.tsx b/src/components/track/TrackGridPanel.tsx index 43718bc..75be0f6 100644 --- a/src/components/track/TrackGridPanel.tsx +++ b/src/components/track/TrackGridPanel.tsx @@ -386,6 +386,13 @@ const TrackGridPanel: React.FC = ({ // 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 = ({ ); }; -export default TrackGridPanel; \ No newline at end of file +export default TrackGridPanel; diff --git a/src/stores/projectStore.test.ts b/src/stores/projectStore.test.ts new file mode 100644 index 0000000..a99935c --- /dev/null +++ b/src/stores/projectStore.test.ts @@ -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(); + }); +});