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

app/browser/windows.js

@@ -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
@@ -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 = {
@@ -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
  }
@@ -341,6 +331,19 @@ const api = {
  }
 
  return windowState.WINDOW_ID_NONE
+ },
+
+ privateMethods: () => {
+ return process.env.NODE_ENV === 'test'
+ ? {
+ cleanupWindow,
+ getWindowState,
+ getWindowValue,
+ updateWindow,
+ siteMatchesTab,
+ updatePinnedTabs
+ }
+ : {}
  }
 }
 

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 />

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

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) {

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 * () {

test/unit/app/browser/windowsTest.js

@@ -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)
+ })
+ })
+ })
+})