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

Commit

Permalink
Merge pull request #9400 from bsclifton/fix-close-tab-behavior
Browse files Browse the repository at this point in the history 
"Select its parent tab" setting now works as expected
  • Loading branch information
@bsclifton
bsclifton authored Jun 16, 2017
2 parents e8f4f25 + b536afb commit bf34c40
Show file tree
Hide file tree
Showing 4 changed files with 52 additions and 35 deletions.
5 changes: 2 additions & 3 deletions app/browser/reducers/tabsReducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,15 +51,14 @@ const updateActiveTab = (state, closeTabId) => {
case tabCloseAction.PARENT:
{
const openerTabId = tabState.getOpenerTabId(state, closeTabId)
const lastActiveTabId = tabState.getLastActiveTabId(state, windowId)
if (openerTabId === lastActiveTabId) {
if (openerTabId !== tabState.TAB_ID_NONE) {
nextTabId = openerTabId
}
break
}
}

// always fall back to NEXT
// DEFAULT: always fall back to NEXT
if (nextTabId === tabState.TAB_ID_NONE) {
nextTabId = tabState.getNextTabIdByIndex(state, windowId, index)
if (nextTabId === tabState.TAB_ID_NONE) {
Expand Down
10 changes: 9 additions & 1 deletion app/common/state/tabState.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,15 @@ const tabState = {
},

getOpenerTabId: (state, tabId) => {
return tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE)
const openerTabId = tabState.getTabPropertyByTabId(state, tabId, 'openerTabId', tabState.TAB_ID_NONE)
if (openerTabId !== tabState.TAB_ID_NONE) {
// Validate that tab exists
const tab = tabState.getByTabId(state, openerTabId)
if (tab) {
return openerTabId
}
}
return tabState.TAB_ID_NONE
},

getIndex: (state, tabId) => {
Expand Down
39 changes: 20 additions & 19 deletions test/unit/app/browser/reducers/tabsReducerTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,7 @@ describe('tabsReducer unit tests', function () {
after(function () {
tabCloseSetting = tabCloseAction.PARENT
})

it('chooses the next tab', function () {
const pickNextAction = {
actionType: action.actionType,
Expand All @@ -316,25 +317,8 @@ describe('tabsReducer unit tests', function () {
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, action)
this.clock.tick(1510)
assert(this.tabsAPI.setActive.withArgs(4).calledOnce)
})
})

describe('when last active tab is not set', function () {
beforeEach(function () {
this.getLastActiveTabIdStub = sinon.stub(this.tabStateAPI, 'getLastActiveTabId')
})
afterEach(function () {
this.getLastActiveTabIdStub.restore()
})

it('chooses next unpinned tab if nextTabId is TAB_ID_NONE', function () {
it('chooses next unpinned tab', function () {
const pickNextAction = {
actionType: action.actionType,
tabId: 3
Expand All @@ -348,7 +332,7 @@ describe('tabsReducer unit tests', function () {
assert(this.tabsAPI.setActive.withArgs(5).calledOnce)
})

it('chooses previous unpinned tab if nextTabId is TAB_ID_NONE', function () {
it('chooses previous unpinned tab', function () {
const testState = this.state
.setIn(['tabs', 2, 'active'], true)
.setIn(['tabs', 3, 'active'], false)
Expand All @@ -375,6 +359,23 @@ describe('tabsReducer unit tests', function () {
})
})
})

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)
})

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)
})
})
})
})

Expand Down
33 changes: 21 additions & 12 deletions test/unit/app/common/state/tabStateTest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ const defaultAppState = Immutable.fromJS({

const twoTabsAppState = defaultAppState
.set('tabs', Immutable.fromJS([
{ tabId: 1, index: 0, windowId: 1, frameKey: 2 },
{ tabId: 2, index: 1, windowId: 1, frameKey: 1 }
{ tabId: 1, index: 0, windowId: 1 },
{ tabId: 2, index: 1, windowId: 1 }
]))
.set('tabsInternal', Immutable.fromJS({
index: {
Expand Down Expand Up @@ -213,7 +213,6 @@ describe('tabState unit tests', function () {
let tab = tabState.getByTabId(this.appState, 2)
assert(tab)
assert.equal(1, tab.get('windowId'))
assert.equal(1, tab.get('frameKey'))
assert.equal(2, tab.get('tabId'))
})

Expand Down Expand Up @@ -342,15 +341,13 @@ describe('tabState unit tests', function () {
.set('tabs', Immutable.fromJS([
{
windowId: 1,
frameKey: 1,
tabId: 1,
index: 0,
myProp: 'test1',
myProp2: 'blah'
},
{
windowId: 1,
frameKey: 1,
tabId: 2,
index: 1,
myProp: 'test2',
Expand All @@ -373,13 +370,11 @@ describe('tabState unit tests', function () {
index: 0,
test: 'blue',
windowId: 1,
frameKey: 1,
myProp: 'test2',
myProp2: 'blah'
},
{
windowId: 1,
frameKey: 1,
tabId: 2,
index: 1,
myProp: 'test2',
Expand All @@ -400,7 +395,6 @@ describe('tabState unit tests', function () {
},
{
windowId: 1,
frameKey: 1,
tabId: 2,
index: 1,
myProp: 'test2',
Expand All @@ -416,7 +410,6 @@ describe('tabState unit tests', function () {
assert.equal('test1', tab.get('myProp'))
assert.equal('blah', tab.get('myProp2'))
assert.equal(1, tab.get('windowId'))
assert.equal(1, tab.get('frameKey'))
assert.equal(1, tab.get('tabId'))
assert.equal(true, state.get('otherProp'))
})
Expand All @@ -438,7 +431,6 @@ describe('tabState unit tests', function () {
it('removes the field specified', function () {
const tab = Immutable.fromJS({
windowId: 1,
frameKey: 1,
tabId: 2,
loginRequiredDetail: {
request: { url: 'someurl' },
Expand All @@ -465,7 +457,6 @@ describe('tabState unit tests', function () {
before(function () {
const tabs = Immutable.fromJS([{
windowId: 1,
frameKey: 1,
tabId: 2,
loginRequiredDetail: {
request: { url: 'someurl' },
Expand Down Expand Up @@ -630,7 +621,7 @@ describe('tabState unit tests', function () {
)
assert.throws(
() => {
tabState.setTabs(defaultAppState, [{ frameKey: 1 }])
tabState.setTabs(defaultAppState, [{ index: 0 }])
},
AssertionError
)
Expand Down Expand Up @@ -703,6 +694,24 @@ describe('tabState unit tests', function () {
})
})

describe('getOpenerTabId', function () {
it('returns tabId of the opener', function () {
const tempState = twoTabsAppState
.setIn(['tabs', 1, 'openerTabId'], 1)
assert.equal(tabState.getOpenerTabId(tempState, 2), 1)
})

it('defaults to TAB_ID_NONE if not found', function () {
assert.equal(tabState.getOpenerTabId(twoTabsAppState, 2), tabState.TAB_ID_NONE)
})

it('returns TAB_ID_NONE if tabId is invalid', function () {
const tempState = twoTabsAppState
.setIn(['tabs', 1, 'openerTabId'], 17)
assert.equal(tabState.getOpenerTabId(tempState, 2), tabState.TAB_ID_NONE)
})
})

describe('getActiveTabId', function () {
before(function () {
this.appState = defaultAppState
Expand Down

0 comments on commit bf34c40

Please sign in to comment.