From a8e12abaa02eeef1723d3cc79f98e10c58182655 Mon Sep 17 00:00:00 2001 From: Pete Miller Date: Wed, 16 Aug 2017 01:34:01 -0700 Subject: [PATCH] Stop losing pinned tabs in a window when adding or removing another pinned 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. 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. --- app/browser/windows.js | 69 ++++++++++++---------------- app/renderer/components/tabs/tab.js | 4 +- js/actions/appActions.js | 9 ++-- test/lib/brave.js | 38 +++++++++++++++ test/tab-components/pinnedTabTest.js | 42 +++++++++++++++++ 5 files changed, 117 insertions(+), 45 deletions(-) diff --git a/app/browser/windows.js b/app/browser/windows.js index 99284906926..1e53f23342d 100644 --- 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') @@ -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 } diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js index e350db02fcb..63cc64f7e01 100644 --- 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 ? diff --git a/js/actions/appActions.js b/js/actions/appActions.js index 992f4ff2626..30a16aa30bf 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -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({ @@ -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 @@ -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 diff --git a/test/lib/brave.js b/test/lib/brave.js index babc0f7c4bc..758967740da 100644 --- 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 index a52136e825b..0710b78d5a9 100644 --- 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 * () {