Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Set next active tab before closing, instead of after. Avoids race-con…
Browse files Browse the repository at this point in the history 
…dition with muon's index-change and active-change tab update events.

Fix #11981
  • Loading branch information
@petemill
petemill committed Dec 6, 2017
1 parent f522e1b commit 0889ecc
Show file tree
Hide file tree
Showing 3 changed files with 113 additions and 121 deletions.
15 changes: 7 additions & 8 deletions app/browser/reducers/tabsReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,14 +178,20 @@ 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(() => {
if (isPinned) {
// 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 {
Expand All @@ -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)
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion app/renderer/reducers/frameReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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:
Expand Down
215 changes: 103 additions & 112 deletions test/unit/app/browser/reducers/tabsReducerTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})

Expand All @@ -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 () {
Expand Down Expand Up @@ -463,6 +351,7 @@ describe('tabsReducer unit tests', function () {

beforeEach(function () {
this.closeWindowSpy = sinon.spy(this.windowsAPI, 'closeWindow')
this.tabsAPI.setActive.reset()
})

afterEach(function () {
Expand Down Expand Up @@ -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()
Expand Down

0 comments on commit 0889ecc

Please sign in to comment.