refactor: Viewports state (#1218)

* Organize viewports reducers

* Organize viewports actions

* Add local state to store dom node and remove hack

* Comment usage of dom in vtk

* Fix set of enabledElements

* Fix warning in html viewport

* Update docs for state

* Add commandsmanager to commandsmodule

platform/core/src/extensions/ExtensionManager.js

@@ -69,6 +69,7 @@ export default class ExtensionManager {
     if (extension.preRegistration) {
       extension.preRegistration({
         servicesManager: this._servicesManager,
+        commandsManager: this._commandsManager,
         configuration,
       });
     }
@@ -112,6 +113,7 @@ export default class ExtensionManager {
     try {
       const extensionModule = getModuleFn({
         servicesManager: this._servicesManager,
+        commandsManager: this._commandsManager,
       });
 
       if (!extensionModule) {

platform/core/src/extensions/ExtensionManager.test.js

@@ -49,7 +49,7 @@ describe('ExtensionManager.js', () => {
       expect(fakeExtension.preRegistration.mock.calls.length).toBe(1);
     });
 
-    it('calls preRegistration() passing configuration and servicesManager instance for extension', () => {
+    it('calls preRegistration() passing configuration along with servicesManager and commandsManager instances for extension', () => {
       const configuration = { config: 'Some configuration' };
       extensionManager._servicesManager = { services: { TestService: {} } };
 
@@ -60,6 +60,7 @@ describe('ExtensionManager.js', () => {
       // Assert
       expect(fakeExtension.preRegistration.mock.calls[0][0]).toEqual({
         servicesManager: extensionManager._servicesManager,
+        commandsManager: extensionManager._commandsManager,
         configuration,
       });
     });
@@ -134,7 +135,7 @@ describe('ExtensionManager.js', () => {
       );
     });
 
-    it('successfully passes a servicesManager instance to each module', () => {
+    it('successfully passes a servicesManager and commandsManager instances to each module', () => {
       extensionManager._servicesManager = { services: { TestService: {} } };
 
       const extension = {
@@ -150,6 +151,7 @@ describe('ExtensionManager.js', () => {
 
       expect(extension.getViewportModule.mock.calls[0][0]).toEqual({
         servicesManager: extensionManager._servicesManager,
+        commandsManager: extensionManager._commandsManager,
       });
     });
 

platform/core/src/redux/actions.js

@@ -12,13 +12,25 @@ import {
   SET_VIEWPORT_LAYOUT_AND_DATA,
 } from './constants/ActionTypes.js';
 
+/**
+ * The definition of a viewport layout.
+ *
+ * @typedef {Object} ViewportLayout
+ * @property {number} numRows -
+ * @property {number} numColumns -
+ * @property {array} viewports -
+ */
+
 /**
  * VIEWPORT
  */
-export const setViewportSpecificData = (viewportIndex, data) => ({
+export const setViewportSpecificData = (
+  viewportIndex,
+  viewportSpecificData
+) => ({
   type: SET_VIEWPORT,
   viewportIndex,
-  data,
+  viewportSpecificData,
 });
 
 export const setViewportActive = viewportIndex => ({
@@ -27,10 +39,7 @@ export const setViewportActive = viewportIndex => ({
 });
 
 /**
- * @param {object} layout
- * @param {number} layout.numRows
- * @param {number} layout.numColumns
- * @param {array} layout.viewports
+ * @param {ViewportLayout} layout
  */
 export const setLayout = ({ numRows, numColumns, viewports }) => ({
   type: SET_VIEWPORT_LAYOUT,
@@ -40,10 +49,9 @@ export const setLayout = ({ numRows, numColumns, viewports }) => ({
 });
 
 /**
- * @param {object} layout
  * @param {number} layout.numRows
  * @param {number} layout.numColumns
- * @param {array} layout.viewports
+ * @param {array} viewports
  */
 export const setViewportLayoutAndData = (
   { numRows, numColumns, viewports },
@@ -61,9 +69,9 @@ export const clearViewportSpecificData = viewportIndex => ({
   viewportIndex,
 });
 
-export const setActiveViewportSpecificData = data => ({
+export const setActiveViewportSpecificData = viewportSpecificData => ({
   type: SET_ACTIVE_SPECIFIC_DATA,
-  data,
+  viewportSpecificData,
 });
 
 /**
@@ -113,13 +121,18 @@ export const setServers = servers => ({
 });
 
 const actions = {
-  // VIEWPORT
+  /**
+   * VIEWPORT
+   */
   setViewportActive,
   setViewportSpecificData,
   setViewportLayoutAndData,
   setLayout,
   clearViewportSpecificData,
   setActiveViewportSpecificData,
+  /**
+   * NOT-VIEWPORT
+   */
   setStudyLoadingProgress,
   clearStudyLoadingProgress,
   setUserPreferences,

platform/core/src/redux/actions.test.js

@@ -28,7 +28,7 @@ describe('actions', () => {
 
   describe('viewport action creators', () => {
     it('should create an action to set the viewport specific data', () => {
-      const data = {
+      const viewportSpecificData = {
         displaySetInstanceUid: 'ef859a23-4631-93ab-d26b-7940a822c699',
         seriesDate: '20151026',
         seriesTime: '082611.370000',
@@ -65,12 +65,12 @@ describe('actions', () => {
 
       const expectedAction = {
         type: types.SET_ACTIVE_SPECIFIC_DATA,
-        data,
+        viewportSpecificData,
       };
 
-      expect(actions.setActiveViewportSpecificData(data)).toEqual(
-        expectedAction
-      );
+      expect(
+        actions.setActiveViewportSpecificData(viewportSpecificData)
+      ).toEqual(expectedAction);
     });
 
     it('should create an action to clear clearViewportSpecificData', () => {

platform/core/src/redux/reducers/viewports.js

@@ -1,3 +1,6 @@
+import cloneDeep from 'lodash.clonedeep';
+import merge from 'lodash.merge';
+
 import {
   CLEAR_VIEWPORT,
   SET_ACTIVE_SPECIFIC_DATA,
@@ -8,10 +11,7 @@ import {
   SET_VIEWPORT_LAYOUT_AND_DATA,
 } from './../constants/ActionTypes.js';
 
-import cloneDeep from 'lodash.clonedeep';
-import merge from 'lodash.merge';
-
-const defaultState = {
+const DEFAULT_STATE = {
   numRows: 1,
   numColumns: 1,
   activeViewportIndex: 0,
@@ -26,94 +26,137 @@ const defaultState = {
 };
 
 /**
- * @param {Object} [state=defaultState]
- * @param {Object} action
- * @param {string} [action.type]
- * @param {number} [action.viewportIndex]
- * @param {Object} [action.layout]
- * @param {Object} [action.viewportSpecificData]
+ * The definition of a viewport action.
+ *
+ * @typedef {Object} ViewportAction
+ * @property {string} type -
+ * @property {Object} data -
+ * @property {Object} layout -
+ * @property {number} viewportIndex -
+ * @property {Object} viewportSpecificData -
  */
-const viewports = (state = defaultState, action) => {
-  let viewportSpecificData;
+
+/**
+ * @param {Object} [state=DEFAULT_STATE] The current viewport state.
+ * @param {ViewportAction} action A viewport action.
+ */
+const viewports = (state = DEFAULT_STATE, action) => {
   let useActiveViewport = false;
+
   switch (action.type) {
-    case SET_VIEWPORT_ACTIVE:
-      return Object.assign({}, state, {
-        activeViewportIndex: action.viewportIndex,
-      });
+    /**
+     * Sets the active viewport index.
+     *
+     * @return {Object} New state.
+     */
+    case SET_VIEWPORT_ACTIVE: {
+      return { ...state, activeViewportIndex: action.viewportIndex };
+    }
+
+    /**
+     * Sets viewport layout.
+     *
+     * @return {Object} New state.
+     */
     case SET_VIEWPORT_LAYOUT: {
-      const { numRows, numColumns, viewports } = action;
-      const layout = {
-        viewports: [...viewports],
+      return {
+        ...state,
+        numRows: action.numRows,
+        numColumns: action.numColumns,
+        layout: { viewports: [...action.viewports] },
       };
-
-      return Object.assign({}, state, { numRows, numColumns, layout });
     }
+
+    /**
+     * Sets viewport layout and data.
+     *
+     * @return {Object} New state.
+     */
     case SET_VIEWPORT_LAYOUT_AND_DATA: {
-      const { numRows, numColumns, viewports, viewportSpecificData } = action;
-      const layout = {
-        viewports: [...viewports],
+      return {
+        ...state,
+        numRows: action.numRows,
+        numColumns: action.numColumns,
+        layout: { viewports: [...action.viewports] },
+        viewportSpecificData: cloneDeep(action.viewportSpecificData),
       };
-
-      return Object.assign({}, state, {
-        numRows,
-        numColumns,
-        layout,
-        viewportSpecificData: cloneDeep(viewportSpecificData),
-      });
     }
+
+    /**
+     * Sets viewport specific data of active viewport.
+     *
+     * @return {Object} New state.
+     */
     case SET_VIEWPORT: {
       const layout = cloneDeep(state.layout);
-      const hasPlugin = action.data && action.data.plugin;
 
-      viewportSpecificData = cloneDeep(state.viewportSpecificData);
+      let viewportSpecificData = cloneDeep(state.viewportSpecificData);
       viewportSpecificData[action.viewportIndex] = merge(
         {},
         viewportSpecificData[action.viewportIndex],
-        action.data
+        action.viewportSpecificData
       );
 
-      if (hasPlugin) {
-        layout.viewports[action.viewportIndex].plugin = action.data.plugin;
+      if (action.viewportSpecificData && action.viewportSpecificData.plugin) {
+        layout.viewports[action.viewportIndex].plugin =
+          action.viewportSpecificData.plugin;
       }
 
-      return Object.assign({}, state, { layout, viewportSpecificData });
+      return { ...state, layout, viewportSpecificData };
     }
+
+    /**
+     * Sets viewport specific data of active/any viewport.
+     *
+     * @return {Object} New state.
+     */
     case SET_ACTIVE_SPECIFIC_DATA:
       useActiveViewport = true;
     // Allow fall-through
     // eslint-disable-next-line
     case SET_SPECIFIC_DATA: {
       const layout = cloneDeep(state.layout);
-      const hasPlugin = action.data && action.data.plugin;
       const viewportIndex = useActiveViewport
         ? state.activeViewportIndex
         : action.viewportIndex;
-      const { dom } = state.viewportSpecificData[viewportIndex];
 
-      viewportSpecificData = cloneDeep(state.viewportSpecificData);
+      let viewportSpecificData = cloneDeep(state.viewportSpecificData);
       viewportSpecificData[viewportIndex] = {
-        dom,
-        ...action.data,
+        ...action.viewportSpecificData,
       };
 
-      if (hasPlugin) {
-        layout.viewports[viewportIndex].plugin = action.data.plugin;
+      if (action.viewportSpecificData && action.viewportSpecificData.plugin) {
+        layout.viewports[viewportIndex].plugin =
+          action.viewportSpecificData.plugin;
       }
 
-      return Object.assign({}, state, { layout, viewportSpecificData });
+      return { ...state, layout, viewportSpecificData };
     }
-    case CLEAR_VIEWPORT:
-      viewportSpecificData = cloneDeep(state.viewportSpecificData);
+
+    /**
+     * Clears viewport specific data of any viewport.
+     *
+     * @return {Object} New state.
+     */
+    case CLEAR_VIEWPORT: {
+      let viewportSpecificData = cloneDeep(state.viewportSpecificData);
+
       if (action.viewportIndex) {
         viewportSpecificData[action.viewportIndex] = {};
-        return Object.assign({}, state, { viewportSpecificData });
+        return { ...state, viewportSpecificData };
       } else {
-        return defaultState;
+        return DEFAULT_STATE;
       }
+    }
 
-    default:
+    /**
+     * Returns the current application state.
+     *
+     * @return {Object} The current state.
+     */
+    default: {
       return state;
+    }
   }
 };
 

platform/core/src/redux/reducers/viewports.test.js

@@ -60,7 +60,7 @@ describe('viewports reducer', () => {
     const setViewportAction = {
       type: types.SET_VIEWPORT,
       viewportIndex: viewportToSet,
-      data: {
+      viewportSpecificData: {
         hello: 'this is that data for the viewport',
         world: 'that will be set for the viewportIndex',
       },
@@ -69,7 +69,7 @@ describe('viewports reducer', () => {
     const updatedState = reducer(undefined, setViewportAction);
     const updatedViewport = updatedState.viewportSpecificData[viewportToSet];
 
-    expect(updatedViewport).toEqual(setViewportAction.data);
+    expect(updatedViewport).toEqual(setViewportAction.viewportSpecificData);
   });
 
   it('should handle CLEAR_VIEWPORT', () => {

platform/viewer/src/connectedComponents/ConnectedCineDialog.js

@@ -2,6 +2,7 @@ import { connect } from 'react-redux';
 import { CineDialog } from '@ohif/ui';
 import OHIF from '@ohif/core';
 import csTools from 'cornerstone-tools';
+import { commandsManager } from './../App.js';
 // Our target output kills the `as` and "import" throws a keyword error
 // import { import as toolImport, getToolState } from 'cornerstone-tools';
 import cloneDeep from 'lodash.clonedeep';
@@ -16,7 +17,8 @@ const { setViewportSpecificData } = OHIF.redux.actions;
 const mapStateToProps = state => {
   // Get activeViewport's `cine` and `stack`
   const { viewportSpecificData, activeViewportIndex } = state.viewports;
-  const { cine, dom } = viewportSpecificData[activeViewportIndex] || {};
+  const { cine } = viewportSpecificData[activeViewportIndex] || {};
+  const dom = commandsManager.runCommand('getActiveViewportEnabledElement');
 
   const cineData = cine || {
     isPlaying: false,

platform/viewer/src/connectedComponents/ConnectedViewportDownloadForm.js

@@ -3,15 +3,16 @@ import { ViewportDownloadForm } from '@ohif/ui';
 import { utils } from '@ohif/core';
 import cornerstone from 'cornerstone-core';
 import cornerstoneTools from 'cornerstone-tools';
+import { commandsManager } from './../App.js';
 
 const MINIMUM_SIZE = 100;
 const DEFAULT_SIZE = 512;
 const MAX_TEXTURE_SIZE = 10000;
 
 const mapStateToProps = (state, ownProps) => {
-  const { viewportSpecificData, activeViewportIndex } = state.viewports;
-  const { dom: activeEnabledElement } =
-    viewportSpecificData[activeViewportIndex] || {};
+  const activeEnabledElement = commandsManager.runCommand(
+    'getActiveViewportEnabledElement'
+  );
 
   return {
     onClose: ownProps.hide,