Skip to content

Commit

Permalink
Stop losing pinned tabs in a window when adding or removing another p…
Browse files Browse the repository at this point in the history
…inned tab

Fix brave#3760, fix brave#9635, fix brave#10037, fix brave#10510, and possibly addresses brave#10117

The previous code resulted in tabs being marked as no longer required to be pinned, and closed prematurely. In many situations, when pinning a tab, all previously pinned tabs would be closed. Sometimes this appeared like a 'crash' when the window closed because all the tabs had been (undesirably) closed.
  • Loading branch information
petemill committed Aug 27, 2017
1 parent c4bcb97 commit a9443d5
Show file tree
Hide file tree
Showing 6 changed files with 117 additions and 49 deletions.
69 changes: 30 additions & 39 deletions app/browser/windows.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ 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')

Expand Down Expand Up @@ -62,51 +61,44 @@ const updateWindow = (windowId) => {
}
}

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 @@ -316,7 +308,6 @@ const api = {
setImmediate(() => {
const win = currentWindows[windowId]
if (win && !win.isDestroyed()) {
win.__alreadyPinnedSites = new Immutable.Set()
updatePinnedTabs(win)
win.__ready = true
}
Expand Down
12 changes: 8 additions & 4 deletions app/common/lib/pinnedSitesUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,13 +110,16 @@ const getKey = (siteDetail) => {
if (!siteDetail) {
return null
}
const location = siteDetail.get('location')
const partitionNumber = siteDetail.get('partitionNumber', 0)
return getKeyFromLocation(location, partitionNumber)
}

let location = siteDetail.get('location')
const getKeyFromLocation = (location, partitionNumber) => {

if (location) {
location = UrlUtil.getLocationIfPDF(location)
return location + '|' +
(siteDetail.get('partitionNumber') || 0)
return `${location}|${partitionNumber}`
}
return null
}
Expand All @@ -126,5 +129,6 @@ module.exports = {
getDetailFromProperties,
getDetailFromFrame,
getPinnedSiteProps,
getKey
getKey,
getKeyFromLocation
}
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 @@ -1111,10 +1111,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 @@ -1124,7 +1123,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 @@ -1141,7 +1140,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
30 changes: 30 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,35 @@ 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}`)
}
// do actual move
await this.moveTabByFrameKey(sourceKey, destinationKey, prepend)
// notify pinned tabs have changed, required for state change
const sourcePinKey = pinnedSiteUtils.getKeyFromLocation(sourceTab.url, sourceTab.partitionNumber || 0)
const destinationPinKey = pinnedSiteUtils.getKeyFromLocation(destinationTab.url, destinationTab.partitionNumber || 0)
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

0 comments on commit a9443d5

Please sign in to comment.