Progress

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 });
+    }
   };
 }

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;
   }

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;

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);

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;
+  }, {});

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]);
+  });
 });