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

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

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

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

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

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)

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

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

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: _,

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

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