Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/user colors #584

Merged
merged 11 commits into from
Oct 22, 2024
Merged

Feature/user colors #584

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
3247637
rename agentUiData and receiveAgentNamesAndStates to defaultUIData
interim17 Oct 2, 2024
0a98fdc
rename setColorChanges redux plumbing to applyUserColorSelections
interim17 Oct 2, 2024
7a4fb7c
reduce global testing threshold
interim17 Oct 2, 2024
3f97eb0
use correct names for defaultUIData in tests
interim17 Oct 2, 2024
5400f74
rename: applyUserColor in place of applyUserColorSelection
interim17 Oct 8, 2024
7e7df61
use applyUserColor in place of applyUserColorSelection
interim17 Oct 14, 2024
affe165
rename: getDefaultUIData -> getDefautlUIDisplayData
interim17 Oct 14, 2024
adbd51d
make string match variable name in apply user color constants
interim17 Oct 14, 2024
a6021ec
add user selected ui display data to redux, distinguished from default
interim17 Oct 14, 2024
b020612
Merge branch 'main' of https://github.com/simularium/simularium-websi…
interim17 Oct 14, 2024
c52e95d
Merge branch 'main' of https://github.com/simularium/simularium-websi…
interim17 Oct 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/containers/ViewerPanel/selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@ import {
getTimeStep,
getFirstFrameTimeOfCachedSimulation,
getSimulariumFile,
getDefaultUIDisplayData,
} from "../../state/trajectory/selectors";
import {
getAgentsToHide,
getCurrentTime,
getHighlightedAgents,
getSelectedUIDisplayData,
} from "../../state/selection/selectors";
import { AgentRenderingCheckboxMap } from "../../state/selection/types";
import { roundTimeForDisplay } from "../../util";
import { DisplayTimes } from "./types";
import { isNetworkSimFileInterface } from "../../state/trajectory/types";

