From fb6f2009720347b72e36abcea71254200cc44bd4 Mon Sep 17 00:00:00 2001 From: Dmitriy Davydov Date: Wed, 25 Jan 2023 09:30:17 +0400 Subject: [PATCH] Current sorting algorithm doesn't distinct between tabs with equal URLs. (#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 | 14 +++- e2e/utils/long-server-response.js | 15 ++++ src/utils/registry-utils.ts | 5 ++ src/utils/tab-registry-factory-test.ts | 112 +++++++++++++++++++------ src/utils/tab-registry-factory.ts | 97 ++++++++++++++++++--- src/utils/tab-registry.ts | 3 +- 6 files changed, 204 insertions(+), 42 deletions(-) create mode 100644 e2e/utils/long-server-response.js create mode 100644 src/utils/registry-utils.ts diff --git a/e2e/popup-e2e.ts b/e2e/popup-e2e.ts index cbe1424..7df08aa 100644 --- a/e2e/popup-e2e.ts +++ b/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.' ) }) diff --git a/e2e/utils/long-server-response.js b/e2e/utils/long-server-response.js new file mode 100644 index 0000000..40d841d --- /dev/null +++ b/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) diff --git a/src/utils/registry-utils.ts b/src/utils/registry-utils.ts new file mode 100644 index 0000000..77a8f47 --- /dev/null +++ b/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') +} diff --git a/src/utils/tab-registry-factory-test.ts b/src/utils/tab-registry-factory-test.ts index c265161..c1a3c84 100644 --- a/src/utils/tab-registry-factory-test.ts +++ b/src/utils/tab-registry-factory-test.ts @@ -7,22 +7,27 @@ function getTab(params: Partial): ITab { return result as ITab } -function mapToUrls(tabs: ITab[]): (string | undefined)[] { - return tabs.map(({url}) => url) +function mapToResult(tabs: ITab[]): Pick[] { + 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)) }) }) }) diff --git a/src/utils/tab-registry-factory.ts b/src/utils/tab-registry-factory.ts index c88f8bf..31c814f 100644 --- a/src/utils/tab-registry-factory.ts +++ b/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() + const urlToCreationOrderMap = new Map() + 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, + })) } } diff --git a/src/utils/tab-registry.ts b/src/utils/tab-registry.ts index b36bc7e..4ade00b 100644 --- a/src/utils/tab-registry.ts +++ b/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 {