diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index 3ae8074bc9c..10bda7d4e0a 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/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')) }) diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index 11189b935a0..8af922790cb 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -92,26 +92,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) { @@ -248,7 +248,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 @@ -259,6 +258,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 diff --git a/docs/state.md b/docs/state.md index f095bbefdeb..b1472f36e8d 100644 --- a/docs/state.md +++ b/docs/state.md @@ -674,6 +674,15 @@ WindowStore alsoAddRememberSiteSetting: boolean, // true if an allow always rule should be added for the acitve frame as well if installed location: string, // location this dialog is for shown: boolean // true if the panel is shown - } + }, + // 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] + } + }, } ``` diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 6f1f7c4d9ea..672c88659da 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -242,14 +242,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 }) diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index ffe230e6cf9..49f250679a2 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -63,7 +63,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: _, diff --git a/js/state/frameStateUtil.js b/js/state/frameStateUtil.js index daefb93d37c..05a33184c3f 100644 --- a/js/state/frameStateUtil.js +++ b/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()]) } diff --git a/js/stores/windowStore.js b/js/stores/windowStore.js index 4e486f59b3f..80e4f9b978b 100644 --- a/js/stores/windowStore.js +++ b/js/stores/windowStore.js @@ -381,25 +381,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) diff --git a/test/unit/app/renderer/reducers/frameReducerTest.js b/test/unit/app/renderer/reducers/frameReducerTest.js index 7448ce312b5..c876e969d80 100644 --- a/test/unit/app/renderer/reducers/frameReducerTest.js +++ b/test/unit/app/renderer/reducers/frameReducerTest.js @@ -1,10 +1,12 @@ -/* global describe, it, before, after */ +/* global describe, it, before, after, afterEach */ const mockery = require('mockery') const Immutable = require('immutable') const assert = require('assert') +const sinon = require('sinon') const fakeElectron = require('../../../lib/fakeElectron') const windowConstants = require('../../../../../js/constants/windowConstants') +const appConstants = require('../../../../../js/constants/appConstants') require('../../../braveUnit') const windowState = Immutable.fromJS({ @@ -12,7 +14,7 @@ const windowState = Immutable.fromJS({ searchDetail: {}, frames: [{ key: 1, - tabId: 1, + tabId: 3, title: 'test', adblock: {blocked: []}, audioPlaybackActive: true, @@ -26,7 +28,7 @@ const windowState = Immutable.fromJS({ fingerprintingProtection: {blocked: []} }, { key: 2, - tabId: 2, + tabId: 5, title: 'test', adblock: {blocked: []}, audioPlaybackActive: true, @@ -38,15 +40,31 @@ const windowState = Immutable.fromJS({ themeColor: '#ffffff', trackingProtection: {blocked: []}, fingerprintingProtection: {blocked: []} + }, { + key: 3, + tabId: 13, + title: 'test', + adblock: {blocked: []}, + audioPlaybackActive: true, + computedThemeColor: '#ff0000', + httpsEverywhere: {a: '1'}, + icon: 'https://www.mastercezarameridote.com/favicon.ico', + location: 'https://www.mastercezarmeridote.com', + noScript: {blocked: []}, + themeColor: '#ffffff', + trackingProtection: {blocked: []}, + fingerprintingProtection: {blocked: []} }], framesInternal: { index: { 1: 0, - 2: 1 + 2: 1, + 3: 2 }, tabIndex: { - 1: 0, - 2: 1 + 3: 0, + 5: 1, + 13: 2 } } }) @@ -59,6 +77,7 @@ describe('frameReducer', function () { warnOnUnregistered: false, useCleanCache: true }) + this.appActions = require('../../../../../js/actions/appActions') mockery.registerMock('electron', fakeElectron) frameReducer = require('../../../../../app/renderer/reducers/frameReducer') }) @@ -144,4 +163,150 @@ describe('frameReducer', function () { }) }) }) + + describe('WINDOW_TAB_MOVE', function () { + before(function () { + this.tabIndexChangeRequestedStub = sinon.stub(this.appActions, 'tabIndexChangeRequested') + }) + afterEach(function () { + this.tabIndexChangeRequestedStub.reset() + }) + after(function () { + this.tabIndexChangeRequestedStub.restore() + }) + describe('Can move last tab to first position', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 3, + destinationFrameKey: 1, + prepend: true + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(13, 0).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + describe('Can move last tab to the middle', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 3, + destinationFrameKey: 1, + prepend: false + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(13, 1).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + describe('Can move tabs to the end', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 1, + destinationFrameKey: 3, + prepend: false + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(3, 2).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + describe('Can move tabs to the middle from the left', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 1, + destinationFrameKey: 2, + prepend: false + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(3, 1).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + describe('Can move middle tab left', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 2, + destinationFrameKey: 1, + prepend: true + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(5, 0).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + describe('Can move middle tab right', function () { + before(function () { + this.newState = frameReducer(windowState, { + actionType: windowConstants.WINDOW_TAB_MOVE, + sourceFrameKey: 2, + destinationFrameKey: 3, + prepend: false + }) + }) + it('calls appActions.tabIndexChangeRequested with the correct args', function () { + assert.equal(this.tabIndexChangeRequestedStub.withArgs(5, 2).calledOnce, true) + }) + it('does not change window state', function () { + assert.deepEqual(this.newState.toJS(), windowState.toJS()) + }) + }) + }) + describe('APP_TAB_UPDATED', function () { + describe('when index changes', function () { + it('re-orders frame index when index changes', function () { + const action = { + actionType: appConstants.APP_TAB_UPDATED, + tabValue: { + tabId: 3, + index: 2 + } + } + + this.newState = frameReducer(windowState, action, Immutable.fromJS(action)) + assert.equal(this.newState.toJS().frames.length, windowState.toJS().frames.length) + // First tab moves to third position + assert.equal(this.newState.toJS().frames[2].tabId, windowState.toJS().frames[0].tabId) + // Old index 1 moves 1 to the left + assert.equal(this.newState.toJS().frames[0].tabId, windowState.toJS().frames[1].tabId) + // Old index 2 moves 1 to the left + assert.equal(this.newState.toJS().frames[1].tabId, windowState.toJS().frames[2].tabId) + }) + }) + describe('when pinned status changes', function () { + // TODO(bbondy): Noticed this is missing while in the context of fixing an unrelated thing. + it.skip('(todo)') + }) + describe('when URL changes', function () { + // TODO(bbondy): Noticed this is missing while in the context of fixing an unrelated thing. + it.skip('(todo)') + }) + describe('when title changes', function () { + // TODO(bbondy): Noticed this is missing while in the context of fixing an unrelated thing. + it.skip('(todo)') + }) + describe('when active state changes', function () { + // TODO(bbondy): Noticed this is missing while in the context of fixing an unrelated thing. + it.skip('(todo)') + }) + }) })