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 16, 2017
1 parent 44b3afc commit fa9b09b
Showing 1 changed file with 28 additions and 37 deletions.
65 changes: 28 additions & 37 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)
// sites are instructions of what should be pinned
// tabs are sites our window already has pinned
const tabsWithSites = new Set()
for (const site of pinnedSites.values()) {
const existingPinnedTab = pinnedTabs.find(tab => siteMatchesTab(site, tab))
if (existingPinnedTab) {
tabsWithSites.add(existingPinnedTab)
} else {
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)
})
}
}
for (const tab of pinnedTabs.values()) {
const shouldBePinned = tabsWithSites.has(tab)
if (!shouldBePinned) {
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

0 comments on commit fa9b09b

Please sign in to comment.