Current sorting algorithm doesn't distinct between tabs with equal UR…

…Ls. (#69)

* Add ids in tab sorting tests

* Use creation order in tabs sorting

* Prioritize the most recently active tab

* Show the effect of creation order usage in e2e tests

e2e/popup-e2e.ts

@@ -92,12 +92,18 @@ describe('popup', function TestPopup() {
     it(`restores visited pages order after the background worker restart`, async () => {
       // Open multiple tabs.
       await helper.openPage('wikipedia.html')
-      const pageExample = await helper.openPage('example.html')
+      const pageExample1 = await helper.openPage('example.html')
+      await pageExample1.evaluate(() => {
+        document.title = 'Example 1'
+      })
       const pageStOverflow = await helper.openPage('stackoverflow.html')
-      await helper.openPage('example.html')
+      const pageExample2 = await helper.openPage('example.html')
+      await pageExample2.evaluate(() => {
+        document.title = 'Example 2'
+      })
       await helper.openPage('links.html')
       // Send command to reload the extension to simulate web worker shut down.
-      await pageExample.bringToFront()
+      await pageExample1.bringToFront()
       await pageStOverflow.bringToFront()
       await helper.sendMessage(e2eReloadExtension())
       await helper.selectTabForward()
@@ -106,7 +112,7 @@ describe('popup', function TestPopup() {
       )
       assert.deepStrictEqual(
         elTexts,
-        ['Stack Overflow', 'Example', 'Links', 'Example', 'Wikipedia'],
+        ['Stack Overflow', 'Example 1', 'Links', 'Example 2', 'Wikipedia'],
         'The history of visited pages is not preserved after the extension reload.'
       )
     })

e2e/utils/long-server-response.js

@@ -0,0 +1,15 @@
+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const http = require('http')
+
+function sendData(req, res) {
+  setTimeout(() => {
+    res.writeHead(200)
+    res.write('Hello, World!')
+    sendData(req, res)
+  }, 1000)
+}
+
+const server = http.createServer((req, res) => {
+  sendData(req, res)
+})
+server.listen(8080)

src/utils/registry-utils.ts

@@ -0,0 +1,5 @@
+import {ITab} from './check-tab'
+
+export function getTabsInfo(tabs: ITab[]): string {
+  return tabs.map((tab) => `#${tab.id} ${tab.title}`).join('\n')
+}

src/utils/tab-registry-factory-test.ts

@@ -7,22 +7,27 @@ function getTab(params: Partial<ITab>): ITab {
   return result as ITab
 }
 
-function mapToUrls(tabs: ITab[]): (string | undefined)[] {
-  return tabs.map(({url}) => url)
+function mapToResult(tabs: ITab[]): Pick<ITab, 'url' | 'id'>[] {
+  return tabs.map(({id, url}) => ({id, url}))
 }
 
 describe(TabRegistryFactory.name, () => {
   describe(`create`, () => {
     it(`creates registry if there are no saved tabs`, () => {
       const openTabs = [
-        getTab({url: 'example'}),
-        getTab({url: 'wikipedia', active: true}),
-        getTab({url: 'stack'}),
+        getTab({id: 1001, url: 'example'}),
+        getTab({id: 1002, url: 'wikipedia', active: true}),
+        getTab({id: 1003, url: 'stack'}),
       ]
       const savedTabs: ITab[] = []
       const registry = TabRegistryFactory.create({numberOfTabsToShow: 5, openTabs, savedTabs})
       const result = registry.getTabs()
-      assert.deepStrictEqual(mapToUrls(result), ['example', 'stack', 'wikipedia'])
+      const expected = [
+        getTab({id: 1001, url: 'example'}),
+        getTab({id: 1003, url: 'stack'}),
+        getTab({id: 1002, url: 'wikipedia'}),
+      ]
+      assert.deepStrictEqual(mapToResult(result), mapToResult(expected))
     })
   })
 
@@ -43,36 +48,95 @@ describe(TabRegistryFactory.name, () => {
     // This answer may provide the solution for the unique tabs between sessions (https://stackoverflow.com/a/14518800/3167855)
     it(`sorts tabs accordingly to saved ones`, () => {
       const openTabs = [
-        getTab({url: 'wikipedia'}),
-        getTab({url: 'stack'}),
-        getTab({url: 'links'}),
-        getTab({url: 'example'}),
-        getTab({url: 'links'}),
+        getTab({id: 1001, url: 'wikipedia'}),
+        getTab({id: 1002, url: 'stack'}),
+        getTab({id: 1003, url: 'links'}),
+        getTab({id: 1004, url: 'example'}),
+        getTab({id: 1005, url: 'links'}),
       ]
       const savedTabs = [
-        getTab({url: 'links'}),
-        getTab({url: 'example'}),
-        getTab({url: 'stack'}),
-        getTab({url: 'wikipedia'}),
-        getTab({url: 'links'}),
+        getTab({id: 2001, url: 'links'}),
+        getTab({id: 2002, url: 'example'}),
+        getTab({id: 2003, url: 'stack'}),
+        getTab({id: 2004, url: 'wikipedia'}),
+        getTab({id: 2005, url: 'links'}),
+      ]
+      const expected = [
+        getTab({id: 1003, url: 'links'}),
+        getTab({id: 1004, url: 'example'}),
+        getTab({id: 1002, url: 'stack'}),
+        getTab({id: 1001, url: 'wikipedia'}),
+        getTab({id: 1005, url: 'links'}),
       ]
       const result = TabRegistryFactory.sortTabs(openTabs, savedTabs)
-      assert.deepStrictEqual(mapToUrls(result), mapToUrls(savedTabs))
+      assert.deepStrictEqual(mapToResult(result), mapToResult(expected))
     })
 
     it(`sorts tabs accordingly to saved ones but prioritizes the active one`, () => {
       const openTabs = [
-        getTab({url: 'wikipedia'}),
-        getTab({url: 'stack', active: true}),
-        getTab({url: 'example'}),
+        getTab({id: 1001, url: 'wikipedia'}),
+        getTab({id: 1002, url: 'stack', active: true}),
+        getTab({id: 1003, url: 'example'}),
+      ]
+      const savedTabs = [
+        getTab({id: 2001, url: 'example'}),
+        getTab({id: 2002, url: 'stack'}),
+        getTab({id: 2003, url: 'wikipedia'}),
+      ]
+      const expected = [
+        getTab({id: 1003, url: 'example'}),
+        getTab({id: 1001, url: 'wikipedia'}),
+        getTab({id: 1002, url: 'stack'}),
+      ]
+      const result = TabRegistryFactory.sortTabs(openTabs, savedTabs)
+      assert.deepStrictEqual(mapToResult(result), mapToResult(expected))
+    })
+
+    it(`sorts tabs accordingly to their creation order`, () => {
+      const openTabs = [
+        getTab({id: 1001, url: 'example', title: 'example 1'}),
+        getTab({id: 1002, url: 'example', title: 'example 2'}),
+        getTab({id: 1003, url: 'wikipedia', active: true}),
       ]
       const savedTabs = [
-        getTab({url: 'example'}),
-        getTab({url: 'stack'}),
-        getTab({url: 'wikipedia'}),
+        getTab({id: 2002, url: 'example', title: 'example 2'}),
+        getTab({id: 2001, url: 'example', title: 'example 1'}),
+        getTab({id: 2003, url: 'wikipedia'}),
+      ]
+      const expected = [
+        getTab({id: 1002, url: 'example', title: 'example 2'}),
+        getTab({id: 1001, url: 'example', title: 'example 1'}),
+        getTab({id: 1003, url: 'wikipedia'}),
+      ]
+      const result = TabRegistryFactory.sortTabs(openTabs, savedTabs)
+      assert.deepStrictEqual(mapToResult(result), mapToResult(expected))
+    })
+
+    it(`prioritizes the most recently active tab`, () => {
+      // Given 2 active tabs in openTabs.
+      // When the tabs are sorted.
+      // Then the last active tab is prioritized.
+
+      // Note: The active tab is not unique in the openTabs array.
+      // It is unique in the window. Because openTabs array contains tabs from all windows,
+      // there may be multiple active tabs.
+      const openTabs = [
+        getTab({id: 1001, url: 'wikipedia', active: true}),
+        getTab({id: 1002, url: 'example', active: true, title: 'example 1'}),
+        getTab({id: 1003, url: 'example', title: 'example 2'}),
+      ]
+      const savedTabs = [
+        getTab({id: 2001, url: 'wikipedia'}),
+        getTab({id: 2003, url: 'example', title: 'example 2'}),
+        getTab({id: 2002, url: 'example', title: 'example 1'}),
+      ]
+      const expected = [
+        getTab({id: 1001, url: 'wikipedia', active: true}),
+        getTab({id: 1003, url: 'example'}),
+        getTab({id: 1002, url: 'example', active: true}),
       ]
       const result = TabRegistryFactory.sortTabs(openTabs, savedTabs)
-      assert.deepStrictEqual(mapToUrls(result), ['example', 'wikipedia', 'stack'])
+      assert.deepStrictEqual(mapToResult(result), mapToResult(expected))
     })
   })
 })

src/utils/tab-registry-factory.ts

@@ -8,6 +8,10 @@ interface ITabRegistryFactoryParams {
   onTabsUpdate?: (tabs: ITab[]) => void
 }
 
+interface ITabWithCreationOrder extends ITab {
+  creationOrder: number
+}
+
 export class TabRegistryFactory {
   static create({
     numberOfTabsToShow,
@@ -24,26 +28,93 @@ export class TabRegistryFactory {
   }
 
   static sortTabs(openTabs: ITab[], savedTabs: ITab[]): ITab[] {
+    const openTabsWithCreationOrder = this.getTabsWithCreationOrder(openTabs)
+    const savedTabsWithCreationOrder = this.getTabsWithCreationOrder(savedTabs)
+    this.sortOpenTabsUsingSimilaritiesInSavedTabs(
+      openTabsWithCreationOrder,
+      savedTabsWithCreationOrder
+    )
+    this.moveActiveTabToTheEnd(openTabsWithCreationOrder)
+    return openTabsWithCreationOrder
+  }
+
+  /**
+   * Goes through the saved tabs and tries to find the corresponding open tab.
+   * If the tab is found then it is moved to the end of the open tabs array.
+   */
+  private static sortOpenTabsUsingSimilaritiesInSavedTabs(
+    openTabs: ITabWithCreationOrder[],
+    savedTabs: ITabWithCreationOrder[]
+  ) {
     savedTabs.forEach((savedTab) => {
-      const similarTabIndex = openTabs.findIndex((openTab) => savedTab.url === openTab.url)
+      const similarTabIndex = this.getSimilarTabIndex(openTabs, savedTab)
       if (similarTabIndex > -1) {
-        const similarTab = openTabs[similarTabIndex]
-        openTabs.splice(similarTabIndex, 1)
-        openTabs.push(similarTab)
+        this.moveItemToTheEnd(openTabs, similarTabIndex)
+      }
+    })
+  }
+
+  /**
+   * Returns the index of the open tab that is similar to the saved tab.
+   * The similarity is determined by the url and the creation order.
+   */
+  private static getSimilarTabIndex(
+    openTabs: ITabWithCreationOrder[],
+    savedTab: ITabWithCreationOrder
+  ): number {
+    let result = -1
+    openTabs.forEach((openTab, index) => {
+      if (openTab.url === savedTab.url && openTab.creationOrder === savedTab.creationOrder) {
+        result = index
       }
     })
-    return openTabs.sort(TabRegistryFactory.activeLast)
+    return result
+  }
+
+  private static moveItemToTheEnd(array: ITab[], itemIndex: number) {
+    console.assert(itemIndex > -1, 'The itemIndex should be greater than -1')
+    console.assert(itemIndex < array.length, 'The itemIndex should be less than the array length')
+    console.assert(array.length > 0, 'The array should not be empty')
+    const similarTab = array[itemIndex]
+    array.splice(itemIndex, 1)
+    array.push(similarTab)
   }
 
-  static activeLast(first: ITab, second: ITab) {
-    const isFirstActive = first.active && !second.active
-    const isSecondActive = !first.active && second.active
-    if (isFirstActive) {
-      return 1
+  private static moveActiveTabToTheEnd(openTabs: ITab[]) {
+    // TODO: Replace with openTabs.findLastIndex((tab) => tab.active)
+    let lastActiveTabIndex = -1
+    for (let i = openTabs.length - 1; i >= 0; i -= 1) {
+      if (openTabs[i].active) {
+        lastActiveTabIndex = i
+        break
+      }
     }
-    if (isSecondActive) {
-      return -1
+    if (lastActiveTabIndex > -1) {
+      this.moveItemToTheEnd(openTabs, lastActiveTabIndex)
     }
-    return 0
+  }
+
+  /**
+   * Adds the creation order to the tabs.
+   * The creation order defines the order in which tabs with the same url were created.
+   *
+   * Creation order algorithm:
+   * - Sort tabs by their ids (lower id means that the tab was created earlier).
+   * - Enumerate tabs with equal urls using the creation order. Cache creation order in idToCreationOrderMap.
+   * - Apply the creation order to the original tabs.
+   */
+  private static getTabsWithCreationOrder(tabs: ITab[]): ITabWithCreationOrder[] {
+    const idToCreationOrderMap = new Map<number, number>()
+    const urlToCreationOrderMap = new Map<string, number>()
+    const sortedTabs = tabs.slice().sort((tab1, tab2) => tab1.id - tab2.id)
+    sortedTabs.forEach((tab) => {
+      const creationOrder = urlToCreationOrderMap.get(tab.url) || 0
+      idToCreationOrderMap.set(tab.id, creationOrder)
+      urlToCreationOrderMap.set(tab.url, creationOrder + 1)
+    })
+    return tabs.map((tab) => ({
+      ...tab,
+      creationOrder: idToCreationOrderMap.get(tab.id) || 0,
+    }))
   }
 }

src/utils/tab-registry.ts

@@ -1,5 +1,6 @@
 import {ITab} from './check-tab'
 import {log} from './logger'
+import {getTabsInfo} from './registry-utils'
 
 interface ITabRegistryOptions {
   tabs: ITab[]
@@ -123,7 +124,7 @@ export default class TabRegistry {
   }
 
   titles() {
-    return this.tabs.map((tab) => `#${tab.id} ${tab.title}`).join(', ')
+    return getTabsInfo(this.tabs)
   }
 
   startInitialization(tab: ITab): ITabInitialization {