From a1c8727d4496d6ce5bf8b38a01a7df5317c3b9b7 Mon Sep 17 00:00:00 2001 From: Xiaohan-Tian <157918347+Xiaohan-Tian@users.noreply.github.com> Date: Fri, 8 May 2026 17:09:53 -0700 Subject: [PATCH] fix: when piano roll window is not visible, select a row on list event table will cause current MIDI region lose selection --- src/components/ListEventPanel.test.tsx | 137 ++++++++++++++++++++++--- src/components/ListEventPanel.tsx | 9 +- src/components/MainContent.test.tsx | 128 +++++++++++++++++++++++ src/components/MainContent.tsx | 2 +- 4 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 src/components/MainContent.test.tsx diff --git a/src/components/ListEventPanel.test.tsx b/src/components/ListEventPanel.test.tsx index e70b074..d76e889 100644 --- a/src/components/ListEventPanel.test.tsx +++ b/src/components/ListEventPanel.test.tsx @@ -1,8 +1,18 @@ import React from 'react'; -import { describe, expect, it, vi } from 'vitest'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; import { fireEvent, render, screen } from '@testing-library/react'; import ListEventPanel from './ListEventPanel'; -import { createMockMidiNote, createMockMidiPitchBend, createMockMidiRegion, createMockMidiTrack } from '../test/utils/mock-data'; +import { KGMidiControllerEvent } from '../core/midi/KGMidiControllerEvent'; +import { KGMidiNote } from '../core/midi/KGMidiNote'; +import { KGMidiPitchBend } from '../core/midi/KGMidiPitchBend'; +import { KGRegion } from '../core/region/KGRegion'; +import { + createMockMidiControllerEvent, + createMockMidiNote, + createMockMidiPitchBend, + createMockMidiRegion, + createMockMidiTrack, +} from '../test/utils/mock-data'; const region = createMockMidiRegion({ id: 'region-1', @@ -11,26 +21,96 @@ const region = createMockMidiRegion({ startFromBeat: 4, notes: [createMockMidiNote({ id: 'note-1', pitch: 60, startBeat: 1, endBeat: 2, velocity: 96 })], pitchBends: [createMockMidiPitchBend({ id: 'bend-1', beat: 0.5, value: 12288 })], + controllerEventsByType: Array.from({ length: 128 }, (_, index) => ( + index === 11 ? [createMockMidiControllerEvent({ id: 'cc11-1', beat: 0.75, value: 100 })] : [] + )), }); const track = createMockMidiTrack({ id: 1, regions: [region] }); +type MockStoreState = { + tracks: typeof track[]; + activeRegionId: string | null; + selectedRegionIds: string[]; + timeSignature: { numerator: number; denominator: number }; + selectedNoteIds: string[]; + selectedPitchBendIds: string[]; + selectedControllerEventIds: string[]; + playheadPosition: number; + updateTrack: ReturnType; + refreshProjectState: ReturnType; + bumpAutomationRedrawVersion: ReturnType; +}; + +const storeState: MockStoreState = { + tracks: [track], + activeRegionId: 'region-1', + selectedRegionIds: ['region-1'], + timeSignature: { numerator: 4, denominator: 4 }, + selectedNoteIds: [], + selectedPitchBendIds: [], + selectedControllerEventIds: [], + playheadPosition: 4, + updateTrack: vi.fn().mockResolvedValue(undefined), + refreshProjectState: vi.fn(), + bumpAutomationRedrawVersion: vi.fn(), +}; + +let selectedItems: Array = []; + +const syncStoreSelectionFromCore = () => { + storeState.selectedRegionIds = selectedItems + .filter(item => item instanceof KGRegion) + .map(item => item.getId()); + storeState.selectedNoteIds = selectedItems + .filter(item => item instanceof KGMidiNote) + .map(item => item.getId()); + storeState.selectedPitchBendIds = selectedItems + .filter(item => item instanceof KGMidiPitchBend) + .map(item => item.getId()); + storeState.selectedControllerEventIds = selectedItems + .filter(item => item instanceof KGMidiControllerEvent) + .map(item => item.getId()); +}; + vi.mock('../stores/projectStore', () => ({ - useProjectStore: () => ({ - tracks: [track], - activeRegionId: 'region-1', - selectedRegionIds: ['region-1'], - timeSignature: { numerator: 4, denominator: 4 }, - selectedNoteIds: [], - selectedPitchBendIds: [], - selectedControllerEventIds: [], - playheadPosition: 4, - updateTrack: vi.fn().mockResolvedValue(undefined), - refreshProjectState: vi.fn(), - bumpAutomationRedrawVersion: vi.fn(), - }), + useProjectStore: () => storeState, +})); + +vi.mock('../core/KGCore', () => ({ + KGCore: { + instance: vi.fn(() => ({ + getSelectedItems: () => selectedItems, + addSelectedItems: (items: typeof selectedItems) => { + const nextIds = new Set(items.map(item => item.getId())); + selectedItems = [...selectedItems.filter(item => !nextIds.has(item.getId())), ...items]; + syncStoreSelectionFromCore(); + }, + removeSelectedItems: (items: typeof selectedItems) => { + const removedIds = new Set(items.map(item => item.getId())); + selectedItems = selectedItems.filter(item => !removedIds.has(item.getId())); + syncStoreSelectionFromCore(); + }, + })), + }, })); describe('ListEventPanel', () => { + beforeEach(() => { + selectedItems = [region]; + region.select(); + region.getNotes().forEach(note => note.deselect()); + region.getPitchBends().forEach(pitchBend => pitchBend.deselect()); + region.getControllerEventsByType().forEach(events => events.forEach(controllerEvent => controllerEvent.deselect())); + storeState.activeRegionId = 'region-1'; + storeState.selectedRegionIds = ['region-1']; + storeState.selectedNoteIds = []; + storeState.selectedPitchBendIds = []; + storeState.selectedControllerEventIds = []; + storeState.updateTrack.mockClear(); + storeState.refreshProjectState.mockClear(); + storeState.bumpAutomationRedrawVersion.mockClear(); + }); + it('renders note and pitch bend rows and toggles them independently', () => { render(); @@ -50,4 +130,31 @@ describe('ListEventPanel', () => { fireEvent.click(screen.getByRole('button', { name: 'Pitch Bends' })); expect(screen.getByText('Pitch Bend')).toBeInTheDocument(); }); + + it('keeps the region selected while selecting rows', () => { + const { rerender } = render(); + + fireEvent.click(screen.getByText('C4').closest('tr')!); + rerender(); + + expect(storeState.selectedRegionIds).toEqual(['region-1']); + expect(storeState.selectedNoteIds).toEqual(['note-1']); + expect(screen.queryByText('Please select a MIDI region, or open one in the Piano Roll, to view its event list.')).not.toBeInTheDocument(); + expect(screen.getByText('C4').closest('tr')).toHaveClass('selected'); + }); + + it('supports additive row selection without dropping the owning region', () => { + const { rerender } = render(); + + fireEvent.click(screen.getByText('C4').closest('tr')!); + rerender(); + fireEvent.click(screen.getByText('Pitch Bend').closest('tr')!, { shiftKey: true }); + rerender(); + + expect(storeState.selectedRegionIds).toEqual(['region-1']); + expect(storeState.selectedNoteIds).toEqual(['note-1']); + expect(storeState.selectedPitchBendIds).toEqual(['bend-1']); + expect(screen.getByText('C4').closest('tr')).toHaveClass('selected'); + expect(screen.getByText('Raw 12288 | 0.500 | 1.00 st').closest('tr')).toHaveClass('selected'); + }); }); diff --git a/src/components/ListEventPanel.tsx b/src/components/ListEventPanel.tsx index 8a56f01..34e0f07 100644 --- a/src/components/ListEventPanel.tsx +++ b/src/components/ListEventPanel.tsx @@ -297,6 +297,11 @@ const ListEventPanel: React.FC = ({ isVisible }) => { .filter(row => nextSelectedIds.has(row.id)) .map(row => row.type === 'note' ? row.note : row.type === 'pitch-bend' ? row.pitchBend : row.controllerEvent); const core = KGCore.instance(); + const previouslySelectedRegionEvents = core.getSelectedItems().filter(item => + (item instanceof KGMidiNote && activeMidiRegion.getNotes().some(note => note.getId() === item.getId())) || + (item instanceof KGMidiPitchBend && activeMidiRegion.getPitchBends().some(pitchBend => pitchBend.getId() === item.getId())) || + (item instanceof KGMidiControllerEvent && activeMidiRegion.getAllControllerEventsFlattened().some(({ event }) => event.getId() === item.getId())) + ); activeMidiRegion.getNotes().forEach(note => { if (nextSelectedIds.has(note.getId())) note.select(); @@ -313,7 +318,9 @@ const ListEventPanel: React.FC = ({ isVisible }) => { }); }); - core.clearSelectedItems(); + if (previouslySelectedRegionEvents.length > 0) { + core.removeSelectedItems(previouslySelectedRegionEvents); + } if (selectedEvents.length > 0) { core.addSelectedItems(selectedEvents); } diff --git a/src/components/MainContent.test.tsx b/src/components/MainContent.test.tsx new file mode 100644 index 0000000..fc0237d --- /dev/null +++ b/src/components/MainContent.test.tsx @@ -0,0 +1,128 @@ +import React from 'react'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { fireEvent, render, screen } from '@testing-library/react'; +import MainContent from './MainContent'; +import { KGMidiRegion } from '../core/region/KGMidiRegion'; +import { createMockMidiTrack } from '../test/utils/mock-data'; + +const midiRegion = new KGMidiRegion('region-1', '1', 0, 'Region 1', 0, 4); +const track = createMockMidiTrack({ id: 1, regions: [midiRegion] }); + +const storeState = { + tracks: [track], + maxBars: 8, + barWidthMultiplier: 1, + reorderTracks: vi.fn(), + updateTrack: vi.fn(), + updateTrackProperties: vi.fn(), + timeSignature: { numerator: 4, denominator: 4 }, + setPlayheadPosition: vi.fn(), + playheadPosition: 0, + isPlaying: false, + autoScrollEnabled: false, + setAutoScrollEnabled: vi.fn(), + clearAllSelections: vi.fn(() => { + storeState.selectedRegionIds = []; + }), + setSelectedTrack: vi.fn(), + selectedRegionIds: [] as string[], + showPianoRoll: false, + activeRegionId: null as string | null, + setShowPianoRoll: vi.fn((show: boolean) => { + storeState.showPianoRoll = show; + }), + setActiveRegionId: vi.fn((regionId: string | null) => { + storeState.activeRegionId = regionId; + }), + pianoRollMode: 'midi-edit' as const, + openMidiPianoRoll: vi.fn(), + openSpectrogramViewer: vi.fn(), + openHybridMode: vi.fn(), + hybridAudioRegionId: null as string | null, + addTrack: vi.fn(), + addAudioTrack: vi.fn(), + projectName: 'Test Project', + savedProjectName: 'Test Project', + requestPianoRollScroll: vi.fn(), + mainContentScrollRequest: null, + activeTrackAutomationTrackId: null, + activeTrackAutomationType: null, + selectedTrackAutomationPointIds: [], + bumpTrackAutomationRedrawVersion: vi.fn(), + refreshProjectState: vi.fn(), + showInstrumentSelection: false, + isLooping: false, + loopingRange: [0, 0] as [number, number], +}; + +// eslint-disable-next-line no-unused-vars +type StoreSelector = (...args: [typeof storeState]) => unknown; +// eslint-disable-next-line no-unused-vars +type RegionClickHandler = (...args: [string, { shiftKey: boolean }]) => void; + +vi.mock('../stores/projectStore', () => ({ + useProjectStore: (selector?: StoreSelector) => ( + selector ? selector(storeState) : storeState + ), +})); + +vi.mock('../core/KGCore', () => ({ + KGCore: { + instance: () => ({ + addSelectedItems: (items: KGMidiRegion[]) => { + storeState.selectedRegionIds = items.map(item => item.getId()); + }, + clearSelectedItems: () => { + storeState.selectedRegionIds = []; + }, + executeCommand: vi.fn(), + }), + }, +})); + +vi.mock('../hooks/useRegionOperations', () => ({ + useRegionOperations: () => ({ + deleteSelectedRegions: vi.fn(() => false), + }), +})); + +vi.mock('./track/TrackInfoPanel', () => ({ + default: () =>
, +})); + +vi.mock('./track/TrackGridPanel', () => ({ + default: ({ onRegionClick }: { onRegionClick?: RegionClickHandler }) => ( + + ), +})); + +vi.mock('./piano-roll/PianoRoll', () => ({ + default: () =>
, +})); + +describe('MainContent', () => { + beforeEach(() => { + storeState.selectedRegionIds = []; + storeState.activeRegionId = null; + storeState.showPianoRoll = false; + storeState.clearAllSelections.mockClear(); + storeState.setSelectedTrack.mockClear(); + storeState.setShowPianoRoll.mockClear(); + storeState.setActiveRegionId.mockClear(); + storeState.openMidiPianoRoll.mockClear(); + storeState.openSpectrogramViewer.mockClear(); + }); + + it('updates activeRegionId when selecting a region with piano roll closed', () => { + render(); + + fireEvent.click(screen.getByRole('button', { name: 'select-region' })); + + expect(storeState.activeRegionId).toBe('region-1'); + expect(storeState.setActiveRegionId).toHaveBeenCalledWith('region-1'); + expect(storeState.showPianoRoll).toBe(false); + expect(storeState.openMidiPianoRoll).not.toHaveBeenCalled(); + }); +}); diff --git a/src/components/MainContent.tsx b/src/components/MainContent.tsx index 977cb56..3cb657b 100644 --- a/src/components/MainContent.tsx +++ b/src/components/MainContent.tsx @@ -587,6 +587,7 @@ const MainContent: React.FC = ({ : null; setSelectedRegionId(lastSelectedRegionId); + setActiveRegionId(lastSelectedRegionId); if (DEBUG_MODE.MAIN_CONTENT) { console.log(`Selected regions: ${selectedRegions.map(selectedRegion => selectedRegion.getId()).join(', ')}`); @@ -598,7 +599,6 @@ const MainContent: React.FC = ({ if (!lastSelectedRegionId) { setShowPianoRoll(false); - setActiveRegionId(null); return; }