diff --git a/js/actionCreators.js b/js/actionCreators.js index 8b6fc6d1..8eff995e 100644 --- a/js/actionCreators.js +++ b/js/actionCreators.js @@ -344,7 +344,7 @@ export function scrollDownFourTracks() { } function findLastIndex(arr, cb) { - for (var i = arr.length - 1; i >= 0; i--) { + for (let i = arr.length - 1; i >= 0; i--) { if (cb(arr[i])) { return i; } @@ -354,20 +354,26 @@ function findLastIndex(arr, cb) { export function dragSelected(offset) { return (dispatch, getState) => { - const { playlist: { trackOrder }, tracks } = getState(); + const { playlist: { trackOrder, tracks } } = getState(); const firstSelected = trackOrder.findIndex( - trackId => tracks[trackId].selected + trackId => tracks[trackId] && tracks[trackId].selected ); + if (firstSelected === -1) { + return; + } const lastSelected = findLastIndex( trackOrder, - trackId => tracks[trackId].selected + trackId => tracks[trackId] && tracks[trackId].selected ); + if (lastSelected === -1) { + throw new Error("We found a first selected, but not a last selected."); + } // Ensure we don't try to drag off either end. - const normalizedOffset = clamp( - offset, - -firstSelected, - trackOrder.length - 1 - lastSelected - ); - dispatch({ type: DRAG_SELECTED, offset: normalizedOffset }); + const min = -firstSelected; + const max = trackOrder.length - 1 - lastSelected; + const normalizedOffset = clamp(offset, min, max); + if (normalizedOffset !== 0) { + dispatch({ type: DRAG_SELECTED, offset: normalizedOffset }); + } }; } diff --git a/js/reducers/playlist.js b/js/reducers/playlist.js index 867adfd2..19661c31 100644 --- a/js/reducers/playlist.js +++ b/js/reducers/playlist.js @@ -15,27 +15,11 @@ import { DRAG_SELECTED } from "../actionTypes"; -import { shuffle } from "../utils"; - -const mapObject = (obj, iteratee) => - // TODO: Could return the original reference if no values change - Object.keys(obj).reduce((newObj, key) => { - newObj[key] = iteratee(obj[key], key); - return newObj; - }, {}); - -const filterObject = (obj, predicate) => - // TODO: Could return the original reference if no values change - Object.keys(obj).reduce((newObj, key) => { - if (predicate(obj[key], key)) { - newObj[key] = obj[key]; - } - return newObj; - }, {}); +import { shuffle, moveSelected, mapObject, filterObject } from "../utils"; const defaultPlaylistState = { trackOrder: [], - currentTrackIndex: null, + currentTrack: null, tracks: {} }; @@ -81,23 +65,15 @@ const playlist = (state = defaultPlaylistState, action) => { }; case REMOVE_ALL_TRACKS: // TODO: Consider disposing of ObjectUrls - return { ...state, trackOrder: [], currentTrackIndex: null, tracks: {} }; + return { ...state, trackOrder: [], currentTrack: null, tracks: {} }; case REMOVE_TRACKS: // TODO: Consider disposing of ObjectUrls const actionIds = action.ids.map(Number); - let { currentTrackIndex } = state; - const filteredTrackOrder = state.trackOrder.filter((id, i) => { - if (i === currentTrackIndex) { - // This is super janky: Using the .filter callback to do unrelated stuff. - // This is what you get when code reviews are not required. - currentTrackIndex = null; - } - return !actionIds.includes(id); - }); + const { currentTrack } = state; return { ...state, - trackOrder: filteredTrackOrder, - currentTrackIndex, + trackOrder: state.trackOrder.filter(id => !actionIds.includes(id)), + currentTrack: actionIds.includes(currentTrack) ? null : currentTrack, tracks: filterObject( state.tracks, (track, id) => !action.ids.includes(id) @@ -120,7 +96,7 @@ const playlist = (state = defaultPlaylistState, action) => { return { ...state, trackOrder: [...state.trackOrder, Number(action.id)], - currentTrackIndex: state.trackOrder.length, + currentTrack: action.id, tracks: { ...state.tracks, [action.id]: { @@ -145,13 +121,20 @@ const playlist = (state = defaultPlaylistState, action) => { case PLAY_TRACK: return { ...state, - currentTrackIndex: state.trackOrder.findIndex( - id => id === Number(action.id) - ) + currentTrack: action.id }; case DRAG_SELECTED: - return state; - + return { + ...state, + trackOrder: moveSelected( + state.trackOrder, + i => { + const id = state.trackOrder[i]; + return state.tracks[id].selected; + }, + action.offset + ) + }; default: return state; } diff --git a/js/selectors.js b/js/selectors.js index 5dad2ac4..a8ba84a9 100644 --- a/js/selectors.js +++ b/js/selectors.js @@ -62,18 +62,16 @@ export const getRunningTimeMessage = createSelector( `${getTimeStr(selectedRunningTime)}/${getTimeStr(totalRunningTime)}` ); -export const getCurrentTrackIndex = state => state.playlist.currentTrackIndex; +// TODO: use slectors to get memoization +export const getCurrentTrackIndex = state => + state.playlist.trackOrder.indexOf(state.playlist.currentTrack); export const getCurrentTrackNumber = createSelector( getCurrentTrackIndex, currentTrackIndex => currentTrackIndex + 1 ); -export const getCurrentTrackId = createSelector( - getTrackOrder, - getCurrentTrackIndex, - (trackOrder, currentTrackIndex) => trackOrder[currentTrackIndex] -); +export const getCurrentTrackId = state => state.playlist.currentTrack; export const nextTrack = (state, n = 1) => { const { playlist: { trackOrder }, media: { repeat } } = state; diff --git a/js/selectors.test.js b/js/selectors.test.js index 8c2a922e..179f6ede 100644 --- a/js/selectors.test.js +++ b/js/selectors.test.js @@ -31,7 +31,7 @@ describe("getEqfData", () => { describe("nextTrack", () => { it("returns null if you don't have any tracks", () => { const state = { - playlist: { currentTrackIndex: null, trackOrder: [] }, + playlist: { currentTrack: null, trackOrder: [] }, media: { repeat: false } }; expect(state.playlist.trackOrder).toEqual([]); @@ -40,7 +40,7 @@ describe("nextTrack", () => { it("returns null if you are going forward from the last track and repeat is not turned on", () => { const state = { - playlist: { currentTrackIndex: 2, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 3, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state)).toBe(null); @@ -48,7 +48,7 @@ describe("nextTrack", () => { it("wraps around if you are going forward from the last track and repeat _is_ turned on", () => { const state = { - playlist: { currentTrackIndex: 2, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 3, trackOrder: [1, 2, 3] }, media: { repeat: true } }; expect(nextTrack(state)).toBe(1); @@ -56,7 +56,7 @@ describe("nextTrack", () => { it("returns null if you are going backward from the first track and repeat is not turned on", () => { const state = { - playlist: { currentTrackIndex: 0, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 1, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state, -1)).toBe(null); @@ -64,7 +64,7 @@ describe("nextTrack", () => { it("wraps around if you are going backwards from the first track and repeat _is_ turned on", () => { const state = { - playlist: { currentTrackIndex: 0, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 1, trackOrder: [1, 2, 3] }, media: { repeat: true } }; expect(nextTrack(state, -1)).toBe(3); @@ -72,7 +72,7 @@ describe("nextTrack", () => { it("does a normal next", () => { const state = { - playlist: { currentTrackIndex: 1, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 2, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state)).toBe(3); @@ -80,7 +80,7 @@ describe("nextTrack", () => { it("does a normal previous", () => { const state = { - playlist: { currentTrackIndex: 1, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 2, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state, -1)).toBe(1); @@ -88,7 +88,7 @@ describe("nextTrack", () => { it("takes you to the last track if you overshoot", () => { const state = { - playlist: { currentTrackIndex: 1, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 2, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state, 10)).toBe(3); @@ -96,7 +96,7 @@ describe("nextTrack", () => { it("takes you to the first track if you overshoot", () => { const state = { - playlist: { currentTrackIndex: 1, trackOrder: [1, 2, 3] }, + playlist: { currentTrack: 2, trackOrder: [1, 2, 3] }, media: { repeat: false } }; expect(nextTrack(state, -10)).toBe(1); diff --git a/js/utils.js b/js/utils.js index 2ba56ab8..010d3e12 100644 --- a/js/utils.js +++ b/js/utils.js @@ -153,10 +153,10 @@ export const moveSelected = (arr, isSelected, offset) => { for (let i = 0; i < newArr.length; i++) { const from = i - offset; // Is a value supposed to move here? - if (isSelected(from)) { + if (from >= 0 && from < arr.length && isSelected(from)) { newArr[i] = arr[from]; } else { - while (isSelected(next)) { + while (next < arr.length && isSelected(next)) { next++; } newArr[i] = arr[next]; @@ -165,3 +165,19 @@ export const moveSelected = (arr, isSelected, offset) => { } return newArr; }; + +export const mapObject = (obj, iteratee) => + // TODO: Could return the original reference if no values change + Object.keys(obj).reduce((newObj, key) => { + newObj[key] = iteratee(obj[key], key); + return newObj; + }, {}); + +export const filterObject = (obj, predicate) => + // TODO: Could return the original reference if no values change + Object.keys(obj).reduce((newObj, key) => { + if (predicate(obj[key], key)) { + newObj[key] = obj[key]; + } + return newObj; + }, {}); diff --git a/js/utils.test.js b/js/utils.test.js index 00a51764..98e3e914 100644 --- a/js/utils.test.js +++ b/js/utils.test.js @@ -206,4 +206,12 @@ describe("moveSelected", () => { ) ).toEqual(["a", "d", "e", "f", "b", "c", "g", "h"]); }); + it("works for a simple example", () => { + const arr = [true, false, false]; + expect(moveSelected(arr, i => arr[i], 1)).toEqual([false, true, false]); + }); + it("works for a simple negative example", () => { + const arr = [false, false, true]; + expect(moveSelected(arr, i => arr[i], -1)).toEqual([false, true, false]); + }); });