From 63647e18dff245c73b7b2fd9d9476c9f23735ffb Mon Sep 17 00:00:00 2001 From: Brian Clifton Date: Fri, 23 Feb 2018 17:57:48 +0000 Subject: [PATCH] Merge pull request #13271 from brave/fix/13264-pinned-tab-persist-order Persist pinned tab order when a tab is moved by any means --- app/browser/reducers/pinnedSitesReducer.js | 56 +++++--- app/browser/tabs.js | 5 +- app/browser/windows.js | 4 + app/common/state/pinnedSitesState.js | 50 ++++--- app/renderer/components/tabs/pinnedTabs.js | 14 +- app/renderer/components/tabs/tab.js | 1 + js/actions/appActions.js | 9 -- js/constants/appConstants.js | 1 - test/lib/brave.js | 31 ----- test/tab-components/pinnedTabTest.js | 2 +- .../reducers/pinnedSitesReducerTest.js | 122 +++++++++++------- 11 files changed, 146 insertions(+), 149 deletions(-) diff --git a/app/browser/reducers/pinnedSitesReducer.js b/app/browser/reducers/pinnedSitesReducer.js index e717dfe23eb..7ff2c57bb69 100644 --- a/app/browser/reducers/pinnedSitesReducer.js +++ b/app/browser/reducers/pinnedSitesReducer.js @@ -16,15 +16,17 @@ const {STATE_SITES} = require('../../../js/constants/stateConstants') const {makeImmutable} = require('../../common/state/immutableUtil') const syncUtil = require('../../../js/state/syncUtil') const pinnedSitesUtil = require('../../common/lib/pinnedSitesUtil') +const {shouldDebugTabEvents} = require('../../cmdLine') const pinnedSitesReducer = (state, action, immutableAction) => { action = immutableAction || makeImmutable(action) switch (action.get('actionType')) { case appConstants.APP_TAB_UPDATED: { + const tabId = action.getIn(['tabValue', 'tabId']) + // has this tab just been pinned or un-pinned? if (action.getIn(['changeInfo', 'pinned']) != null) { const pinned = action.getIn(['changeInfo', 'pinned']) - const tabId = action.getIn(['tabValue', 'tabId']) const tab = tabState.getByTabId(state, tabId) if (!tab) { console.warn('Trying to pin a tabId which does not exist:', tabId, 'tabs: ', state.get('tabs').toJS()) @@ -38,6 +40,39 @@ const pinnedSitesReducer = (state, action, immutableAction) => { state = pinnedSitesState.removePinnedSite(state, siteDetail) } state = syncUtil.updateObjectCache(state, siteDetail, STATE_SITES.PINNED_SITES) + } else if (action.getIn(['changeInfo', 'index']) != null && tabState.isTabPinned(state, tabId)) { + // The tab index changed and tab is already pinned. + // We cannot rely on the index reported by muon as pinned tabs may not always start at 0, + // and each window may have a different index for each pinned tab, + // but we want the 'order' in pinnedSites to be sequential starting at 0. + // So, focus on the order of the tabs, and make our own index. + const windowId = tabState.getWindowId(state, tabId) + if (windowId == null || windowId === -1) { + break + } + let windowPinnedTabs = tabState.getPinnedTabsByWindowId(state, windowId) + if (!windowPinnedTabs) { + break + } + windowPinnedTabs = windowPinnedTabs.sort((a, b) => a.get('index') - b.get('index')) + const tab = tabState.getByTabId(state, tabId) + const windowPinnedTabIndex = windowPinnedTabs.findIndex(pinnedTab => pinnedTab === tab) + if (windowPinnedTabIndex === -1) { + console.error(`pinnedSitesReducer:APP_TAB_UPDATED: could not find tab ${tabId} as a pinned tab in tabState!`) + } + const siteKey = pinnedSitesUtil.getKey(pinnedSitesUtil.getDetailsFromTab(pinnedSitesState.getSites(state), tab)) + // update state for new order so pinned tabs order is persisted on restart + if (shouldDebugTabEvents) { + console.log(`Moving pinned site '${siteKey}' to order: ${windowPinnedTabIndex}`) + } + const newState = pinnedSitesState.moveSiteToNewOrder(state, siteKey, windowPinnedTabIndex, shouldDebugTabEvents) + // did anything change? + if (newState !== state) { + state = newState + // make sure it's synced + const newSite = pinnedSitesState.getSite(state, siteKey) + state = syncUtil.updateObjectCache(state, newSite, STATE_SITES.PINNED_SITES) + } } break } @@ -49,25 +84,6 @@ const pinnedSitesReducer = (state, action, immutableAction) => { } break } - case appConstants.APP_ON_PINNED_TAB_REORDER: - { - const siteKey = action.get('siteKey') - - if (siteKey == null) { - break - } - - state = pinnedSitesState.reOrderSite( - state, - siteKey, - action.get('destinationKey'), - action.get('prepend') - ) - - const newSite = pinnedSitesState.getSite(state, siteKey) - state = syncUtil.updateObjectCache(state, newSite, STATE_SITES.PINNED_SITES) - break - } } return state diff --git a/app/browser/tabs.js b/app/browser/tabs.js index b092853229a..62b3d10c5d5 100644 --- a/app/browser/tabs.js +++ b/app/browser/tabs.js @@ -73,7 +73,7 @@ const getTabValue = function (tabId) { const updateTab = (tabId, changeInfo = {}) => { let tabValue = getTabValue(tabId) if (shouldDebugTabEvents) { - console.log('tab updated from muon', { tabId, changeIndex: changeInfo.index, changeActive: changeInfo.active, newIndex: tabValue && tabValue.get('index'), newActive: tabValue && tabValue.get('active') }) + console.log(`Tab [${tabId}] updated from muon`, changeInfo, { newIndex: tabValue && tabValue.get('index'), newActive: tabValue && tabValue.get('active') }) } if (tabValue) { appActions.tabUpdated(tabValue, makeImmutable(changeInfo)) @@ -988,6 +988,9 @@ const api = { } const doCreate = () => { + if (shouldDebugTabEvents) { + console.log('Creating tab with properties: ', createProperties) + } extensions.createTab(createProperties, (tab) => { cb && cb(tab) }) diff --git a/app/browser/windows.js b/app/browser/windows.js index 2f62826530d..f7496a39f28 100644 --- a/app/browser/windows.js +++ b/app/browser/windows.js @@ -125,7 +125,10 @@ const updatePinnedTabs = (win, appState) => { // tabs are sites our window already has pinned // for each site which should be pinned, find if it's already pinned const statePinnedSitesOrdered = statePinnedSites.sort((a, b) => a.get('order') - b.get('order')) + // pinned sites should always be at the front of the window tab indexes, starting with 0 + let pinnedSiteIndex = -1 for (const site of statePinnedSitesOrdered.values()) { + pinnedSiteIndex++ 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 @@ -135,6 +138,7 @@ const updatePinnedTabs = (win, appState) => { appActions.createTabRequested({ url: site.get('location'), partitionNumber: site.get('partitionNumber'), + index: pinnedSiteIndex, pinned: true, active: false, windowId diff --git a/app/common/state/pinnedSitesState.js b/app/common/state/pinnedSitesState.js index 59fd051a50a..ad4a5ddf186 100644 --- a/app/common/state/pinnedSitesState.js +++ b/app/common/state/pinnedSitesState.js @@ -98,48 +98,42 @@ const pinnedSiteState = { }, /** - * Moves the specified pinned site from one location to another + * Moves the specified pinned site from one position to another * - * @param state The application state Immutable map + * @param state The application state Immutable map * @param sourceKey The site key to move - * @param destinationKey The site key to move to - * @param prepend Whether the destination detail should be prepended or not + * @param newOrder The new position to move to * @return The new state Immutable object */ - reOrderSite: (state, sourceKey, destinationKey, prepend) => { - state = validateState(state) - let sites = state.get(STATE_SITES.PINNED_SITES) + moveSiteToNewOrder: (state, sourceKey, newOrder, shouldDebug = false) => { + const sites = state.get(STATE_SITES.PINNED_SITES) + if (shouldDebug) { + console.log('moveSiteToNewOrder pinnedSites before', sites.toJS()) + } let sourceSite = sites.get(sourceKey, Immutable.Map()) - const destinationSite = sites.get(destinationKey, Immutable.Map()) - - if (sourceSite.isEmpty()) { + if (sourceSite.isEmpty() || sourceSite.get('order') === newOrder) { + if (shouldDebug) { + console.log('NO CHANGE') + } return state } - - const sourceSiteIndex = sourceSite.get('order') - const destinationSiteIndex = destinationSite.get('order') - let newIndex = destinationSiteIndex + (prepend ? 0 : 1) - if (destinationSiteIndex > sourceSiteIndex) { - --newIndex - } - - state = state.set(STATE_SITES.PINNED_SITES, state.get(STATE_SITES.PINNED_SITES).map((site, index) => { + const sourceSiteOrder = sourceSite.get('order') + state = state.set(STATE_SITES.PINNED_SITES, sites.map((site, index) => { const siteOrder = site.get('order') - if (index === sourceKey) { - return site + if (index === sourceKey && siteOrder !== newOrder) { + return site.set('order', newOrder) } - - if (siteOrder >= newIndex && siteOrder < sourceSiteIndex) { + if (siteOrder >= newOrder && siteOrder < sourceSiteOrder) { return site.set('order', siteOrder + 1) - } else if (siteOrder <= newIndex && siteOrder > sourceSiteIndex) { + } else if (siteOrder <= newOrder && siteOrder > sourceSiteOrder) { return site.set('order', siteOrder - 1) } - return site })) - - sourceSite = sourceSite.set('order', newIndex) - return state.setIn([STATE_SITES.PINNED_SITES, sourceKey], sourceSite) + if (shouldDebug) { + console.log('moveSiteToNewOrder pinnedSites after', state.get(STATE_SITES.PINNED_SITES).toJS()) + } + return state } } diff --git a/app/renderer/components/tabs/pinnedTabs.js b/app/renderer/components/tabs/pinnedTabs.js index c8522cc74a3..c197afa6e1f 100644 --- a/app/renderer/components/tabs/pinnedTabs.js +++ b/app/renderer/components/tabs/pinnedTabs.js @@ -25,7 +25,6 @@ const dragTypes = require('../../../../js/constants/dragTypes') const dnd = require('../../../../js/dnd') const dndData = require('../../../../js/dndData') const frameStateUtil = require('../../../../js/state/frameStateUtil') -const pinnedSitesUtil = require('../../../common/lib/pinnedSitesUtil') const {isIntermediateAboutPage} = require('../../../../js/lib/appUrlUtil') class PinnedTabs extends React.Component { @@ -54,18 +53,11 @@ class PinnedTabs extends React.Component { let droppedOnTab = dnd.closestFromXOffset(this.tabRefs.filter((node) => node && node.props.frameKey !== key), clientX).selectedRef if (droppedOnTab) { const isLeftSide = dnd.isLeftSide(ReactDOM.findDOMNode(droppedOnTab), clientX) + const sourceIsPinned = sourceDragData.get('pinnedLocation') + // TODO: pass in needs-pinning in moveTab, and do nothing else here windowActions.moveTab(key, droppedOnTab.props.frameKey, isLeftSide) - if (!sourceDragData.get('pinnedLocation')) { + if (!sourceIsPinned) { appActions.tabPinned(sourceDragData.get('tabId'), true) - } else { - const sourceDetails = pinnedSitesUtil.getDetailFromFrame(sourceDragData) - const droppedOnFrame = this.dropFrame(droppedOnTab.props.frameKey) - const destinationDetails = pinnedSitesUtil.getDetailFromFrame(droppedOnFrame) - appActions.onPinnedTabReorder( - pinnedSitesUtil.getKey(sourceDetails), - pinnedSitesUtil.getKey(destinationDetails), - isLeftSide - ) } } }, 0) diff --git a/app/renderer/components/tabs/tab.js b/app/renderer/components/tabs/tab.js index 547fa166e84..503a9d3512d 100644 --- a/app/renderer/components/tabs/tab.js +++ b/app/renderer/components/tabs/tab.js @@ -341,6 +341,7 @@ class Tab extends React.Component { onMouseEnter={this.onMouseEnter} onMouseLeave={this.onMouseLeave} data-test-id='tab-area' + data-tab-id={this.props.tabId} data-frame-key={this.props.frameKey} ref={elementRef => { this.elementRef = elementRef }} > diff --git a/js/actions/appActions.js b/js/actions/appActions.js index f1fbcb330b8..c198bd5e0f1 100644 --- a/js/actions/appActions.js +++ b/js/actions/appActions.js @@ -1779,15 +1779,6 @@ const appActions = { }) }, - onPinnedTabReorder: function (siteKey, destinationKey, prepend) { - dispatch({ - actionType: appConstants.APP_ON_PINNED_TAB_REORDER, - siteKey, - destinationKey, - prepend - }) - }, - /** * Dispatches a message that bookmark calculation was done * @param bookmarkList {Object} - Object is a list of bookmarks with key, width and parentFolderId as a property diff --git a/js/constants/appConstants.js b/js/constants/appConstants.js index 0594e4b5545..ca133e4b360 100644 --- a/js/constants/appConstants.js +++ b/js/constants/appConstants.js @@ -147,7 +147,6 @@ const appConstants = { APP_LEARN_SPELLING: _, APP_FORGET_LEARNED_SPELLING: _, APP_SET_VERSION_INFO: _, - APP_ON_PINNED_TAB_REORDER: _, APP_ADD_BOOKMARK_FOLDER: _, APP_EDIT_BOOKMARK_FOLDER: _, APP_REMOVE_BOOKMARK_FOLDER: _, diff --git a/test/lib/brave.js b/test/lib/brave.js index 50c49597da2..23339e62d88 100644 --- a/test/lib/brave.js +++ b/test/lib/brave.js @@ -10,7 +10,6 @@ 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() @@ -645,36 +644,6 @@ var exports = { }, 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('moveTabIncrementally', function (moveNext, windowId = 1) { logVerbose(`moveTabIncrementally(${moveNext}, ${windowId}`) return this.execute(function (moveNext, windowId) { diff --git a/test/tab-components/pinnedTabTest.js b/test/tab-components/pinnedTabTest.js index b0287071a43..1cf636eb0fb 100644 --- a/test/tab-components/pinnedTabTest.js +++ b/test/tab-components/pinnedTabTest.js @@ -90,7 +90,7 @@ describe('pinnedTabs', function () { .windowByUrl(Brave.browserWindowUrl) .waitForExist('[data-test-id="tab"][data-frame-key="4"]') // change pinned tabs order - .movePinnedTabByFrameKey(3, 2, true) + .moveTabByFrameKey(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 diff --git a/test/unit/app/browser/reducers/pinnedSitesReducerTest.js b/test/unit/app/browser/reducers/pinnedSitesReducerTest.js index 89dd49ac93c..b34fba742d5 100644 --- a/test/unit/app/browser/reducers/pinnedSitesReducerTest.js +++ b/test/unit/app/browser/reducers/pinnedSitesReducerTest.js @@ -7,8 +7,10 @@ const mockery = require('mockery') const Immutable = require('immutable') const assert = require('assert') const sinon = require('sinon') - +const fakeElectron = require('../../../lib/fakeElectron') +const fakeAdBlock = require('../../../lib/fakeAdBlock') const appConstants = require('../../../../../js/constants/appConstants') + require('../../../braveUnit') describe('pinnedSitesReducer unit test', function () { @@ -40,25 +42,54 @@ describe('pinnedSitesReducer unit test', function () { windowId: 1, windowUUID: 'uuid', url: 'https://brave.com/', - title: 'Brave' + title: 'Brave', + partitionNumber: 0, + pinned: true, + index: 0 + }, { + tabId: 2, + windowId: 1, + windowUUID: 'uuid', + url: 'https://petemill.com/', + title: 'Brave', + partitionNumber: 0, + pinned: true, + index: 1 + }, { + tabId: 3, + windowId: 1, + windowUUID: 'uuid', + url: 'https://clifton.io/', + title: 'Brave', + partitionNumber: 0, + pinned: true, + index: 2 }], tabsInternal: { index: { + 3: 2, + 2: 1, 1: 0 } }, pinnedSites: { 'https://brave.com/|0': { location: 'https://brave.com/', - order: 0, + order: 2, // changed to 0 title: 'Brave', partitionNumber: 0 }, 'https://clifton.io/|0': { location: 'https://clifton.io/', - order: 1, + order: 0, // changed to 1 title: 'Clifton', partitionNumber: 0 + }, + 'https://petemill.com/|0': { + location: 'https://petemill.com/', + order: 1, // changed to 2 + title: 'Pete', + partitionNumber: 0 } } }) @@ -69,6 +100,8 @@ describe('pinnedSitesReducer unit test', function () { warnOnUnregistered: false, useCleanCache: true }) + mockery.registerMock('electron', fakeElectron) + mockery.registerMock('ad-block', fakeAdBlock) pinnedSitesReducer = require('../../../../../app/browser/reducers/pinnedSitesReducer') pinnedSitesState = require('../../../../../app/common/state/pinnedSitesState') }) @@ -151,6 +184,44 @@ describe('pinnedSitesReducer unit test', function () { assert.equal(spyRemove.calledOnce, true) assert.deepEqual(newState.toJS(), state.toJS()) }) + + it('reorder pinned site', function () { + // for this test there are three tabs, in completely different + // order than the 'pinned sites' state, but we only learn + // about each tab index change one at a time + // i.e. tabs are at b:0 p:1 c:2 but pinned state has c:0 p:1 b:2 + // and we learn about b moving to 0 first, then c moving to 2 + // first tab + const actualFirst = pinnedSitesReducer(stateWithData, { + actionType: appConstants.APP_TAB_UPDATED, + changeInfo: { + index: 1 // irrelevant + }, + tabValue: { + tabId: 1 // b + } + }) + let expectedFirstState = stateWithData + .setIn(['pinnedSites', 'https://clifton.io/|0', 'order'], 1) + .setIn(['pinnedSites', 'https://brave.com/|0', 'order'], 0) + .setIn(['pinnedSites', 'https://petemill.com/|0', 'order'], 2) + assert.deepEqual(actualFirst.toJS(), expectedFirstState.toJS()) + // second tab (it was swapped with) + const actualSecond = pinnedSitesReducer(actualFirst, { + actionType: appConstants.APP_TAB_UPDATED, + changeInfo: { + index: 1 // irrelevant + }, + tabValue: { + tabId: 3 // c + } + }) + let expectedSecondState = stateWithData + .setIn(['pinnedSites', 'https://clifton.io/|0', 'order'], 2) + .setIn(['pinnedSites', 'https://brave.com/|0', 'order'], 0) + .setIn(['pinnedSites', 'https://petemill.com/|0', 'order'], 1) + assert.deepEqual(actualSecond.toJS(), expectedSecondState.toJS()) + }) }) describe('APP_CREATE_TAB_REQUESTED', function () { @@ -186,47 +257,4 @@ describe('pinnedSitesReducer unit test', function () { assert.deepEqual(newState.toJS(), expectedState.toJS()) }) }) - - describe('APP_ON_PINNED_TAB_REORDER', function () { - let spy - - afterEach(function () { - spy.restore() - }) - - it('null case', function () { - spy = sinon.spy(pinnedSitesState, 'reOrderSite') - const newState = pinnedSitesReducer(state, { - actionType: appConstants.APP_ON_PINNED_TAB_REORDER - }) - assert.equal(spy.notCalled, true) - assert.deepEqual(newState.toJS(), state.toJS()) - }) - - it('check if works', function () { - spy = sinon.spy(pinnedSitesState, 'reOrderSite') - const newState = pinnedSitesReducer(stateWithData, { - actionType: appConstants.APP_ON_PINNED_TAB_REORDER, - siteKey: 'https://clifton.io/|0', - destinationKey: 'https://brave.com/|0', - prepend: true - }) - const expectedState = state.setIn(['pinnedSites'], Immutable.fromJS({ - 'https://clifton.io/|0': { - location: 'https://clifton.io/', - order: 0, - title: 'Clifton', - partitionNumber: 0 - }, - 'https://brave.com/|0': { - location: 'https://brave.com/', - order: 1, - title: 'Brave', - partitionNumber: 0 - } - })) - assert.equal(spy.calledOnce, true) - assert.deepEqual(newState.toJS(), expectedState.toJS()) - }) - }) })