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 #10531 from petemill/fix-pinned-tabs
Browse files Browse the repository at this point in the history
Stop losing pinned tabs in a window when adding or removing another pinned tab
  • Loading branch information
bsclifton committed Aug 29, 2017
2 parents 6fe9849 + 4ced9dd commit c93dbd8
Show file tree
Hide file tree
Showing 6 changed files with 254 additions and 46 deletions.
83 changes: 43 additions & 40 deletions app/browser/windows.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ const {getPinnedTabsByWindowId} = require('../common/state/tabState')
const messages = require('../../js/constants/messages')
const settings = require('../../js/constants/settings')
const windowState = require('../common/state/windowState')
const Immutable = require('immutable')
const pinnedSitesState = require('../common/state/pinnedSitesState')
const pinnedSitesUtil = require('../common/lib/pinnedSitesUtil')
const windowActions = require('../../js/actions/windowActions')

// TODO(bridiver) - set window uuid
Expand Down Expand Up @@ -64,51 +62,44 @@ const updateWindow = (windowId, updateDefault = false) => {
}
}

const siteMatchesTab = (site, tab) => {
const matchesLocation = getLocationIfPDF(tab.get('url')) === site.get('location')
const matchesPartition = tab.get('partitionNumber', 0) === site.get('partitionNumber', 0)
return matchesLocation && matchesPartition
}

