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

…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. This mostly occured after a pinned-tab reordering - afterwards, when pinning or unpinning a tab, all previously pinned tabs with a new order would be closed. Sometimes this appeared like a 'crash' when the window closed because all the tabs had been (undesirably) closed.
petemill · Aug 29, 2017 · 01008da · 01008da
1 parent 6fe9849
commit 01008da
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 45 deletions.
diff --git a/app/browser/windows.js b/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')
 const windowActions = require('../../js/actions/windowActions')
@@ -64,51 +63,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 = {
@@ -310,7 +302,6 @@ const api = {
     setImmediate(() => {
       const win = currentWindows[windowId]
       if (win && !win.isDestroyed()) {
-        win.__alreadyPinnedSites = new Immutable.Set()
         updatePinnedTabs(win)
         win.__ready = true
       }

diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js
@@ -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 />

diff --git a/js/actions/appActions.js b/js/actions/appActions.js
@@ -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({
@@ -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
@@ -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

diff --git a/test/lib/brave.js b/test/lib/brave.js
@@ -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()
@@ -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) {

diff --git a/test/tab-components/pinnedTabTest.js b/test/tab-components/pinnedTabTest.js
@@ -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 * () {