diff --git a/src/firefly/js/charts/ChartsCntlr.js b/src/firefly/js/charts/ChartsCntlr.js index 8ee7007377..65c30a8030 100644 --- a/src/firefly/js/charts/ChartsCntlr.js +++ b/src/firefly/js/charts/ChartsCntlr.js @@ -2,7 +2,7 @@ * License information at https://github.com/Caltech-IPAC/firefly/blob/master/License.txt */ -import {cloneDeep, has, get, isArray, isEmpty, isString, isUndefined, omit, omitBy, set, range, unset, uniqueId} from 'lodash'; +import {cloneDeep, get, isArray, isEmpty, isString, isUndefined, omit, omitBy, set, range, unset, uniqueId} from 'lodash'; import shallowequal from 'shallowequal'; import {flux} from '../core/ReduxFlux.js'; @@ -700,35 +700,36 @@ function reduceData(state={}, action={}) { { const {chartId, chartType, mounted, ...rest} = action.payload; // if a chart is replaced (added with the same id) mounted should not change - const nMounted = isUndefined(mounted) ? get(state, [chartId, 'mounted']) : mounted; + const nMounted = isUndefined(mounted) ? state[chartId]?.mounted : mounted; // save the original payload, so that the chart could be recreated rest['_original'] = cloneDeep(action.payload); applyDefaults(rest); useScatterGL && changeToScatterGL(rest); // the first trace is put as the last curve for plotly rendering - const tData = get(rest, ['data', 'length']); + const tData = rest.data?.length; if (tData) { Object.assign(rest, {activeTrace: tData-1, curveNumberMap: range(tData)}); } - state = updateSet(state, chartId, - omitBy({ + state = {...state, + [chartId]: omitBy({ chartId, chartType, mounted: nMounted, ...rest - }, isUndefined)); + }, isUndefined)}; logger.info(`CHART_ADD ${chartId} #mounted ${nMounted}`); return state; } case (CHART_UPDATE) : { const {chartId, changes} = action.payload; - let chartData = getChartData(chartId); + let chartData = state[chartId] ?? {}; chartData = updateObject(chartData, changes); useScatterGL && changeToScatterGL(chartData); - return updateSet(state, chartId, chartData); + // updateObject returns the same object when nothing changed; keep state too, so subscribers don't re-render + return chartData === state[chartId] ? state : {...state, [chartId]: chartData}; } case (CHART_TRACE_REMOVE) : { @@ -743,19 +744,19 @@ function reduceData(state={}, action={}) { chartData = updateObject(chartData, moreChanges); } } - return updateSet(state, chartId, chartData); + return chartData === state[chartId] ? state : {...state, [chartId]: chartData}; } case (CHART_REMOVE) : { const {chartId} = action.payload; logger.info('CHART_REMOVE '+chartId); - return omit(state, chartId); + return omit(state, [chartId]); } case (CHART_MOUNTED) : { const {chartId} = action.payload; - const n = get(state, [chartId,'mounted'], 0); - state = updateSet(state, [chartId,'mounted'], Number(n) + 1); + const mountedCnt = state[chartId]?.mounted ?? 0; + state = {...state, [chartId]: {...state[chartId], mounted: Number(mountedCnt) + 1}}; logger.info(`CHART_MOUNTED ${chartId} #mounted ${state[chartId].mounted}`); return state; @@ -763,10 +764,10 @@ function reduceData(state={}, action={}) { case (CHART_UNMOUNTED) : { const {chartId} = action.payload; - if (has(state, chartId)) { - const n = get(state, [chartId,'mounted'], 0); - if (n > 0) { - state = updateSet(state, [chartId, 'mounted'], Number(n) - 1); + if (state[chartId]) { + const mountedCnt = state[chartId].mounted ?? 0; + if (mountedCnt > 0) { + state = {...state, [chartId]: {...state[chartId], mounted: Number(mountedCnt) - 1}}; } else { logger.error(`CHART_UNMOUNT on unmounted chartId ${chartId}`); } @@ -934,7 +935,7 @@ function removeTrace({chartId, traceNum}) { } export function getChartData(chartId, defaultChartData={}) { - return get(flux.getState(), [CHART_SPACE_PATH, 'data', chartId], defaultChartData); + return flux.getState()[CHART_SPACE_PATH]?.data?.[chartId] ?? defaultChartData; } @@ -1003,7 +1004,7 @@ export function getExpandedChartProps() { export function getChartIdsForTable(tbl_id) { const chartIds = []; - const state = get(flux.getState(), [CHART_SPACE_PATH, 'data']); + const state = flux.getState()[CHART_SPACE_PATH]?.data ?? {}; Object.keys(state).forEach((cid) => { if (state[cid].tbl_id === tbl_id) { chartIds.push(cid); @@ -1014,7 +1015,7 @@ export function getChartIdsForTable(tbl_id) { export function getChartIdsInGroup(groupId) { const chartIds = []; - const state = get(flux.getState(), [CHART_SPACE_PATH, 'data']); + const state = flux.getState()[CHART_SPACE_PATH]?.data ?? {}; Object.keys(state).forEach((cid) => { if (state[cid].groupId === groupId) { chartIds.push(cid); @@ -1024,7 +1025,7 @@ export function getChartIdsInGroup(groupId) { } export function removeChartsInGroup(groupId) { - const chartData = get(flux.getState(), [CHART_SPACE_PATH, 'data']); + const chartData = flux.getState()[CHART_SPACE_PATH]?.data ?? {}; Object.values(chartData) .filter( (v) => !groupId || v.groupId === groupId) .forEach( (v) => dispatchChartRemove(v.chartId)); diff --git a/src/firefly/js/charts/TableStatsCntlr.js b/src/firefly/js/charts/TableStatsCntlr.js index edd0b5523b..70f560c884 100644 --- a/src/firefly/js/charts/TableStatsCntlr.js +++ b/src/firefly/js/charts/TableStatsCntlr.js @@ -1,9 +1,8 @@ import {useEffect} from 'react'; import {flux} from '../core/ReduxFlux.js'; -import {get, has, omit} from 'lodash'; +import {omit} from 'lodash'; -import {updateSet, updateMerge} from '../util/WebUtil.js'; import ColValuesStatistics from './ColValuesStatistics.js'; import {REINIT_APP} from '../core/CoreConst'; @@ -54,11 +53,11 @@ function reducers() { * @param {function} dispatcher only for special dispatching uses such as remote */ export function dispatchLoadTblStats(searchRequest, dispatcher= flux.process) { - const tbl_id = get(searchRequest, 'tbl_id'); + const tbl_id = searchRequest?.tbl_id; if (!tbl_id) return; // use resultSetID to determine if a call needs to be placed - const {resultSetID, numericColCnt} = get(flux.getState(), [TBLSTATS_DATA_KEY, tbl_id], {}); - const resultSetIDNow = get(getTblById(tbl_id), 'tableMeta.resultSetID'); + const {resultSetID, numericColCnt} = flux.getState()[TBLSTATS_DATA_KEY]?.[tbl_id] ?? {}; + const resultSetIDNow = getTblById(tbl_id)?.tableMeta?.resultSetID; const curNumericColCnt = getColumns(getTblById(tbl_id), COL_TYPE.NUMBER)?.length; if (resultSetID !== resultSetIDNow @@ -103,24 +102,24 @@ export function reducer(state=getInitState(), action={}) { case (TablesCntlr.TABLE_REMOVE) : { const {tbl_id} = action.payload; - if (has(state, tbl_id)) { - return Object.assign({}, omit(state,[tbl_id])); + if (state[tbl_id]) { + return omit(state, [tbl_id]); } return state; } case (LOAD_TBL_STATS) : { - const tblId = get(action.payload, ['searchRequest', 'tbl_id']); + const tblId = action.payload.searchRequest?.tbl_id; // save original table file path - const resultSetID = get(getTblById(tblId), 'tableMeta.resultSetID'); + const resultSetID = getTblById(tblId)?.tableMeta?.resultSetID; - return updateSet(state, tblId, {resultSetID, isColStatsReady: false}); + return {...state, [tblId]: {resultSetID, isColStatsReady: false}}; } case (UPDATE_TBL_STATS) : { const {tblId, isColStatsReady, colStats} = action.payload; - return updateMerge(state, tblId, {isColStatsReady, colStats}); + return {...state, [tblId]: {...state[tblId], isColStatsReady, colStats}}; } case (REINIT_APP) : return getInitState(); @@ -149,7 +148,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) { const {tbl_id} = activeTableServerRequest; - const resultSetIDOnSubmit = get(getTblById(tbl_id), 'tableMeta.resultSetID'); + const resultSetIDOnSubmit = getTblById(tbl_id)?.tableMeta?.resultSetID; // searchRequest const sreq = cloneRequest(activeTableServerRequest, {'startIdx': 0, 'pageSize': MAX_ROW}); @@ -162,15 +161,15 @@ function fetchTblStats(dispatch, activeTableServerRequest) { (tableModel) => { // if the original table has changed no need to continue - const resultSetIDOnFetch = get(getTblById(tbl_id), 'tableMeta.resultSetID'); + const resultSetIDOnFetch = getTblById(tbl_id)?.tableMeta?.resultSetID; if (resultSetIDOnSubmit !== resultSetIDOnFetch) { return; } - const tblData = get(tableModel, 'tableData.data'); + const tblData = tableModel?.tableData?.data; let colStats = []; if (tblData) { - const columns = get(getTblById(tbl_id), 'tableData.columns', []); + const columns = getTblById(tbl_id)?.tableData?.columns ?? []; colStats = tblData.reduce((colstats, arow) => { const r = new ColValuesStatistics(...arow); const col = columns.find((c)=>{return c.name=== r.name;}); @@ -206,7 +205,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) { ).finally(() => { pendingStatsTblIds.delete(tbl_id); const currentRequest = getTblById(tbl_id)?.request; - const currentResultSetID = get(getTblById(tbl_id), 'tableMeta.resultSetID'); + const currentResultSetID = getTblById(tbl_id)?.tableMeta?.resultSetID; if (currentRequest && currentResultSetID !== resultSetIDOnSubmit) { dispatchLoadTblStats(currentRequest); } @@ -216,7 +215,7 @@ function fetchTblStats(dispatch, activeTableServerRequest) { export function getColValStats(tblId) { if (!tblId) { return undefined; } const tblStatsData = flux.getState()[TBLSTATS_DATA_KEY][tblId]; - if (get(tblStatsData,'isColStatsReady')) { + if (tblStatsData?.isColStatsReady) { return tblStatsData.colStats; } } diff --git a/src/firefly/js/charts/__tests__/ChartsCntlr-test.js b/src/firefly/js/charts/__tests__/ChartsCntlr-test.js new file mode 100644 index 0000000000..d8ed5e4ca1 --- /dev/null +++ b/src/firefly/js/charts/__tests__/ChartsCntlr-test.js @@ -0,0 +1,63 @@ +/*eslint-env node, jest */ +import {flux} from 'firefly/core/ReduxFlux.js'; +import { + CHART_ADD, CHART_MOUNTED, CHART_REMOVE, CHART_TRACE_REMOVE, CHART_UNMOUNTED, CHART_UPDATE, reducer +} from 'firefly/charts/ChartsCntlr.js'; + +// default chart ids are 'default-' + tbl_id, and tbl_id can contain a period +const DOTTED_CHART_ID = 'default-2mass.catalog'; + +describe('ChartsCntlr reducer with a chart id containing a period', () => { + const addedState = reducer(undefined, + {type: CHART_ADD, payload: {chartId: DOTTED_CHART_ID, chartType: 'plot.ly', groupId: 'main'}}); + + test('CHART_ADD stores the chart under the whole id, not a nested path', () => { + expect(addedState.data[DOTTED_CHART_ID]?.chartId).toBe(DOTTED_CHART_ID); + expect(addedState.data['default-2mass']).toBeUndefined(); + }); + + test('CHART_MOUNTED and CHART_UNMOUNTED update the mounted count of that chart', () => { + const mountedState = reducer(addedState, {type: CHART_MOUNTED, payload: {chartId: DOTTED_CHART_ID}}); + expect(mountedState.data[DOTTED_CHART_ID].mounted).toBe(1); + + const unmountedState = reducer(mountedState, {type: CHART_UNMOUNTED, payload: {chartId: DOTTED_CHART_ID}}); + expect(unmountedState.data[DOTTED_CHART_ID].mounted).toBe(0); + }); + + test('CHART_REMOVE removes the chart', () => { + const removedState = reducer(addedState, {type: CHART_REMOVE, payload: {chartId: DOTTED_CHART_ID}}); + expect(removedState.data).toEqual({}); + }); + + test('CHART_UPDATE applies changes to that chart', () => { + const updatedState = reducer(addedState, + {type: CHART_UPDATE, payload: {chartId: DOTTED_CHART_ID, changes: {'layout.title.text': 'J vs H-K'}}}); + expect(updatedState.data[DOTTED_CHART_ID].layout.title.text).toBe('J vs H-K'); + expect(updatedState.data['default-2mass']).toBeUndefined(); + }); + + test('CHART_UPDATE returns the same state when changes do not change the chart', () => { + const chartType = addedState.data[DOTTED_CHART_ID].chartType; + const unchangedState = reducer(addedState, + {type: CHART_UPDATE, payload: {chartId: DOTTED_CHART_ID, changes: {chartType}}}); + expect(unchangedState.data).toBe(addedState.data); + }); +}); + +// CHART_TRACE_REMOVE reads the current chart from the store (removeTrace calls getChartData), not from the reducer state +describe('ChartsCntlr reducer with a chart id containing a period, reading chart data from the store', () => { + let storeChartState; + beforeEach(() => jest.spyOn(flux, 'getState').mockImplementation(() => ({charts: storeChartState}))); + afterEach(() => jest.restoreAllMocks()); + + test('CHART_TRACE_REMOVE removes the trace from that chart', () => { + const twoTraces = [{name: 'trace0', type: 'scatter'}, {name: 'trace1', type: 'scatter'}]; + storeChartState = reducer(undefined, + {type: CHART_ADD, payload: {chartId: DOTTED_CHART_ID, chartType: 'plot.ly', groupId: 'main', data: twoTraces}}); + + const traceRemovedState = reducer(storeChartState, + {type: CHART_TRACE_REMOVE, payload: {chartId: DOTTED_CHART_ID, traceNum: 0}}); + expect(traceRemovedState.data[DOTTED_CHART_ID].data.map((trace) => trace.name)).toEqual(['trace1']); + expect(traceRemovedState.data['default-2mass']).toBeUndefined(); + }); +}); diff --git a/src/firefly/js/charts/__tests__/TableStatsCntlr-test.js b/src/firefly/js/charts/__tests__/TableStatsCntlr-test.js new file mode 100644 index 0000000000..cea2f44238 --- /dev/null +++ b/src/firefly/js/charts/__tests__/TableStatsCntlr-test.js @@ -0,0 +1,21 @@ +/*eslint-env node, jest */ +import {reducer, UPDATE_TBL_STATS} from 'firefly/charts/TableStatsCntlr.js'; +import {TABLE_REMOVE} from 'firefly/tables/TablesCntlr.js'; + +const DOTTED_TBL_ID = '2mass.catalog'; + +describe('TableStatsCntlr reducer with a table id containing a period', () => { + const colStats = [{name: 'ra', min: 202.3, max: 202.7}]; + const updatedState = reducer({}, + {type: UPDATE_TBL_STATS, payload: {tblId: DOTTED_TBL_ID, isColStatsReady: true, colStats}}); + + test('UPDATE_TBL_STATS stores stats under the whole id, not a nested path', () => { + expect(updatedState[DOTTED_TBL_ID]).toEqual({isColStatsReady: true, colStats}); + expect(updatedState['2mass']).toBeUndefined(); + }); + + test('TABLE_REMOVE removes the stats of that table', () => { + const removedState = reducer(updatedState, {type: TABLE_REMOVE, payload: {tbl_id: DOTTED_TBL_ID}}); + expect(removedState).toEqual({}); + }); +}); diff --git a/src/firefly/js/charts/ui/PlotlyWrapper.jsx b/src/firefly/js/charts/ui/PlotlyWrapper.jsx index 23053696b0..3774d8efa4 100644 --- a/src/firefly/js/charts/ui/PlotlyWrapper.jsx +++ b/src/firefly/js/charts/ui/PlotlyWrapper.jsx @@ -91,7 +91,7 @@ function isSlowResize() { export async function downloadChart(chartId) { const Plotly= await getPlotLy(); // in API we can have same chart unexpanded and expanded - const chartDivAll = document.querySelectorAll(`#${chartId}`); + const chartDivAll = document.querySelectorAll(`#${CSS.escape(chartId)}`); if (chartId && chartDivAll && chartDivAll.length > 0) { const chartDiv = chartDivAll[chartDivAll.length-1]; showPlotLySaveDialog(Plotly,chartDiv);