Don't allow controlling frame index from window renderer

Tab move no longer allows controlling of the frame index

Fix #10961

app/browser/reducers/tabsReducer.js

@@ -219,7 +219,7 @@ const tabsReducer = (state, action, immutableAction) => {
         tabs.setActive(action.get('tabId'))
       })
       break
-    case appConstants.APP_TAB_INDEX_CHANGED:
+    case appConstants.APP_TAB_INDEX_CHANGE_REQUESTED:
       setImmediate(() => {
         tabs.setTabIndex(action.get('tabId'), action.get('index'))
       })

app/renderer/reducers/frameReducer.js

@@ -94,26 +94,26 @@ const frameReducer = (state, action, immutableAction) => {
       if (!frame) {
         break
       }
-
       let frames = state.get('frames')
-      const changeInfoIndex = changeInfo.get('index')
-      const sourceFrameIndex = frameStateUtil.getFrameIndex(state, frame.get('key'))
-      // When cloning index can be largerthan number of created frames, in this
-      // case we don't want to do anything.
-      if (changeInfoIndex !== undefined &&
-          sourceFrameIndex !== changeInfoIndex &&
-          changeInfoIndex < frames.size) {
-        frame = frame.set('index', changeInfoIndex)
+      const index = tab.get('index')
+      const sourceFrameIndex = frameStateUtil.getIndexByTabId(state, tabId)
+      if (index != null &&
+          sourceFrameIndex !== index) {
+        frame = frame.set('index', index)
         frames = frames
           .splice(sourceFrameIndex, 1)
-          .splice(changeInfoIndex, 0, frame)
+          .splice(index, 0, frame)
         state = state.set('frames', frames)
         // Since the tab could have changed pages, update the tab page as well
-        state = frameStateUtil.updateFramesInternalIndex(state, Math.min(sourceFrameIndex, changeInfoIndex))
-        state = frameStateUtil.moveFrame(state, tabId, changeInfoIndex)
+        state = frameStateUtil.updateFramesInternalIndex(state, Math.min(sourceFrameIndex, index))
+        state = frameStateUtil.moveFrame(state, tabId, index)
+
+        // Update tab page index to the active tab in case the active tab changed
+        const activeFrame = frameStateUtil.getActiveFrame(state)
+        state = frameStateUtil.updateTabPageIndex(state, activeFrame.get('tabId'))
+        state = frameStateUtil.setPreviewFrameKey(state, null)
       }
 
-      const index = frameStateUtil.getIndexByTabId(state, tabId)
       const pinned = immutableAction.getIn(['changeInfo', 'pinned'])
       if (pinned != null) {
         if (pinned) {
@@ -246,7 +246,6 @@ const frameReducer = (state, action, immutableAction) => {
     case windowConstants.WINDOW_SET_FULL_SCREEN:
       state = setFullScreen(state, action)
       break
-
     case windowConstants.WINDOW_ON_FRAME_BOOKMARK:
       {
         // TODO make this an appAction that gets the bookmark data from tabState
@@ -257,6 +256,18 @@ const frameReducer = (state, action, immutableAction) => {
         }
         break
       }
+    // TODO(bbondy): We should remove this window action completely and just go directly to
+    // the browser process with an app action.
+    case windowConstants.WINDOW_TAB_MOVE: {
+      const sourceFrameIndex = frameStateUtil.getFrameIndex(state, action.sourceFrameKey)
+      let newIndex = frameStateUtil.getFrameIndex(state, action.destinationFrameKey) + (action.prepend ? 0 : 1)
+      if (newIndex > sourceFrameIndex) {
+        newIndex--
+      }
+      const frame = frameStateUtil.getFrameByIndex(state, sourceFrameIndex)
+      appActions.tabIndexChangeRequested(frame.get('tabId'), newIndex)
+      break
+    }
   }
 
   return state

docs/state.md

@@ -678,6 +678,15 @@ WindowStore
     top: number,
     type: string,  // "normal", "popup", or "devtools"
     width: number,
-  }
+  },
+  // framesInternal is the same as index in the frames list, it's just a cache by frameKey and tabId
+  framesInternal: {
+    index: {
+      [frameKey]: [index]
+    },
+    tabIndex: {
+      [tabId]: [index]
+    }
+  },
 }
 ```

js/actions/appActions.js

@@ -243,14 +243,14 @@ const appActions = {
   },
 
   /**
-   * Dispatches a message to the store to change the tab index
+   * Dispatches a message to the store to indicate a user action requested that the tab index change
    *
    * @param {Number} tabId - the tabId
    * @param {Number} index - the new index
    */
-  tabIndexChanged: function (tabId, index) {
+  tabIndexChangeRequested: function (tabId, index) {
     dispatch({
-      actionType: appConstants.APP_TAB_INDEX_CHANGED,
+      actionType: appConstants.APP_TAB_INDEX_CHANGE_REQUESTED,
       tabId,
       index
     })

js/constants/appConstants.js

@@ -61,7 +61,7 @@ const appConstants = {
   APP_TAB_ATTACHED: _,
   APP_TAB_WILL_ATTACH: _,
   APP_TAB_ACTIVATE_REQUESTED: _,
-  APP_TAB_INDEX_CHANGED: _,
+  APP_TAB_INDEX_CHANGE_REQUESTED: _,
   APP_TAB_CLOSE_REQUESTED: _,
   APP_CREATE_TAB_REQUESTED: _,
   APP_LOAD_URL_REQUESTED: _,

js/state/frameStateUtil.js

@@ -65,8 +65,11 @@ function getNonPinnedFrames (state) {
   return state.get('frames').filter((frame) => !frame.get('pinnedLocation'))
 }
 
-function getFrameIndex (state, key) {
-  return getFramesInternalIndex(state, key)
+function getFrameIndex (state, frameKey) {
+  if (frameKey == null) return -1
+
+  const index = state.getIn(['framesInternal', 'index', frameKey.toString()])
+  return index == null ? -1 : index
 }
 
 function getActiveFrameIndex (state) {
@@ -181,11 +184,14 @@ function getNonPinnedFrameCount (state) {
 }
 
 function getFrameByTabId (state, tabId) {
-  return getFrameByIndex(state, getFramesInternalIndexByTabId(state, tabId))
+  return getFrameByIndex(state, getIndexByTabId(state, tabId))
 }
 
 function getIndexByTabId (state, tabId) {
-  return getFramesInternalIndexByTabId(state, tabId)
+  if (tabId == null) return -1
+
+  const index = state.getIn(['framesInternal', 'tabIndex', tabId.toString()])
+  return index == null ? -1 : index
 }
 
 function getActiveFrame (state) {
@@ -493,20 +499,6 @@ const frameStatePathByTabId = (state, tabId) => {
 
 const activeFrameStatePath = (state) => frameStatePath(state, getActiveFrameKey(state))
 
-const getFramesInternalIndex = (state, frameKey) => {
-  if (frameKey == null) return -1
-
-  const index = state.getIn(['framesInternal', 'index', frameKey.toString()])
-  return index == null ? -1 : index
-}
-
-const getFramesInternalIndexByTabId = (state, tabId) => {
-  if (tabId == null) return -1
-
-  const index = state.getIn(['framesInternal', 'tabIndex', tabId.toString()])
-  return index == null ? -1 : index
-}
-
 const deleteTabInternalIndex = (state, tabId) => {
   return state.deleteIn(['framesInternal', 'tabIndex', tabId.toString()])
 }

js/stores/windowStore.js

@@ -393,25 +393,6 @@ const doAction = (action) => {
         windowState = frameStateUtil.setTabPageHoverState(windowState, action.tabPageIndex, action.hoverState)
         break
       }
-    case windowConstants.WINDOW_TAB_MOVE:
-      {
-        const sourceFrameProps = frameStateUtil.getFrameByKey(windowState, action.sourceFrameKey)
-        const sourceFrameIndex = frameStateUtil.getFrameIndex(windowState, action.sourceFrameKey)
-        const activeFrame = frameStateUtil.getActiveFrame(windowState)
-        let newIndex = frameStateUtil.getFrameIndex(windowState, action.destinationFrameKey) + (action.prepend ? 0 : 1)
-        let frames = frameStateUtil.getFrames(windowState).splice(sourceFrameIndex, 1)
-        if (newIndex > sourceFrameIndex) {
-          newIndex--
-        }
-        frames = frames.splice(newIndex, 0, sourceFrameProps)
-        windowState = windowState.set('frames', frames)
-        // Since the tab could have changed pages, update the tab page as well
-        windowState = frameStateUtil.updateFramesInternalIndex(windowState, Math.min(sourceFrameIndex, newIndex))
-        windowState = frameStateUtil.moveFrame(windowState, sourceFrameProps.get('tabId'), newIndex)
-        windowState = frameStateUtil.updateTabPageIndex(windowState, activeFrame.get('tabId'))
-        appActions.tabIndexChanged(activeFrame.get('tabId'), newIndex)
-        break
-      }
     case windowConstants.WINDOW_SET_LINK_HOVER_PREVIEW:
       {
         const framePath = frameStateUtil.activeFrameStatePath(windowState)