export const getSelectionStateInfoForViewer = createSelector(
[getHighlightedAgents, getAgentsToHide, getDefaultUIDisplayData],
[getHighlightedAgents, getAgentsToHide, getSelectedUIDisplayData],
(highlightedAgents, hiddenAgents, appliedColors): SelectionStateInfo => ({
highlightedAgents,
hiddenAgents,
Expand Down
12 changes: 12 additions & 0 deletions src/state/selection/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { UIDisplayData } from "@aics/simularium-viewer";
import { AgentMetadata, ColorChange } from "../../constants/interfaces";
import {
SELECT_METADATA,
Expand All @@ -10,6 +11,7 @@ import {
SET_RECENT_COLORS,
SET_SELECTED_AGENT,
APPLY_USER_COLOR,
SET_SELECTED_DISPLAY_DATA,
} from "./constants";
import {
ChangeAgentsRenderingStateAction,
Expand All @@ -21,6 +23,7 @@ import {
ApplyUserColorAction,
SetRecentColorsAction,
SetSelectedAgentMetadataAction,
SetSelectedUIDisplayDataAction,
} from "./types";

export function changeTime(time: number): ChangeTimeAction {
Expand Down Expand Up @@ -103,3 +106,12 @@ export function setSelectedAgentMetadata(
type: SET_SELECTED_AGENT,
};
}

export function setSelectedUIDisplayData(
displayData: UIDisplayData
): SetSelectedUIDisplayDataAction {
return {
payload: displayData,
type: SET_SELECTED_DISPLAY_DATA,
};
}
3 changes: 3 additions & 0 deletions src/state/selection/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,6 @@ export const RESET_AGENT_SELECTIONS_AND_HIGHLIGHTS = makeSelectConstant(
export const APPLY_USER_COLOR = makeSelectConstant("apply-user-color");
export const SET_RECENT_COLORS = makeSelectConstant("set-recent-colors");
export const SET_SELECTED_AGENT = makeSelectConstant("set-selected-agent");
export const SET_SELECTED_DISPLAY_DATA = makeSelectConstant(
"set-selected-display-data"
);
8 changes: 4 additions & 4 deletions src/state/selection/logics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import { createLogic } from "redux-logic";
import { UIDisplayData } from "@aics/simularium-viewer";

import { ReduxLogicDeps } from "../types";
import { getDefaultUIDisplayData } from "../trajectory/selectors";
import { setDefaultUIData } from "../trajectory/actions";
import { APPLY_USER_COLOR } from "./constants";
import { getSelectedUIDisplayData } from "./selectors";
import { setSelectedUIDisplayData } from "./actions";

const storeColorsLogic = createLogic({
process(deps: ReduxLogicDeps, dispatch, done) {
const { action, getState } = deps;
const uiData: UIDisplayData = getDefaultUIDisplayData(getState());
const uiData: UIDisplayData = getSelectedUIDisplayData(getState());
const colorChange = action.payload;
const newUiData = uiData.map((agent) => {
const newAgent = { ...agent };
Expand All @@ -32,7 +32,7 @@ const storeColorsLogic = createLogic({
}
return newAgent;
});
dispatch(setDefaultUIData(newUiData));
dispatch(setSelectedUIDisplayData(newUiData));
done();
Comment on lines 34 to 36
Copy link
Contributor

@frasercl frasercl Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a bit bummed to see this synchronous state derivation code in a logic. Everything this code does would (I think) work just fine in a reducer branch, and would probably be more at home there, where it wouldn't have to re-dispatch a new action and immediately call done to clear out resources held by redux-logic for what it assumes to be an async operation. That change is well out of scope for this PR though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good opportunity for a refactor. This logic was put in place during previous work on color changing, and its role has changed.

We can derive the new UiDisplayData and pass it into redux directly, redux never needs to handle the ColorChange. I am inclined to do this in ModelPanel instead of in the reducer because the pattern in this repo is for reducers to simply pass on their action payload without side effects.

Adding (synchronous) logic to a reducer feels like breaking that pattern in a way that puts code in more places which is part of the pain of redux, for me at least. But I accept that this redux-logic is not the right place for it either.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrote a brief issue (#596 ) and will address in a separate PR.

},
type: APPLY_USER_COLOR,
Expand Down
18 changes: 18 additions & 0 deletions src/state/selection/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
RESET_AGENT_SELECTIONS_AND_HIGHLIGHTS,
SET_RECENT_COLORS,
SET_SELECTED_AGENT,
SET_SELECTED_DISPLAY_DATA,
} from "./constants";
import {
ChangeAgentsRenderingStateAction,
Expand All @@ -24,6 +25,7 @@ import {
ResetAction,
SetRecentColorsAction,
SetSelectedAgentMetadataAction,
SetSelectedUIDisplayDataAction,
} from "./types";

export const initialState = {
Expand All @@ -33,6 +35,7 @@ export const initialState = {
agentHighlightMap: {},
recentColors: [],
selectedAgentMetadata: {},
selectedUIDisplayData: [],
};

const actionToConfigMap: TypeToDescriptionMap = {
Expand Down Expand Up @@ -154,6 +157,21 @@ const actionToConfigMap: TypeToDescriptionMap = {
};
},
},
[SET_SELECTED_DISPLAY_DATA]: {
accepts: (
action: AnyAction
): action is SetSelectedUIDisplayDataAction =>
action.type === SET_SELECTED_DISPLAY_DATA,
perform: (
state: SelectionStateBranch,
action: SetSelectedUIDisplayDataAction
) => {
return {
...state,
selectedUIDisplayData: action.payload,
};
},
},
};

export default makeReducer<SelectionStateBranch>(
Expand Down
2 changes: 2 additions & 0 deletions src/state/selection/selectors/basic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ export const getNumberCollapsed = (state: State) =>
export const getRecentColors = (state: State) => state.selection.recentColors;
export const getSelectedAgentMetadata = (state: State) =>
state.selection.selectedAgentMetadata;
export const getSelectedUIDisplayData = (state: State) =>
state.selection.selectedUIDisplayData;
6 changes: 6 additions & 0 deletions src/state/selection/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { UIDisplayData } from "@aics/simularium-viewer";
import { AgentMetadata, ColorChange } from "../../constants/interfaces";
import { TrajectoryStateBranch } from "../trajectory/types";

Expand Down Expand Up @@ -81,3 +82,8 @@ export interface SetSelectedAgentMetadataAction {
payload: AgentMetadata;
type: string;
}

export interface SetSelectedUIDisplayDataAction {
payload: UIDisplayData;
type: string;
}
12 changes: 12 additions & 0 deletions src/state/trajectory/logics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { ViewerStatus } from "../viewer/types";
import {
changeTime,
resetAgentSelectionsAndHighlights,
setSelectedUIDisplayData,
} from "../selection/actions";
import { setSimulariumController } from "../simularium/actions";
import { getSimulariumController } from "../simularium/selectors";
Expand Down Expand Up @@ -62,6 +63,7 @@ import {
CONVERT_FILE,
RECEIVE_CONVERTED_FILE,
CANCEL_CONVERSION,
SET_DEFAULT_UI_DATA,
} from "./constants";
import {
ReceiveAction,
Expand Down Expand Up @@ -638,6 +640,15 @@ const cancelConversionLogic = createLogic({
type: CANCEL_CONVERSION,
});

const setInitialSelectedUIData = createLogic({
process(deps: ReduxLogicDeps, dispatch) {
const { action } = deps;
const uiData = action.payload;
dispatch(setSelectedUIDisplayData(uiData));
},
type: SET_DEFAULT_UI_DATA,
});
Comment on lines +643 to +650
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic looks like it just re-dispatches the triggering action with a different type? If not, what am I missing; if so, why not just use that action in place of the one that triggers this logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your point is valid!

Context: this was a larger PR, and I got the feedback that I should chunk things up in more bite size pieces. You're correct that this doesn't really need this logic yet, we can just call both setDefaultUIData and setSelectedUIDisplayData in ViewerPanel, and I would happily make that change for now.

I know because I have a crystal ball, that we are likely going to need this asynchronous logic because we won't know what happens first: parsing of the trajectory to derive defaultUIData and retrieval from browser storage of selectedUIData.

This is a problem of my making! Trying to work backwards from bigger feature branches into smaller PRs can lead to issues like this.

Maybe the defining of the subtasks is the real problem, as this whole PR is somewhat contrived and serves no purpose except to lay the groundwork for forthcoming code, so I wonder what is the greater sin: using this logic to set derived state, or correctly storing state that is never accessed in the first place.


export default [
requestPlotDataLogic,
loadLocalFile,
Expand All @@ -650,4 +661,5 @@ export default [
convertFileLogic,
receiveConvertedFileLogic,
cancelConversionLogic,
setInitialSelectedUIData,
];
Loading