From 0889ecce6b6f55a2637f074327ac40f9809733a8 Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Tue, 5 Dec 2017 15:35:34 -0800 Subject: [PATCH] Set next active tab before closing, instead of after. Avoids race-condition with muon's index-change and active-change tab update events. Fix #11981 --- app/browser/reducers/tabsReducer.js | 15 +- app/renderer/reducers/frameReducer.js | 4 +- .../app/browser/reducers/tabsReducerTest.js | 215 +++++++++--------- 3 files changed, 113 insertions(+), 121 deletions(-) diff --git a/app/browser/reducers/tabsReducer.js b/app/browser/reducers/tabsReducer.js index feb3b816909..9910c3dcfd5 100644 --- a/app/browser/reducers/tabsReducer.js +++ b/app/browser/reducers/tabsReducer.js @@ -178,7 +178,6 @@ const tabsReducer = (state, action, immutableAction) => { const isPinned = tabState.isTabPinned(state, tabId) const nonPinnedTabs = tabState.getNonPinnedTabsByWindowId(state, windowId) const pinnedTabs = tabState.getPinnedTabsByWindowId(state, windowId) - if (nonPinnedTabs.size > 1 || (nonPinnedTabs.size > 0 && pinnedTabs.size > 0)) { setImmediate(() => { @@ -186,6 +185,13 @@ const tabsReducer = (state, action, immutableAction) => { // if a tab is pinned, unpin before closing state = tabs.pin(state, tabId, false) } + const isActive = tabValue.get('active') + if (isActive) { + const nextActiveTabId = tabs.getNextActiveTab(state, tabId) + if (nextActiveTabId !== tabState.TAB_ID_NONE) { + tabs.setActive(nextActiveTabId) + } + } tabs.closeTab(tabId, action.get('forceClosePinned')) }) } else { @@ -201,8 +207,6 @@ const tabsReducer = (state, action, immutableAction) => { if (tabId === tabState.TAB_ID_NONE) { break } - const nextActiveTabId = tabs.getNextActiveTab(state, tabId) - // Must be called before tab is removed // But still check for no tabId because on tab detach there's a dummy tabId const tabValue = tabState.getByTabId(state, tabId) @@ -211,11 +215,6 @@ const tabsReducer = (state, action, immutableAction) => { state = tabs.updateTabsStateForWindow(state, windowIdOfTabBeingRemoved) } state = tabState.removeTabByTabId(state, tabId) - setImmediate(() => { - if (nextActiveTabId !== tabState.TAB_ID_NONE) { - tabs.setActive(nextActiveTabId) - } - }) tabs.forgetTab(tabId) } break diff --git a/app/renderer/reducers/frameReducer.js b/app/renderer/reducers/frameReducer.js index f16aa1d3bc2..241d0b48c75 100644 --- a/app/renderer/reducers/frameReducer.js +++ b/app/renderer/reducers/frameReducer.js @@ -29,6 +29,7 @@ const setFullScreen = (state, action) => { } const closeFrame = (state, action) => { + console.log("remove frame", action.frameKey) const index = frameStateUtil.getFrameIndex(state, action.frameKey) if (index === -1) { return state @@ -61,7 +62,7 @@ const closeFrame = (state, action) => { } else if (hoverState && frameStateUtil.getPreviewFrameKey(state) === action.frameKey) { state = frameStateUtil.setPreviewFrameKey(state, null) } - + console.log(frameStateUtil.getFrames(state).toJS()) return state } @@ -188,6 +189,7 @@ const frameReducer = (state, action, immutableAction) => { state = state.setIn(['frames', index, 'hasBeenActivated'], true) } state = frameStateUtil.updateTabPageIndex(state, tabId) + console.log(frameStateUtil.getFrames(state).toJS()) } break case windowConstants.WINDOW_SET_NAVIGATED: diff --git a/test/unit/app/browser/reducers/tabsReducerTest.js b/test/unit/app/browser/reducers/tabsReducerTest.js index cf59fd9c4d7..7af6431fad5 100644 --- a/test/unit/app/browser/reducers/tabsReducerTest.js +++ b/test/unit/app/browser/reducers/tabsReducerTest.js @@ -267,7 +267,6 @@ describe('tabsReducer unit tests', function () { beforeEach(function () { this.removeTabByTabIdSpy = sinon.stub(this.tabStateAPI, 'removeTabByTabId', (state) => state) - this.tabsAPI.setActive.reset() this.tabsAPI.forgetTab.reset() }) @@ -292,117 +291,6 @@ describe('tabsReducer unit tests', function () { this.clock.tick(1510) assert(this.tabStateAPI.removeTabByTabId.notCalled) }) - - describe('when updating active tab', function () { - describe('when TAB_CLOSE_ACTION is set to LAST_ACTIVE', function () { - before(function () { - tabCloseSetting = tabCloseAction.LAST_ACTIVE - }) - after(function () { - tabCloseSetting = tabCloseAction.PARENT - }) - it('chooses the last active tab', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - }) - - describe('when TAB_CLOSE_ACTION is set to NEXT', function () { - before(function () { - tabCloseSetting = tabCloseAction.NEXT - }) - after(function () { - tabCloseSetting = tabCloseAction.PARENT - }) - - it('chooses the next tab', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 2, 'openerTabId'], 5) - .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([4, 5, 3])) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - - it('chooses next unpinned tab', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'active'], false) - .setIn(['tabs', 3, 'pinned'], true) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(5).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - - it('chooses previous unpinned tab', function () { - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'active'], false) - .setIn(['tabs', 3, 'pinned'], true) - tabsReducer(testState, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - - describe('if no unpinned tabs come after this', function () { - it('considers pinned tabs which come after this', function () { - const pickNextAction = { - actionType: action.actionType, - tabId: 3 - } - const testState = this.state - .setIn(['tabs', 2, 'active'], true) - .setIn(['tabs', 3, 'pinned'], true) - .setIn(['tabs', 4, 'active'], false) - .setIn(['tabs', 4, 'pinned'], true) - tabsReducer(testState, pickNextAction) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(3).calledOnce) - }) - }) - }) - - describe('when TAB_CLOSE_ACTION is set to PARENT', function () { - it('chooses parent tab id (if parent tab was last active)', function () { - tabsReducer(this.state, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(4).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - - it('chooses parent tab id (even if parent tab was NOT last active)', function () { - const testState = this.state - .setIn(['tabs', 4, 'openerTabId'], 3) - .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([])) - tabsReducer(testState, action) - this.clock.tick(1510) - assert(this.tabsAPI.setActive.withArgs(3).calledOnce) - assert.equal(this.tabsAPI.updateTabsStateForWindow.getCall(0).args[1], 2) - assert(this.tabsAPI.forgetTab.withArgs(5).calledOnce) - }) - }) - }) }) describe.skip('APP_ALLOW_FLASH_ONCE', function () { @@ -463,6 +351,7 @@ describe('tabsReducer unit tests', function () { beforeEach(function () { this.closeWindowSpy = sinon.spy(this.windowsAPI, 'closeWindow') + this.tabsAPI.setActive.reset() }) afterEach(function () { @@ -504,6 +393,108 @@ describe('tabsReducer unit tests', function () { }) }) + describe('when tabId is of active tab', function () { + const actionActiveTab = { + actionType: action.actionType, + tabId: 5 + } + describe('when TAB_CLOSE_ACTION is set to LAST_ACTIVE', function () { + before(function () { + tabCloseSetting = tabCloseAction.LAST_ACTIVE + }) + after(function () { + tabCloseSetting = tabCloseAction.PARENT + }) + it('chooses the last active tab', function () { + tabsReducer(this.state, actionActiveTab) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + }) + }) + + describe('when TAB_CLOSE_ACTION is set to NEXT', function () { + before(function () { + tabCloseSetting = tabCloseAction.NEXT + }) + after(function () { + tabCloseSetting = tabCloseAction.PARENT + }) + + it('chooses the next tab', function () { + const pickNextAction = { + actionType: action.actionType, + tabId: 3 + } + const testState = this.state + .setIn(['tabs', 2, 'active'], true) + .setIn(['tabs', 2, 'openerTabId'], 5) + .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([4, 5, 3])) + tabsReducer(testState, pickNextAction) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + }) + + it('chooses next unpinned tab', function () { + const pickNextAction = { + actionType: action.actionType, + tabId: 3 + } + const testState = this.state + .setIn(['tabs', 2, 'active'], true) + .setIn(['tabs', 3, 'active'], false) + .setIn(['tabs', 3, 'pinned'], true) + tabsReducer(testState, pickNextAction) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(5).calledOnce) + }) + + it('chooses previous unpinned tab', function () { + const testState = this.state + .setIn(['tabs', 2, 'active'], true) + .setIn(['tabs', 3, 'active'], false) + .setIn(['tabs', 3, 'pinned'], true) + tabsReducer(testState, actionActiveTab) + this.clock.tick(1510) + + assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + }) + + describe('if no unpinned tabs come after this', function () { + it('considers pinned tabs which come after this', function () { + const pickNextAction = { + actionType: action.actionType, + tabId: 3 + } + const testState = this.state + .setIn(['tabs', 2, 'active'], true) + .setIn(['tabs', 3, 'pinned'], true) + .setIn(['tabs', 4, 'active'], false) + .setIn(['tabs', 4, 'pinned'], true) + tabsReducer(testState, pickNextAction) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + }) + }) + }) + + describe('when TAB_CLOSE_ACTION is set to PARENT', function () { + it('chooses parent tab id (if parent tab was last active)', function () { + tabsReducer(this.state, actionActiveTab) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(4).calledOnce) + }) + + it('chooses parent tab id (even if parent tab was NOT last active)', function () { + const testState = this.state + .setIn(['tabs', 4, 'openerTabId'], 3) + .setIn(['tabsInternal', 'lastActive', '2'], Immutable.fromJS([])) + tabsReducer(testState, actionActiveTab) + this.clock.tick(1510) + assert(this.tabsAPI.setActive.withArgs(3).calledOnce) + }) + }) + }) + describe('with isDevToolsFocused', function () { afterEach(function () { this.tabsAPI.isDevToolsFocused.restore()