From fe0badc4b3080d73f147faf45706af3c8b5d12ba Mon Sep 17 00:00:00 2001 From: Jordan Eldredge Date: Sat, 20 Feb 2021 22:19:31 -0800 Subject: [PATCH] Derive how windows hidden state is computed (#1068) This resolves a bug: 1. Put the Milkdrop window in Desktop mode 2. Put the Milkdrop window in Fullscreen mode 3. Press esc 4. Try to move the Milkdrop window (it won't) Keeping the "hidden" state up to date was fragile. By computing it we can avoid edge cases like the one above where we failed to reset the Milkdrop window's hidden state. --- packages/webamp/js/actionCreators/index.ts | 11 +-------- packages/webamp/js/actionCreators/windows.ts | 9 ------- packages/webamp/js/actionTypes.ts | 1 - packages/webamp/js/reducers/windows.ts | 25 ++++---------------- packages/webamp/js/selectors.ts | 15 +++++++++--- packages/webamp/js/serialization.test.ts | 7 ------ 6 files changed, 17 insertions(+), 51 deletions(-) diff --git a/packages/webamp/js/actionCreators/index.ts b/packages/webamp/js/actionCreators/index.ts index f8595e7c..81d79e2a 100644 --- a/packages/webamp/js/actionCreators/index.ts +++ b/packages/webamp/js/actionCreators/index.ts @@ -19,20 +19,13 @@ import { WINDOWS } from "../constants"; import { Thunk, Action, Slider } from "../types"; import { SerializedStateV1 } from "../serializedStates/v1Types"; import * as Selectors from "../selectors"; -import { - ensureWindowsAreOnScreen, - showWindow, - hideWindow, - setFocusedWindow, -} from "./windows"; +import { ensureWindowsAreOnScreen, setFocusedWindow } from "./windows"; export { toggleDoubleSizeMode, toggleEqualizerShadeMode, togglePlaylistShadeMode, closeWindow, - hideWindow, - showWindow, setWindowSize, toggleWindow, updateWindowPositions, @@ -186,10 +179,8 @@ export function loadDefaultSkin(): Action { export function toggleMilkdropDesktop(): Thunk { return (dispatch, getState) => { if (Selectors.getMilkdropDesktopEnabled(getState())) { - dispatch(showWindow(WINDOWS.MILKDROP)); dispatch({ type: SET_MILKDROP_DESKTOP, enabled: false }); } else { - dispatch(hideWindow(WINDOWS.MILKDROP)); dispatch({ type: SET_MILKDROP_DESKTOP, enabled: true }); } }; diff --git a/packages/webamp/js/actionCreators/windows.ts b/packages/webamp/js/actionCreators/windows.ts index ebdbe904..a45e9bdc 100644 --- a/packages/webamp/js/actionCreators/windows.ts +++ b/packages/webamp/js/actionCreators/windows.ts @@ -8,7 +8,6 @@ import { TOGGLE_WINDOW, CLOSE_WINDOW, TOGGLE_WINDOW_SHADE_MODE, - SET_WINDOW_VISIBILITY, BROWSER_WINDOW_SIZE_CHANGED, RESET_WINDOW_SIZES, TOGGLE_LLAMA_MODE, @@ -88,14 +87,6 @@ export function closeWindow(windowId: WindowId): Action { return { type: CLOSE_WINDOW, windowId }; } -export function hideWindow(windowId: WindowId): Action { - return { type: SET_WINDOW_VISIBILITY, windowId, hidden: true }; -} - -export function showWindow(windowId: WindowId): Action { - return { type: SET_WINDOW_VISIBILITY, windowId, hidden: false }; -} - export function setFocusedWindow(window: WindowId | null): Action { return { type: SET_FOCUSED_WINDOW, window }; } diff --git a/packages/webamp/js/actionTypes.ts b/packages/webamp/js/actionTypes.ts index 2e129a1a..a0fca6f3 100644 --- a/packages/webamp/js/actionTypes.ts +++ b/packages/webamp/js/actionTypes.ts @@ -66,7 +66,6 @@ export const LOADED = "LOADED"; export const SET_Z_INDEX = "SET_Z_INDEX"; export const DISABLE_MARQUEE = "DISABLE_MARQUEE"; export const SET_DUMMY_VIZ_DATA = "SET_DUMMY_VIZ_DATA"; -export const SET_WINDOW_VISIBILITY = "SET_WINDOW_VISIBILITY"; export const LOADING = "LOADING"; export const CLOSE_REQUESTED = "CLOSE_REQUESTED"; export const LOAD_SERIALIZED_STATE = "LOAD_SERIALIZED_STATE"; diff --git a/packages/webamp/js/reducers/windows.ts b/packages/webamp/js/reducers/windows.ts index bbae8913..2c5f0395 100644 --- a/packages/webamp/js/reducers/windows.ts +++ b/packages/webamp/js/reducers/windows.ts @@ -4,7 +4,6 @@ import { SET_FOCUSED_WINDOW, TOGGLE_WINDOW, CLOSE_WINDOW, - SET_WINDOW_VISIBILITY, UPDATE_WINDOW_POSITIONS, WINDOW_SIZE_CHANGED, TOGGLE_WINDOW_SHADE_MODE, @@ -26,7 +25,6 @@ export interface WebampWindow { title: string; size: [number, number]; open: boolean; - hidden: boolean; shade?: boolean; canResize: boolean; canShade: boolean; @@ -55,7 +53,6 @@ const defaultWindowsState: WindowsState = { title: "Main Window", size: [0, 0], open: true, - hidden: false, shade: false, canResize: false, canShade: true, @@ -67,7 +64,6 @@ const defaultWindowsState: WindowsState = { title: "Equalizer", size: [0, 0], open: true, - hidden: false, shade: false, canResize: false, canShade: true, @@ -79,7 +75,6 @@ const defaultWindowsState: WindowsState = { title: "Playlist Editor", size: [0, 0], open: true, - hidden: false, shade: false, canResize: true, canShade: true, @@ -111,7 +106,6 @@ const windows = ( title: "Milkdrop", size: [0, 0], open: action.open, - hidden: false, shade: false, canResize: true, canShade: false, @@ -155,8 +149,6 @@ const windows = ( [action.windowId]: { ...windowState, open: !windowState.open, - // Reset hidden state when opening window - hidden: windowState.open ? windowState.hidden : false, }, }, }; @@ -171,17 +163,6 @@ const windows = ( }, }, }; - case SET_WINDOW_VISIBILITY: - return { - ...state, - genWindows: { - ...state.genWindows, - [action.windowId]: { - ...state.genWindows[action.windowId], - hidden: action.hidden, - }, - }, - }; case WINDOW_SIZE_CHANGED: const { canResize } = state.genWindows[action.windowId]; if (!canResize) { @@ -235,7 +216,9 @@ const windows = ( if (serializedW == null) { return w; } - return { ...w, ...serializedW }; + // Pull out `hidden` since it's been removed from our state. + const { hidden, ...rest } = serializedW; + return { ...w, ...rest }; }), focused, }; @@ -260,7 +243,7 @@ export function getSerializedState( return { size: w.size, open: w.open, - hidden: w.hidden, + hidden: false, // Not used any more shade: w.shade || false, position: w.position, }; diff --git a/packages/webamp/js/selectors.ts b/packages/webamp/js/selectors.ts index 6cb0ee85..797072fc 100644 --- a/packages/webamp/js/selectors.ts +++ b/packages/webamp/js/selectors.ts @@ -211,9 +211,14 @@ export const getWindowOpen = createSelector(getGenWindows, (genWindows) => { return (windowId: WindowId) => genWindows[windowId].open; }); -export const getWindowHidden = createSelector(getGenWindows, (genWindows) => { - return (windowId: WindowId) => genWindows[windowId].hidden; -}); +export const getWindowHidden = createSelector( + getMilkdropWindowEnabled, + (milkdropWindowEnabled) => { + return (windowId: WindowId) => { + return windowId === WINDOWS.MILKDROP && !milkdropWindowEnabled; + }; + } +); export const getWindowShade = createSelector(getGenWindows, (genWindows) => { return (windowId: WindowId) => genWindows[windowId].shade; @@ -654,6 +659,10 @@ export function getMilkdropMessage(state: AppState): MilkdropMessage | null { return state.milkdrop.message; } +export function getMilkdropWindowEnabled(state: AppState): boolean { + return state.milkdrop.display === "WINDOW"; +} + export function getMilkdropDesktopEnabled(state: AppState): boolean { return state.milkdrop.display === "DESKTOP"; } diff --git a/packages/webamp/js/serialization.test.ts b/packages/webamp/js/serialization.test.ts index 60190187..af16d104 100644 --- a/packages/webamp/js/serialization.test.ts +++ b/packages/webamp/js/serialization.test.ts @@ -193,13 +193,6 @@ describe("can serialize", () => { expected: false, }); - testSerialization({ - name: "window hidden", - action: Actions.hideWindow("playlist"), - selector: (state) => Selectors.getWindowHidden(state)("playlist"), - expected: true, - }); - testSerialization({ name: "window shade", // @ts-ignore