From bbaef5f7b0c91e4d6392fd4bc922864fbb5cdcdc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9e?= Date: Tue, 12 Dec 2023 19:45:24 +0000 Subject: [PATCH] Move playlist selectors into slice (#2801) * Move playlist selectors into slice * Compute playlist.active at selection time --- src/containers/PlaylistManagerPanel.tsx | 9 +- src/mobile/containers/MainView.jsx | 2 +- src/mobile/containers/PlaylistManager.tsx | 8 +- src/mobile/containers/PlaylistPanel.js | 12 +- src/reducers/__tests__/playlists.js | 77 ++++++------- src/reducers/playlists.ts | 80 ++++++++----- src/selectors/playlistSelectors.js | 131 ---------------------- 7 files changed, 100 insertions(+), 219 deletions(-) delete mode 100644 src/selectors/playlistSelectors.js diff --git a/src/containers/PlaylistManagerPanel.tsx b/src/containers/PlaylistManagerPanel.tsx index 199e2603e..08ad0bb30 100644 --- a/src/containers/PlaylistManagerPanel.tsx +++ b/src/containers/PlaylistManagerPanel.tsx @@ -1,10 +1,5 @@ import React from 'react'; import { useSelector, useDispatch } from '../hooks/useRedux'; -import { - filteredSelectedPlaylistItemsSelector, - isSelectedPlaylistLoadingSelector, - playlistItemFilterSelector, -} from '../selectors/playlistSelectors'; import { cannotDeleteActivePlaylist, } from '../actions/PlaylistActionCreators'; @@ -19,6 +14,8 @@ import { type PlaylistItem, type InsertTarget, selectedPlaylistSelector, + filteredSelectedPlaylistItemsSelector, + playlistItemFilterSelector, setPlaylistFilter, } from '../reducers/playlists'; @@ -31,9 +28,9 @@ function PlaylistPanelContainer() { } const playlistItems = useSelector(filteredSelectedPlaylistItemsSelector)!; - const loading = useSelector(isSelectedPlaylistLoadingSelector); const filter = useSelector(playlistItemFilterSelector); const dispatch = useDispatch(); + const loading = playlist.loading ?? false; const playlistID = playlist._id; const onShufflePlaylist = useCallback(async () => { diff --git a/src/mobile/containers/MainView.jsx b/src/mobile/containers/MainView.jsx index d3164fe3a..d4f3d090f 100644 --- a/src/mobile/containers/MainView.jsx +++ b/src/mobile/containers/MainView.jsx @@ -2,12 +2,12 @@ import React from 'react'; import { useSelector, useDispatch } from '../../hooks/useRedux'; import { toggleOverlay } from '../../reducers/activeOverlay'; import { setVideoEnabled, videoEnabledSelector } from '../../reducers/settings'; +import { playlistsSelector } from '../../reducers/playlists'; import { mediaSelector, startTimeSelector } from '../../selectors/boothSelectors'; import { sizeSelector as waitlistSizeSelector, positionSelector as waitlistPositionSelector, } from '../../selectors/waitlistSelectors'; -import { playlistsSelector } from '../../selectors/playlistSelectors'; import { openDrawer, openUsersDrawer } from '../actions/DrawerActionCreators'; import MainView from '../components/MainView'; diff --git a/src/mobile/containers/PlaylistManager.tsx b/src/mobile/containers/PlaylistManager.tsx index 1eefcce9a..94e39c32b 100644 --- a/src/mobile/containers/PlaylistManager.tsx +++ b/src/mobile/containers/PlaylistManager.tsx @@ -1,11 +1,11 @@ import React from 'react'; import { useDispatch, useSelector } from '../../hooks/useRedux'; -import { - filteredSelectedPlaylistItemsSelector, -} from '../../selectors/playlistSelectors'; import createLazyOverlay from '../../components/LazyOverlay'; import { closeOverlay } from '../../reducers/activeOverlay'; -import { selectedPlaylistSelector } from '../../reducers/playlists'; +import { + selectedPlaylistSelector, + filteredSelectedPlaylistItemsSelector, +} from '../../reducers/playlists'; const { useCallback, diff --git a/src/mobile/containers/PlaylistPanel.js b/src/mobile/containers/PlaylistPanel.js index 416f4eab5..cd5159650 100644 --- a/src/mobile/containers/PlaylistPanel.js +++ b/src/mobile/containers/PlaylistPanel.js @@ -1,11 +1,6 @@ import { bindActionCreators } from 'redux'; import { createStructuredSelector } from 'reselect'; import { connect } from 'react-redux'; -import { - filteredSelectedPlaylistItemsSelector, - isSelectedPlaylistLoadingSelector, - isFilteredSelector, -} from '../../selectors/playlistSelectors'; import { cannotDeleteActivePlaylist } from '../../actions/PlaylistActionCreators'; import PlaylistPanel from '../components/PlaylistManager/PlaylistPanel'; import { @@ -15,15 +10,16 @@ import { renamePlaylist, activatePlaylist, movePlaylistItems, - selectedPlaylistSelector, setPlaylistFilter, + selectedPlaylistSelector, + filteredSelectedPlaylistItemsSelector, + playlistItemFilterSelector, } from '../../reducers/playlists'; const mapStateToProps = createStructuredSelector({ playlist: selectedPlaylistSelector, media: filteredSelectedPlaylistItemsSelector, - loading: isSelectedPlaylistLoadingSelector, - isFiltered: isFilteredSelector, + filter: playlistItemFilterSelector, }); const onMoveMedia = (playlistID) => (media, opts) => ( diff --git a/src/reducers/__tests__/playlists.js b/src/reducers/__tests__/playlists.js index 5a6145b43..b7d59f4ad 100644 --- a/src/reducers/__tests__/playlists.js +++ b/src/reducers/__tests__/playlists.js @@ -1,13 +1,6 @@ import createStore from '../../redux/configureStore'; import { favoriteMediaComplete } from '../../actions/VoteActionCreators'; -import * as s from '../../selectors/playlistSelectors'; -import { - activatePlaylist, - loadPlaylist, - addPlaylistItems, - movePlaylistItems, - selectPlaylist, -} from '../playlists'; +import * as p from '../playlists'; function preloadPlaylists(playlists) { return { @@ -36,7 +29,7 @@ const initialisePlaylist = (dispatch) => { { _id: 'BeevKCM1NnNeW91leyLZu', artist: 'tricot', title: '99.974°C' }, ]; const playlistID = 'ZcU_8-UyI10Tx79R4CjRv'; - dispatch(loadPlaylist.fulfilled({ + dispatch(p.loadPlaylist.fulfilled({ items, page: 0, pageSize: 5, @@ -44,35 +37,35 @@ const initialisePlaylist = (dispatch) => { }, 'tviXX4Jyv0SUTosPCddWA', { playlistID, })); - dispatch(selectPlaylist(playlistID)); + dispatch(p.selectPlaylist(playlistID)); return { items, playlistID }; }; describe('reducers/playlists', () => { it('should not respond to unrelated actions', () => { const { dispatch, getState } = createStore(); - expect(s.playlistsSelector(getState())).toEqual([]); + expect(p.playlistsSelector(getState())).toEqual([]); dispatch({ type: 'randomOtherAction', payload: {} }); - expect(s.playlistsSelector(getState())).toEqual([]); + expect(p.playlistsSelector(getState())).toEqual([]); }); describe('action: playlists/SELECT_PLAYLIST', () => { it('sets the given playlist as the current playlist', () => { const { dispatch, getState } = createStore(preloadPlaylists(testPlaylists)); - expect(s.selectedPlaylistIDSelector(getState())).toBeNull(); + expect(p.selectedPlaylistIDSelector(getState())).toBeNull(); - dispatch(selectPlaylist('ZcU_8-UyI10Tx79R4CjRv')); - expect(s.selectedPlaylistIDSelector(getState())).toBe('ZcU_8-UyI10Tx79R4CjRv'); - expect(s.selectedPlaylistSelector(getState())).toHaveProperty('_id', 'ZcU_8-UyI10Tx79R4CjRv'); + dispatch(p.selectPlaylist('ZcU_8-UyI10Tx79R4CjRv')); + expect(p.selectedPlaylistIDSelector(getState())).toBe('ZcU_8-UyI10Tx79R4CjRv'); + expect(p.selectedPlaylistSelector(getState())).toHaveProperty('_id', 'ZcU_8-UyI10Tx79R4CjRv'); - dispatch(selectPlaylist('Kzy3kUckOgAV7iwrekHSE')); - expect(s.selectedPlaylistIDSelector(getState())).toBe('Kzy3kUckOgAV7iwrekHSE'); - expect(s.selectedPlaylistSelector(getState())).toHaveProperty('_id', 'Kzy3kUckOgAV7iwrekHSE'); + dispatch(p.selectPlaylist('Kzy3kUckOgAV7iwrekHSE')); + expect(p.selectedPlaylistIDSelector(getState())).toBe('Kzy3kUckOgAV7iwrekHSE'); + expect(p.selectedPlaylistSelector(getState())).toHaveProperty('_id', 'Kzy3kUckOgAV7iwrekHSE'); - dispatch(selectPlaylist(null)); - expect(s.selectedPlaylistIDSelector(getState())).toBeNull(); - expect(s.selectedPlaylistSelector(getState())).toBeNull(); + dispatch(p.selectPlaylist(null)); + expect(p.selectedPlaylistIDSelector(getState())).toBeNull(); + expect(p.selectedPlaylistSelector(getState())).toBeNull(); }); }); @@ -81,9 +74,9 @@ describe('reducers/playlists', () => { const { dispatch, getState } = createStore(preloadPlaylists(testPlaylists)); dispatch(initialisePlaylist); - expect(s.selectedPlaylistSelector(getState()).media).toHaveLength(5); + expect(p.selectedPlaylistItemsSelector(getState())).toHaveLength(5); - dispatch(addPlaylistItems.fulfilled({ + dispatch(p.addPlaylistItems.fulfilled({ playlistSize: 7, items: [ { _id: 'VlaaQAxk_Qy5orK1Vcr2C', artist: 'The Microphones', title: 'I Want Wind To Blow' }, @@ -95,7 +88,7 @@ describe('reducers/playlists', () => { items: [], })); - const { media } = s.selectedPlaylistSelector(getState()); + const media = p.selectedPlaylistItemsSelector(getState()); expect(media).toHaveLength(7); expect(media.map((playlistItem) => playlistItem._id)).toEqual([ 'Cnun9zo6oNr1wMCFRhnaO', 'PD_n42XxNCdQjDy5VB_SE', '66Z6y6JA4m5WmmNF3O7Ii', 'NkwUIwNmraSZ4A4eiC3GQ', 'VlaaQAxk_Qy5orK1Vcr2C', 'T9sdCu_-3o70qWk0YeDxJ', 'BeevKCM1NnNeW91leyLZu', @@ -106,12 +99,12 @@ describe('reducers/playlists', () => { const { dispatch, getState } = createStore(preloadPlaylists(testPlaylists)); const { playlistID } = dispatch(initialisePlaylist); // Test an active, but not selected playlist. - dispatch(selectPlaylist(null)); - dispatch(activatePlaylist.fulfilled(null, '', playlistID)); + dispatch(p.selectPlaylist(null)); + dispatch(p.activatePlaylist.fulfilled(null, '', playlistID)); - expect(s.activePlaylistSelector(getState()).media).toHaveLength(5); + expect(p.activePlaylistItemsSelector(getState())).toHaveLength(5); - dispatch(addPlaylistItems.fulfilled({ + dispatch(p.addPlaylistItems.fulfilled({ playlistSize: 7, items: [ { _id: 'VlaaQAxk_Qy5orK1Vcr2C', artist: 'The Microphones', title: 'I Want Wind To Blow' }, @@ -123,15 +116,15 @@ describe('reducers/playlists', () => { items: [], })); - expect(s.activePlaylistSelector(getState()).media).toHaveLength(7); - expect(s.nextMediaSelector(getState())._id).toBe('VlaaQAxk_Qy5orK1Vcr2C'); + expect(p.activePlaylistItemsSelector(getState())).toHaveLength(7); + expect(p.nextMediaSelector(getState())._id).toBe('VlaaQAxk_Qy5orK1Vcr2C'); }); it('appends favourited items to the end of the playlist', () => { const { dispatch, getState } = createStore(preloadPlaylists(testPlaylists)); const { playlistID } = dispatch(initialisePlaylist); - expect(s.selectedPlaylistSelector(getState()).media).toHaveLength(5); + expect(p.selectedPlaylistItemsSelector(getState())).toHaveLength(5); dispatch(favoriteMediaComplete(playlistID, 'vGA5mxhJpYkrsHSfxPcqX', { playlistSize: 6, @@ -140,8 +133,8 @@ describe('reducers/playlists', () => { ], })); - const { size, media } = s.selectedPlaylistSelector(getState()); - expect(size).toBe(6); + expect(p.selectedPlaylistSelector(getState())).toHaveProperty('size', 6); + const media = p.selectedPlaylistItemsSelector(getState()); expect(media).toHaveLength(6); expect(media[5]._id).toBe('RDeVBExmCGXvmT0mN0P3n'); }); @@ -152,16 +145,16 @@ describe('reducers/playlists', () => { const { dispatch, getState } = createStore(preloadPlaylists(testPlaylists)); const { items } = dispatch(initialisePlaylist); - expect(s.selectedPlaylistSelector(getState()).media).toHaveLength(5); + expect(p.selectedPlaylistItemsSelector(getState())).toHaveLength(5); - dispatch(movePlaylistItems.fulfilled({ + dispatch(p.movePlaylistItems.fulfilled({ location: { after: 'NkwUIwNmraSZ4A4eiC3GQ' }, }, '', { playlistID: 'ZcU_8-UyI10Tx79R4CjRv', medias: [items[1], items[2]], })); - const selectedItemIDs = s.selectedPlaylistSelector(getState()).media + const selectedItemIDs = p.selectedPlaylistItemsSelector(getState()) .map((playlistItem) => playlistItem._id); expect(selectedItemIDs).toEqual(['Cnun9zo6oNr1wMCFRhnaO', 'NkwUIwNmraSZ4A4eiC3GQ', 'PD_n42XxNCdQjDy5VB_SE', '66Z6y6JA4m5WmmNF3O7Ii', 'BeevKCM1NnNeW91leyLZu']); }); @@ -175,7 +168,7 @@ describe('reducers/playlists', () => { { _id: 'NkwUIwNmraSZ4A4eiC3GQ', artist: 'Angel Haze', title: 'A Tribe Called Red' }, { _id: 'BeevKCM1NnNeW91leyLZu', artist: 'tricot', title: '99.974°C' }, ]; - dispatch(loadPlaylist.fulfilled({ + dispatch(p.loadPlaylist.fulfilled({ items, page: 0, pageSize: 5, @@ -183,11 +176,11 @@ describe('reducers/playlists', () => { }, 'W34UluniSyaY6UB3Xz1jy', { playlistID: 'ZcU_8-UyI10Tx79R4CjRv', })); - dispatch(selectPlaylist('ZcU_8-UyI10Tx79R4CjRv')); + dispatch(p.selectPlaylist('ZcU_8-UyI10Tx79R4CjRv')); - expect(s.selectedPlaylistSelector(getState()).media).toHaveLength(5); + expect(p.selectedPlaylistItemsSelector(getState())).toHaveLength(5); - dispatch(movePlaylistItems.fulfilled({ + dispatch(p.movePlaylistItems.fulfilled({ location: { after: 'NkwUIwNmraSZ4A4eiC3GQ' }, }, '', { playlistID: 'ZcU_8-UyI10Tx79R4CjRv', @@ -195,7 +188,7 @@ describe('reducers/playlists', () => { })); const getID = (item) => (item ? item._id : null); - expect(s.selectedPlaylistSelector(getState()).media.map(getID)).toEqual([ + expect(p.selectedPlaylistItemsSelector(getState()).map(getID)).toEqual([ 'PD_n42XxNCdQjDy5VB_SE', null, 'NkwUIwNmraSZ4A4eiC3GQ', 'Cnun9zo6oNr1wMCFRhnaO', 'BeevKCM1NnNeW91leyLZu', ]); }); diff --git a/src/reducers/playlists.ts b/src/reducers/playlists.ts index a488e92b5..e6ea21100 100644 --- a/src/reducers/playlists.ts +++ b/src/reducers/playlists.ts @@ -430,23 +430,21 @@ const slice = createSlice({ builder .addCase(initState.fulfilled, (state, { payload }) => { if (payload.playlists) { - state.playlists = indexBy(payload.playlists.map((playlist: Playlist) => ({ - ...playlist, - active: playlist._id === payload.activePlaylist, - })), '_id'); + state.playlists = indexBy(payload.playlists, '_id'); // Preload the first item in the active playlist so it can be shown in // the footer bar immediately. Else it would flash "This playlist is empty" // for a moment. - if (payload.activePlaylist && payload.firstActivePlaylistItem) { - const item = { - ...payload.firstActivePlaylistItem.media, - ...payload.firstActivePlaylistItem, - }; + if (payload.activePlaylist) { const activePlaylist = state.playlists[payload.activePlaylist]; // Probably overly defensive, but avoid a crash if we got inconsistent data const size = activePlaylist ? activePlaylist.size : 1; state.playlistItems[payload.activePlaylist] ??= Array(size).fill(null); - state.playlistItems[payload.activePlaylist]![0] = item; + if (payload.firstActivePlaylistItem) { + state.playlistItems[payload.activePlaylist]![0] = { + ...payload.firstActivePlaylistItem.media, + ...payload.firstActivePlaylistItem, + }; + } } state.activePlaylistID = payload.activePlaylist; // Select the first playlist by default if there is no active playlist. @@ -476,7 +474,6 @@ const slice = createSlice({ const playlist = state.playlists[action.meta.arg]; if (playlist != null) { playlist.loading = false; - playlist.active = true; state.activePlaylistID = action.meta.arg; } }) @@ -704,8 +701,29 @@ const slice = createSlice({ }, selectors: { playlistsByID: (state) => state.playlists, - playlists: (state) => Object.values(state.playlists).sort(byName), - playlist: (state, id: string) => state.playlists[id] ?? null, + playlists: (state): Playlist[] => { + return Object.keys(state.playlists) + .map((id) => slice.getSelectors().playlist(state, id)!) + .sort(byName); + }, + playlist: (state, id: string) => { + const playlist = state.playlists[id] ?? null; + if (playlist != null && playlist._id === state.activePlaylistID) { + return { ...playlist, active: true }; + } + return playlist; + }, + playlistItems: (state, id: string) => { + const playlist = state.playlists[id]; + const playlistItems = state.playlistItems[id]; + if (playlistItems) { + return playlistItems; + } + if (playlist) { + return Array(playlist.size).fill(null) as PlaylistItemList; + } + return null; + }, activePlaylistID: (state) => state.activePlaylistID, selectedPlaylistID: (state) => state.selectedPlaylistID, activePlaylist: (state): Playlist | null => ( @@ -718,25 +736,31 @@ const slice = createSlice({ ? slice.getSelectors().playlist(state, state.selectedPlaylistID) : null ), - // FIXME should be null if it doesn't exist - activePlaylistItems: (state): PlaylistItemList => { - const { playlistItems, activePlaylistID } = state; - if (activePlaylistID && activePlaylistID in playlistItems) { - return playlistItems[activePlaylistID] ?? []; - } - return []; + activePlaylistItems: (state): PlaylistItemList | null => { + const { activePlaylistID } = state; + return typeof activePlaylistID === 'string' + ? slice.getSelectors().playlistItems(state, activePlaylistID) + : null; + }, + selectedPlaylistItems: (state) : PlaylistItemList | null => { + const { selectedPlaylistID } = state; + return typeof selectedPlaylistID === 'string' + ? slice.getSelectors().playlistItems(state, selectedPlaylistID) + : null; + }, + playlistItemFilter: (state) => { + return state.currentFilter?.filter; }, - // FIXME should be null if it doesn't exist - selectedPlaylistItems: (state): PlaylistItemList => { - const { playlistItems, selectedPlaylistID } = state; - if (typeof selectedPlaylistID === 'string' && selectedPlaylistID in playlistItems) { - return playlistItems[selectedPlaylistID] ?? []; + filteredSelectedPlaylistItems: (state) : PlaylistItemList | null => { + const { selectedPlaylistID, currentFilter } = state; + if (typeof selectedPlaylistID !== 'string') { + return null; } - return []; + return currentFilter?.items ?? slice.getSelectors().playlistItems(state, selectedPlaylistID); }, nextMedia: (state): PlaylistItem | null => { const s = slice.getSelectors(); - return s.activePlaylistItems(state)[0] ?? null; + return s.activePlaylistItems(state)?.[0] ?? null; }, }, }); @@ -770,6 +794,8 @@ export const { selectedPlaylist: selectedPlaylistSelector, activePlaylistItems: activePlaylistItemsSelector, selectedPlaylistItems: selectedPlaylistItemsSelector, + playlistItemFilter: playlistItemFilterSelector, + filteredSelectedPlaylistItems: filteredSelectedPlaylistItemsSelector, nextMedia: nextMediaSelector, } = slice.selectors; export default slice.reducer; diff --git a/src/selectors/playlistSelectors.js b/src/selectors/playlistSelectors.js deleted file mode 100644 index 242601881..000000000 --- a/src/selectors/playlistSelectors.js +++ /dev/null @@ -1,131 +0,0 @@ -import { createSelector } from 'reselect'; -import naturalCmp from 'natural-compare'; - -const byName = (a, b) => naturalCmp(a.name.toLowerCase(), b.name.toLowerCase()); - -/** @param {import('../redux/configureStore').StoreState} state */ -const baseSelector = (state) => state.playlists; - -export const playlistsByIDSelector = createSelector( - baseSelector, - (playlists) => playlists.playlists, -); - -export const playlistsSelector = createSelector( - playlistsByIDSelector, - (playlists) => Object.values(playlists).sort(byName), -); - -export const playlistItemsSelector = createSelector( - baseSelector, - (playlists) => playlists.playlistItems, -); - -export const activePlaylistIDSelector = createSelector( - baseSelector, - (playlists) => playlists.activePlaylistID, -); - -const activeMediaSelector = createSelector( - playlistItemsSelector, - activePlaylistIDSelector, - (playlistItems, activePlaylist) => { - if (activePlaylist && activePlaylist in playlistItems) { - return playlistItems[activePlaylist]; - } - return []; - }, -); - -function mergePlaylistItems(playlist, playlistItems) { - if (playlist) { - return { - ...playlist, - media: playlistItems, - }; - } - return null; -} - -export const activePlaylistSelector = createSelector( - baseSelector, - activePlaylistIDSelector, - activeMediaSelector, - (playlists, activeID, activeMedia) => ( - mergePlaylistItems(playlists.playlists[activeID], activeMedia) - ), -); - -export const selectedPlaylistIDSelector = createSelector( - baseSelector, - (playlists) => playlists.selectedPlaylistID, -); - -const selectedMediaSelector = createSelector( - playlistItemsSelector, - selectedPlaylistIDSelector, - (playlistItems, selectedPlaylist) => { - if (selectedPlaylist && selectedPlaylist in playlistItems) { - return playlistItems[selectedPlaylist]; - } - return []; - }, -); - -const filterSelector = createSelector( - baseSelector, - (base) => base.currentFilter, -); - -const currentFilterSelector = createSelector( - filterSelector, - selectedPlaylistIDSelector, - (filter, selectedID) => { - if (filter && filter.playlistID === selectedID) { - return filter; - } - return null; - }, -); - -export const playlistItemFilterSelector = createSelector( - currentFilterSelector, - (filter) => filter && filter.filter, -); - -export const filteredSelectedPlaylistItemsSelector = createSelector( - selectedMediaSelector, - currentFilterSelector, - (selectedItems, filter) => { - if (filter) { - return filter.items; - } - return selectedItems; - }, -); - -export const selectedPlaylistSelector = createSelector( - baseSelector, - selectedPlaylistIDSelector, - selectedMediaSelector, - (playlists, selectedID, selectedMedia) => ( - typeof selectedID === 'string' - ? mergePlaylistItems(playlists.playlists[selectedID], selectedMedia) - : null - ), -); - -export const nextMediaSelector = createSelector( - activeMediaSelector, - (media) => (media ? media[0] : null), -); - -export const isSelectedPlaylistLoadingSelector = createSelector( - selectedPlaylistSelector, - (selectedPlaylist) => Boolean(selectedPlaylist.loading), -); - -export const isFilteredSelector = createSelector( - playlistItemFilterSelector, - (filter) => Boolean(filter), -);