Stop losing pinned tabs in a window when adding or removing another p…

…inned tab

Fix #3760, fix #9635, fix #10037, fix #10510, and possibly addresses #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.

app/browser/windows.js

@@ -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')
 
@@ -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 = {
@@ -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
       }