const updatePinnedTabs = (win) => {
if (win.webContents.browserWindowOptions.disposition === 'new-popup') {
return
}

const appStore = require('../../js/stores/appStore')
const state = appStore.getState()
const windowId = win.id
const pinnedSites = pinnedSitesState.getSites(state).map(site => pinnedSitesUtil.getPinnedSiteProps(site))
const pinnedTabs = getPinnedTabsByWindowId(state, windowId)

pinnedSites.filter((site) =>
pinnedTabs.find((tab) =>
getLocationIfPDF(tab.get('url')) === site.get('location') &&
(tab.get('partitionNumber') || 0) === (site.get('partitionNumber') || 0))).forEach((site) => {
win.__alreadyPinnedSites = win.__alreadyPinnedSites.add(site)
const pinnedSites = pinnedSitesState.getSites(state)
let pinnedWindowTabs = getPinnedTabsByWindowId(state, windowId)
// sites are instructions of what should be pinned
// tabs are sites our window already has pinned
// for each site which should be pinned, find if it's already pinned
for (const site of pinnedSites.values()) {
const existingPinnedTabIdx = pinnedWindowTabs.findIndex(tab => siteMatchesTab(site, tab))
if (existingPinnedTabIdx !== -1) {
// if it's already pinned we don't need to consider the tab in further searches
pinnedWindowTabs = pinnedWindowTabs.remove(existingPinnedTabIdx)
} else {
// if it's not already pinned, create new pinned tab
appActions.createTabRequested({
url: site.get('location'),
partitionNumber: site.get('partitionNumber'),
pinned: true,
active: false,
windowId
})

const sitesToAdd = pinnedSites.filter((site) =>
!win.__alreadyPinnedSites.find((pinned) => pinned.equals(site)))

sitesToAdd.forEach((site) => {
win.__alreadyPinnedSites = win.__alreadyPinnedSites.add(site)
appActions.createTabRequested({
url: site.get('location'),
partitionNumber: site.get('partitionNumber'),
pinned: true,
active: false,
windowId
})
})

const sitesToClose = win.__alreadyPinnedSites.filter((pinned) =>
!pinnedSites.find((site) => pinned.equals(site)))

sitesToClose
.forEach((site) => {
const tab = pinnedTabs.find((tab) =>
tab.get('url') === site.get('location') &&
(tab.get('partitionNumber') || 0) === (site.get('partitionNumber') || 0))
if (tab) {
appActions.tabCloseRequested(tab.get('tabId'), true)
}
win.__alreadyPinnedSites = win.__alreadyPinnedSites.remove(site)
})
}
}
// all that's left for tabs are the ones that we should close
for (const tab of pinnedWindowTabs) {
appActions.tabCloseRequested(tab.get('tabId'), true)
}
}

const api = {
Expand Down Expand Up @@ -310,7 +301,6 @@ const api = {
setImmediate(() => {
const win = currentWindows[windowId]
if (win && !win.isDestroyed()) {
win.__alreadyPinnedSites = new Immutable.Set()
updatePinnedTabs(win)
win.__ready = true
}
Expand Down Expand Up @@ -341,6 +331,19 @@ const api = {
}

return windowState.WINDOW_ID_NONE
},

privateMethods: () => {
return process.env.NODE_ENV === 'test'
? {
cleanupWindow,
getWindowState,
getWindowValue,
updateWindow,
siteMatchesTab,
updatePinnedTabs
}
: {}
}
}

Expand Down
4 changes: 3 additions & 1 deletion app/renderer/components/tabs/tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,9 @@ class Tab extends React.Component {
style={this.props.tabWidth ? { flex: `0 0 ${this.props.tabWidth}px` } : {}}
onMouseMove={this.onMouseMove}
onMouseEnter={this.onMouseEnter}
onMouseLeave={this.onMouseLeave}>
onMouseLeave={this.onMouseLeave}
data-test-id='tab-area'
data-frame-key={this.props.frameKey}>
{
this.props.isActive && this.props.notificationBarActive
? <NotificationBarCaret />
Expand Down
9 changes: 4 additions & 5 deletions js/actions/appActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1087,10 +1087,9 @@ const appActions = {
},

/**
* Update ledger publishers pinned percentages according to the new synopsis
* Open dialog for default download path setting
* Dispatches a message when a tab is being pinned
* Dispatches a message to change a the pinned status of a tab
* @param {number} tabId - The tabId of the tab to pin
* @param {boolean} pinned - true if the pin should be pinned, false if the tab should be unpinned
*/
tabPinned: function (tabId, pinned) {
dispatch({
Expand All @@ -1100,7 +1099,7 @@ const appActions = {
})
},

/*
/**
* Dispatches a message when a web contents is added
* @param {number} windowId - The windowId of the host window
* @param {object} frameOpts - frame options for the added web contents
Expand All @@ -1117,7 +1116,7 @@ const appActions = {
})
},

/*
/**
* Notifies the app that a drag operation started from within the app
* @param {number} windowId - The source windowId the drag is starting from
* @param {string} dragType - The type of data
Expand Down
38 changes: 38 additions & 0 deletions test/lib/brave.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ const path = require('path')
const fs = require('fs-extra')
const os = require('os')
const {getTargetAboutUrl, isSourceAboutUrl, getBraveExtIndexHTML} = require('../../js/lib/appUrlUtil')
const pinnedSiteUtils = require('../../app/common/lib/pinnedSitesUtil')

var chaiAsPromised = require('chai-as-promised')
chai.should()
Expand Down Expand Up @@ -600,6 +601,43 @@ var exports = {
})
})

this.app.client.addCommand('moveTabByFrameKey', function (sourceKey, destinationKey, prepend) {
logVerbose(`moveTabByFrameKey(${sourceKey}, ${destinationKey}, ${prepend})`)
return this.execute(function (sourceKey, destinationKey, prepend) {
return devTools('electron').testData.windowActions.moveTab(sourceKey, destinationKey, prepend)
}, sourceKey, destinationKey, prepend)
})

this.app.client.addCommand('movePinnedTabByFrameKey', async function (sourceKey, destinationKey, prepend, windowId = 1) {
logVerbose(`movePinnedTabByFrameKey(${sourceKey}, ${destinationKey}, ${prepend})`)
// get info on tabs to move
const state = await this.getAppState()
const sourceTab = state.value.tabs.find(tab => tab.windowId === windowId && tab.frame.key === sourceKey)
if (!sourceTab) {
throw new Error(`movePinnedTabByIndex could not find source tab with key ${sourceKey} in window ${windowId}`)
}
const destinationTab = state.value.tabs.find(tab => tab.windowId === windowId && tab.frame.key === destinationKey)
if (!destinationTab) {
throw new Error(`movePinnedTabByIndex could not find destination tab with key ${destinationKey} in window ${windowId}`)
}
const sourceTabSiteDetail = Immutable.fromJS({
location: sourceTab.url,
partitionNumber: sourceTab.partitionNumber
})
const destinationTabSiteDetail = Immutable.fromJS({
location: destinationTab.url,
paritionNumber: destinationTab.partitionNumber
})
// do actual move
await this.moveTabByFrameKey(sourceKey, destinationKey, prepend)
// notify pinned tabs have changed, required for state change
const sourcePinKey = pinnedSiteUtils.getKey(sourceTabSiteDetail)
const destinationPinKey = pinnedSiteUtils.getKey(destinationTabSiteDetail)
return this.execute(function (sourcePinKey, destinationPinKey, prepend) {
return devTools('appActions').onPinnedTabReorder(sourcePinKey, destinationPinKey, prepend)
}, sourcePinKey, destinationPinKey, prepend)
})

this.app.client.addCommand('ipcOn', function (message, fn) {
logVerbose('ipcOn("' + message + '")')
return this.execute(function (message, fn) {
Expand Down
42 changes: 42 additions & 0 deletions test/tab-components/pinnedTabTest.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,48 @@ describe('pinnedTabs', function () {
})
})

describe('Moving pinned tabs', function () {
Brave.beforeEach(this)
// test case for bug solved with #10531
it('reorders pins without forgetting about them', function * () {
yield setup(this.app.client)
const page1 = Brave.server.url('page1.html')
const page2 = Brave.server.url('page2.html')
const page3 = Brave.server.url('page_no_title.html')
const page4 = Brave.server.url('red_bg.html')
yield this.app.client
// open new tab and pin it
.newTab({ url: page1 })
.waitForUrl(page1)
.windowByUrl(Brave.browserWindowUrl)
.waitForExist('[data-test-id="tab"][data-frame-key="2"]')
.pinTabByIndex(1, true)
// open another new tab and pin it
.newTab({ url: page2 })
.waitForUrl(page2)
.windowByUrl(Brave.browserWindowUrl)
.waitForExist('[data-test-id="tab"][data-frame-key="3"]')
.pinTabByIndex(2, true)
// make sure a non pinned page exists
.newTab({ url: page3 })
.waitForUrl(page3)
.windowByUrl(Brave.browserWindowUrl)
.waitForExist('[data-test-id="tab"][data-frame-key="4"]')
// change pinned tabs order
.movePinnedTabByFrameKey(3, 2, true)
// check the move worked
.waitForExist('[data-test-id="tab-area"][data-frame-key="3"] + [data-test-id="tab-area"][data-frame-key="2"]')
// create another tab and pin it
.newTab({ url: page4 })
.waitForUrl(page4)
.windowByUrl(Brave.browserWindowUrl)
.waitForExist('[data-test-id="tab"][data-frame-key="5"]')
.pinTabByIndex(4, true)
// check we still have the other pinned tabs
.waitForExist('[data-test-id="tab-area"][data-frame-key="3"] + [data-test-id="tab-area"][data-frame-key="2"] + [data-test-id="tab-area"][data-frame-key="5"]')
})
})

describe('Pinning with partitions', function () {
Brave.beforeAll(this)
it('pinning the same site again with a different session is allowed', function * () {
Expand Down
124 changes: 124 additions & 0 deletions test/unit/app/browser/windowsTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
/* global describe, it, before, beforeEach, after, afterEach */
const mockery = require('mockery')
const sinon = require('sinon')
const Immutable = require('immutable')
const assert = require('assert')
const fakeElectron = require('../../lib/fakeElectron')
const fakeAdBlock = require('../../lib/fakeAdBlock')

require('../../braveUnit')

describe('window API unit tests', function () {
let windows, appActions
let appStore
let defaultState, createTabState, tabCloseState

before(function () {
mockery.enable({
warnOnReplace: false,
warnOnUnregistered: false,
useCleanCache: true
})

const tab1 = {
id: 1,
url: 'https://pinned-tab.com',
partitionNumber: 0,
windowId: 1,
pinned: true
}
const pinned1 = {
'https://pinned-tab.com|0|0': {
location: 'https://pinned-tab.com',
partitionNumber: 0,
title: 'Brave Software - Example Pinned Tab',
order: 1
}
}
const pinned2 = {
'https://another-pinned-tab.com|0|0': {
location: 'https://another-pinned-tab.com',
partitionNumber: pinned1.partitionNumber,
title: pinned1.title,
order: pinned1.order
}
}

defaultState = Immutable.fromJS({
pinnedSites: pinned1,
tabs: [tab1]
})

createTabState = Immutable.fromJS({
pinnedSites: pinned2,
tabs: [tab1]
})

tabCloseState = Immutable.fromJS({
pinnedSites: {},
tabs: [tab1]
})

appStore = {
getState: () => Immutable.fromJS(defaultState)
}

mockery.registerMock('electron', fakeElectron)
mockery.registerMock('ad-block', fakeAdBlock)
mockery.registerMock('../../js/stores/appStore', appStore)

windows = require('../../../../app/browser/windows')
appActions = require('../../../../js/actions/appActions')
})

after(function () {
mockery.disable()
})

describe('privateMethods', function () {
let updatePinnedTabs
let createTabRequestedSpy, tabCloseRequestedSpy
const win = {
id: 1,
webContents: {
browserWindowOptions: {
disposition: ''
}
}
}

before(function () {
const methods = windows.privateMethods()
updatePinnedTabs = methods.updatePinnedTabs
})
beforeEach(function () {
createTabRequestedSpy = sinon.spy(appActions, 'createTabRequested')
tabCloseRequestedSpy = sinon.spy(appActions, 'tabCloseRequested')
})
afterEach(function () {
createTabRequestedSpy.restore()
tabCloseRequestedSpy.restore()
appStore.getState = () => Immutable.fromJS(defaultState)
})

describe('updatePinnedTabs', function () {
it('takes no action if pinnedSites list matches tab state', function () {
updatePinnedTabs(win)
assert.equal(createTabRequestedSpy.calledOnce, false)
assert.equal(tabCloseRequestedSpy.calledOnce, false)
})

it('calls `appActions.createTabRequested` for pinnedSites not in tab state', function () {
appStore.getState = () => Immutable.fromJS(createTabState)
updatePinnedTabs(win)
assert.equal(createTabRequestedSpy.calledOnce, true)
})

it('calls `appActions.tabCloseRequested` for items in tab state but not in pinnedSites', function () {
appStore.getState = () => Immutable.fromJS(tabCloseState)
updatePinnedTabs(win)
assert.equal(tabCloseRequestedSpy.calledOnce, true)
})
})
})
})

0 comments on commit c93dbd8

Please sign in to comment.