From a4c5062ce2398d2d940d88155210f60f865afde4 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 01:44:57 +0100 Subject: [PATCH 01/19] simplify createNativeBookmarkTree change isNativeBookmarkInToolbarContainer to isNativeBookmarkIdOfToolbarContainer remove the nativeToolbarContainerId parameter from createNativeBookmarkTree utilite isNativeBookmarkIdOfToolbarContainer in createNativeSeparator() --- .../chromium-bookmark.service.ts | 25 ++++++++++--------- .../webext-bookmark.service.ts | 25 ++++++------------- 2 files changed, 21 insertions(+), 29 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 802846c55..60be4c240 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -79,7 +79,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { bookmark: NativeBookmarks.BookmarkTreeNode ): ng.IPromise { // Check if bookmark is in toolbar - return this.isNativeBookmarkInToolbarContainer(bookmark) + return this.isNativeBookmarkIdOfToolbarContainer(bookmark.parentId) .then((inToolbar) => { // Skip process if bookmark is not in toolbar and already native separator if ( @@ -215,18 +215,19 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } createNativeSeparator( - parentId: string, - nativeToolbarContainerId: string + parentId: string ): ng.IPromise { - const newSeparator: NativeBookmarks.CreateDetails = { - parentId, - title: - parentId === nativeToolbarContainerId - ? Globals.Bookmarks.VerticalSeparatorTitle - : Globals.Bookmarks.HorizontalSeparatorTitle, - url: this.platformSvc.getNewTabUrl() - }; - return browser.bookmarks.create(newSeparator).catch((err) => { + return this.isNativeBookmarkIdOfToolbarContainer(parentId).then(inToolbar => { + const newSeparator: NativeBookmarks.CreateDetails = { + parentId, + title: + inToolbar + ? Globals.Bookmarks.VerticalSeparatorTitle + : Globals.Bookmarks.HorizontalSeparatorTitle, + url: this.platformSvc.getNewTabUrl() + }; + return browser.bookmarks.create(newSeparator); + }).catch((err) => { this.logSvc.logInfo('Failed to create native separator'); throw new Exceptions.FailedCreateNativeBookmarksException(undefined, err); }); diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index f595b06cf..81e5ea84c 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -334,14 +334,10 @@ export default abstract class WebExtBookmarkService { abstract createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise; - createNativeBookmarkTree( - parentId: string, - bookmarks: Bookmark[], - nativeToolbarContainerId?: string - ): ng.IPromise { + createNativeBookmarkTree(parentId: string, bookmarks: Bookmark[]): ng.IPromise { let processError: Error; let total = 0; - const createRecursive = (id: string, bookmarksToCreate: Bookmark[] = [], toolbarId: string) => { + const createRecursive = (id: string, bookmarksToCreate: Bookmark[] = []) => { const createChildBookmarksPromises = []; // Create bookmarks at the top level of the supplied array @@ -354,13 +350,11 @@ export default abstract class WebExtBookmarkService { } return this.bookmarkHelperSvc.getBookmarkType(bookmark) === BookmarkType.Separator - ? this.createNativeSeparator(id, toolbarId).then(() => {}) + ? this.createNativeSeparator(id).then(() => {}) : this.createNativeBookmark(id, bookmark.title, bookmark.url).then((newNativeBookmark) => { // If the bookmark has children, recurse if (bookmark.children?.length) { - createChildBookmarksPromises.push( - createRecursive(newNativeBookmark.id, bookmark.children, toolbarId) - ); + createChildBookmarksPromises.push(createRecursive(newNativeBookmark.id, bookmark.children)); } }); }); @@ -374,13 +368,10 @@ export default abstract class WebExtBookmarkService { throw err; }); }; - return createRecursive(parentId, bookmarks, nativeToolbarContainerId).then(() => total); + return createRecursive(parentId, bookmarks).then(() => total); } - abstract createNativeSeparator( - parentId: string, - nativeToolbarContainerId: string - ): ng.IPromise; + abstract createNativeSeparator(parentId: string): ng.IPromise; abstract disableEventListeners(): ng.IPromise; @@ -455,9 +446,9 @@ export default abstract class WebExtBookmarkService { return returnUrl; } - isNativeBookmarkInToolbarContainer(nativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + isNativeBookmarkIdOfToolbarContainer(nativeBookmarkId: string): ng.IPromise { return this.getNativeContainerIds().then((nativeContainerIds) => { - return nativeBookmark.parentId === nativeContainerIds.get(BookmarkContainer.Toolbar); + return nativeBookmarkId === nativeContainerIds.get(BookmarkContainer.Toolbar); }); } From 4dd2ee71ab76fbc39e5803d89f0494a418c128e2 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 01:47:00 +0100 Subject: [PATCH 02/19] ensureContainersExist implementation uses new MandatoryBookmarkContainers enum for list of necessary containers --- src/modules/shared/bookmark/bookmark.enum.ts | 8 +++++++- .../chromium-bookmark/chromium-bookmark.service.ts | 12 +++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/src/modules/shared/bookmark/bookmark.enum.ts b/src/modules/shared/bookmark/bookmark.enum.ts index c7d8a2399..071e00039 100644 --- a/src/modules/shared/bookmark/bookmark.enum.ts +++ b/src/modules/shared/bookmark/bookmark.enum.ts @@ -13,6 +13,12 @@ enum BookmarkContainer { Toolbar = '[xbs] Toolbar' } +const MandatoryBookmarkContainers: Array = [ + BookmarkContainer.Other, + BookmarkContainer.Mobile, + BookmarkContainer.Toolbar +]; + enum BookmarkType { Bookmark = 'bookmark', Container = 'container', @@ -20,4 +26,4 @@ enum BookmarkType { Separator = 'separator' } -export { BookmarkChangeType, BookmarkContainer, BookmarkType }; +export { BookmarkChangeType, BookmarkContainer, BookmarkType, MandatoryBookmarkContainers }; diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 60be4c240..c6197177b 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -2,7 +2,12 @@ import angular from 'angular'; import { Injectable } from 'angular-ts-decorators'; import autobind from 'autobind-decorator'; import { Bookmarks as NativeBookmarks, browser } from 'webextension-polyfill-ts'; -import { BookmarkChangeType, BookmarkContainer, BookmarkType } from '../../../../shared/bookmark/bookmark.enum'; +import { + BookmarkChangeType, + BookmarkContainer, + BookmarkType, + MandatoryBookmarkContainers +} from '../../../../shared/bookmark/bookmark.enum'; import { AddNativeBookmarkChangeData, Bookmark, @@ -277,8 +282,9 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { // Add supported containers const bookmarksToReturn = angular.copy(bookmarks); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarksToReturn, true); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarksToReturn, true); + MandatoryBookmarkContainers.forEach((element) => { + this.bookmarkHelperSvc.getContainer(element, bookmarksToReturn, true); + }); // Return sorted containers return bookmarksToReturn.sort((x, y) => { From 363558c2a6bd5827ca3f71bbe965f77b31641a04 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 02:10:05 +0100 Subject: [PATCH 03/19] cosmetic fix --- .../shared/chromium-bookmark/chromium-bookmark.service.ts | 1 - .../webext/shared/webext-bookmark/webext-bookmark.service.ts | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index c6197177b..3df52aee0 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -461,7 +461,6 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { if (this.isSeparator(bookmark)) { bookmark.type = BookmarkType.Separator; } - return bookmark; }); return nativeBookmarks; } diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 81e5ea84c..ac44b4cb2 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -44,7 +44,7 @@ export default abstract class WebExtBookmarkService { nativeBookmarkEventsQueue: any[] = []; processNativeBookmarkEventsTimeout: ng.IPromise; - unsupportedContainers = []; + unsupportedContainers: BookmarkContainer[] = []; static $inject = [ '$injector', From 3add6fbdb012d0c2dc71b6b8cedeb639170f38ae Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 02:15:21 +0100 Subject: [PATCH 04/19] NativeContainersInfo with platformDefaultBookmarksNodeId --- .../chromium-bookmark.service.ts | 91 ++++++++++++++----- .../firefox-bookmark.service.ts | 5 +- .../webext-bookmark/NativeContainersInfo.ts | 5 + .../webext-bookmark.service.ts | 26 ++++-- 4 files changed, 93 insertions(+), 34 deletions(-) create mode 100644 src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 3df52aee0..835cef416 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -18,13 +18,16 @@ import { import * as Exceptions from '../../../../shared/exception/exception'; import Globals from '../../../../shared/global-shared.constants'; import { WebpageMetadata } from '../../../../shared/global-shared.interface'; +import { NativeContainersInfo } from '../../../shared/webext-bookmark/NativeContainersInfo'; import WebExtBookmarkService from '../../../shared/webext-bookmark/webext-bookmark.service'; @autobind @Injectable('BookmarkService') export default class ChromiumBookmarkService extends WebExtBookmarkService { - otherBookmarksNodeId = '2'; - toolbarBookmarksNodeId = '1'; + otherBookmarksNodeTitle = 'Other bookmarks'; + toolbarBookmarksNodeTitle = 'Bookmarks bar'; + menuBookmarksNodeTitle = 'Menu bookmarks'; + mobileBookmarksNodeTitle = 'Mobile bookmarks'; unsupportedContainers = [BookmarkContainer.Menu, BookmarkContainer.Mobile]; clearNativeBookmarks(): ng.IPromise { @@ -465,13 +468,13 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { return nativeBookmarks; } - getNativeContainerIds(): ng.IPromise> { + getNativeContainerIds(): ng.IPromise { return this.utilitySvc .isSyncEnabled() .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) .then((bookmarks) => { // Initialise container ids object using containers defined in bookmarks - const containerIds = new Map(); + const containerIds = new NativeContainersInfo(); if (!angular.isUndefined(bookmarks)) { bookmarks.forEach((x) => { containerIds.set(x.title as BookmarkContainer, undefined); @@ -481,35 +484,75 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { // Populate container ids return browser.bookmarks.getTree().then((tree) => { // Get the root child nodes - const otherBookmarksNode = tree[0].children.find((x) => { - return x.id === this.otherBookmarksNodeId; + let otherBookmarksNode = tree[0].children.find((x) => { + return x.title === this.otherBookmarksNodeTitle; }); - const toolbarBookmarksNode = tree[0].children.find((x) => { - return x.id === this.toolbarBookmarksNodeId; + let toolbarBookmarksNode = tree[0].children.find((x) => { + return x.title === this.toolbarBookmarksNodeTitle; + }); + let menuBookmarksNode = tree[0].children.find((x) => { + return x.title === this.menuBookmarksNodeTitle; + }); + let mobileBookmarksNode = tree[0].children.find((x) => { + return x.title === this.mobileBookmarksNodeTitle; }); - // Throw an error if a native container node is not found - if (!otherBookmarksNode || !toolbarBookmarksNode) { - if (!otherBookmarksNode) { - this.logSvc.logWarning('Missing container: other bookmarks'); - } - if (!toolbarBookmarksNode) { - this.logSvc.logWarning('Missing container: toolbar bookmarks'); - } + // TODO: improve this logic + const defaultBookmarksNode = otherBookmarksNode || mobileBookmarksNode; + if (!defaultBookmarksNode) { + // coulnd not find a default container to create folders to place other containers in throw new Exceptions.ContainerNotFoundException(); } + // TODO: FINISH THIS! + // HACK!!!! + this.unsupportedContainers = []; + // Check for unsupported containers - const menuBookmarksNode = otherBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Menu; - }); - const mobileBookmarksNode = otherBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Mobile; - }); + if (!otherBookmarksNode) { + this.logSvc.logWarning('Unsupported container: other bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Other); + otherBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Other; + }); + } + if (!toolbarBookmarksNode) { + this.logSvc.logWarning('Unsupported container: toolbar bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Toolbar); + toolbarBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Toolbar; + }); + } + if (!menuBookmarksNode) { + this.logSvc.logWarning('Unsupported container: menu bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Menu); + menuBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Menu; + }); + } + if (!mobileBookmarksNode) { + this.logSvc.logWarning('Unsupported container: mobile bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Mobile); + mobileBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Mobile; + }); + } // Add container ids to result - containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); - containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); + { + // must be always defined! + containerIds.platformDefaultBookmarksNodeId = defaultBookmarksNode.id; + } + if (!angular.isUndefined(otherBookmarksNode)) { + containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); + } + if (!angular.isUndefined(toolbarBookmarksNode)) { + containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); + } if (!angular.isUndefined(menuBookmarksNode)) { containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); } diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index 6d25114a2..d50d7a83d 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -13,6 +13,7 @@ import { import * as Exceptions from '../../../../shared/exception/exception'; import { WebpageMetadata } from '../../../../shared/global-shared.interface'; import WebExtBookmarkService from '../../../shared/webext-bookmark/webext-bookmark.service'; +import { NativeContainersInfo } from "../../../shared/webext-bookmark/NativeContainersInfo"; @autobind @Injectable('BookmarkService') @@ -466,13 +467,13 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - getNativeContainerIds(): ng.IPromise> { + getNativeContainerIds(): ng.IPromise { return this.utilitySvc .isSyncEnabled() .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) .then((bookmarks) => { // Initialise container ids object using containers defined in bookmarks - const containerIds = new Map(); + const containerIds = new NativeContainersInfo(); if (!angular.isUndefined(bookmarks)) { bookmarks.forEach((x) => { containerIds.set(x.title as BookmarkContainer, undefined); diff --git a/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts b/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts new file mode 100644 index 000000000..d5502551f --- /dev/null +++ b/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts @@ -0,0 +1,5 @@ +import { BookmarkContainer } from '../../../shared/bookmark/bookmark.enum'; + +export class NativeContainersInfo extends Map { + platformDefaultBookmarksNodeId: string; +} diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index ac44b4cb2..b6194e5cf 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -27,6 +27,7 @@ import SyncEngineService from '../../../shared/sync/sync-engine/sync-engine.serv import UtilityService from '../../../shared/utility/utility.service'; import { BookmarkIdMapping } from '../bookmark-id-mapper/bookmark-id-mapper.interface'; import BookmarkIdMapperService from '../bookmark-id-mapper/bookmark-id-mapper.service'; +import { NativeContainersInfo } from './NativeContainersInfo'; @autobind export default abstract class WebExtBookmarkService { @@ -429,7 +430,7 @@ export default abstract class WebExtBookmarkService { abstract getNativeBookmarksAsBookmarks(): ng.IPromise; - abstract getNativeContainerIds(): ng.IPromise>; + abstract getNativeContainerIds(): ng.IPromise; getSupportedUrl(url: string): string { if (angular.isUndefined(url ?? undefined)) { @@ -1022,22 +1023,31 @@ export default abstract class WebExtBookmarkService { wasContainerChanged(changedNativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { return this.getNativeContainerIds().then((nativeContainerIds) => { - // If parent is not other bookmarks, no container was changed - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - if ((changedNativeBookmark as NativeBookmarks.BookmarkTreeNode).parentId !== otherBookmarksId) { + // If parent is not platform-default bookmarks, no container was changed + const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; + if (changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId) { return false; } + // Michal Kotoun's note: this would not work before my changes and certainly will not work now! Replacement bellow. // If any native container ids are undefined, container was removed - if (Array.from(nativeContainerIds.values()).includes(undefined)) { + // if (Array.from(nativeContainerIds.values()).includes(undefined)) { + // return true; + // } + // If its an unsupported container bookmark native node && its native container ids is undefined, container was removed + const titleAsBookmarkContainer = changedNativeBookmark.title as BookmarkContainer; + const isUnsupportedContainerBookmark = this.unsupportedContainers.includes(titleAsBookmarkContainer); + if (isUnsupportedContainerBookmark && angular.isUndefined(nativeContainerIds.get(titleAsBookmarkContainer))) { return true; } return browser.bookmarks - .getChildren(otherBookmarksId) + .getChildren(platformDefaultBookmarksNodeId) .then((children) => { - // Get all native bookmarks in other bookmarks that are unsupported containers and check for duplicates - const containers = children.filter((x) => this.unsupportedContainers.includes(x.title)).map((x) => x.title); + // Get all native bookmarks - in platform-default bookmarks node - that are unsupported containers and check for duplicates + const containers = children + .filter((x) => this.unsupportedContainers.includes(x.title as BookmarkContainer)) + .map((x) => x.title); return containers.length !== new Set(containers).size; }) .catch((err) => { From 5fa2f08f7ce2c80260e99688c837989d774fc103 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 02:44:25 +0100 Subject: [PATCH 05/19] simple uses of platformDefaultBookmarksNodeId --- .../shared/webext-bookmark/webext-bookmark.service.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index b6194e5cf..8ac9e1e03 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -284,8 +284,8 @@ export default abstract class WebExtBookmarkService { countNativeContainersBeforeIndex(parentId: string, index: number): ng.IPromise { // Get native container ids return this.getNativeContainerIds().then((nativeContainerIds) => { - // No containers to adjust for if parent is not other bookmarks - if (parentId !== nativeContainerIds.get(BookmarkContainer.Other)) { + // No containers to adjust for if parent is not platform-default bookmarks node + if (parentId !== nativeContainerIds.platformDefaultBookmarksNodeId) { return 0; } @@ -609,11 +609,11 @@ export default abstract class WebExtBookmarkService { } processChangeTypeAddOnNativeBookmarks(id: number, createInfo: BookmarkMetadata): ng.IPromise { - // Create native bookmark in other bookmarks container + // Create native bookmark in platform default bookmarks container return this.getNativeContainerIds() .then((nativeContainerIds) => { - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - return this.createNativeBookmark(otherBookmarksId, createInfo.title, createInfo.url); + const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; + return this.createNativeBookmark(platformDefaultBookmarksNodeId, createInfo.title, createInfo.url); }) .then((newNativeBookmark) => { // Add id mapping for new bookmark From 8a4fdaa2bb1d4aac2e1e812b821c8e2e3e2b23fe Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 02:45:23 +0100 Subject: [PATCH 06/19] Complex uses of platformDefaultBookmarksNodeId + generification of webext- and chromium- bookmark.service --- .../chromium-bookmark.service.ts | 372 +++++++----------- .../webext-bookmark.service.ts | 143 +++---- 2 files changed, 188 insertions(+), 327 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 835cef416..6e402d5ee 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -34,49 +34,49 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { // Get native container ids return this.getNativeContainerIds() .then((nativeContainerIds) => { - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Clear other bookmarks - const clearOthers = browser.bookmarks - .getChildren(otherBookmarksId) - .then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }) - .catch((err) => { - this.logSvc.logWarning('Error clearing other bookmarks'); - throw err; - }); + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const clearPromises = []; + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName = BookmarkContainer[containerEnumVal]; + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); - // Clear bookmarks toolbar if enabled - const clearToolbar = this.$q((resolve, reject) => { - return this.settingsSvc - .syncBookmarksToolbar() - .then((syncBookmarksToolbar) => { + if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { this.logSvc.logInfo('Not clearing toolbar'); - return resolve(); + continue; } - return browser.bookmarks.getChildren(toolbarBookmarksId).then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }); - }) - .then(resolve) - .catch((err) => { - this.logSvc.logWarning('Error clearing bookmarks toolbar'); - reject(err); - }); - }); + } - return this.$q.all([clearOthers, clearToolbar]).then(() => {}); + // Clear bookmarks of that type + if (nativeBookmarkNodeId) { + const clearPromise = browser.bookmarks + .getChildren(nativeBookmarkNodeId) + .then((children) => { + // Do not remove the bookmark-folders that server as mount-point for unsupported containers + if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + children = children.filter( + (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + ); + } + return this.$q.all( + children.map((child) => { + return this.removeNativeBookmarks(child.id); + }) + ); + }) + .catch((err) => { + this.logSvc.logWarning(`Error clearing ${containerEnumVal} bookmarks`); + throw err; + }); + clearPromises.push(clearPromise); + } + } + + return this.$q.all(clearPromises).then(() => {}); + }); }) .catch((err) => { throw new Exceptions.FailedRemoveNativeBookmarksException(undefined, err); @@ -135,110 +135,67 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise { - // Get containers - const menuContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Menu, bookmarks); - const mobileContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Mobile, bookmarks); - const otherContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarks); - const toolbarContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarks); - // Get native container ids return this.getNativeContainerIds() .then((nativeContainerIds) => { - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Populate menu bookmarks in other bookmarks - let populateMenu = this.$q.resolve(0); - if (menuContainer) { - populateMenu = browser.bookmarks - .getSubTree(otherBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(otherBookmarksId, [menuContainer], toolbarBookmarksId); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating bookmarks menu.'); - throw err; - }); - } - - // Populate mobile bookmarks in other bookmarks - let populateMobile = this.$q.resolve(0); - if (mobileContainer) { - populateMobile = browser.bookmarks - .getSubTree(otherBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(otherBookmarksId, [mobileContainer], toolbarBookmarksId); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating mobile bookmarks.'); - throw err; - }); - } - - // Populate other bookmarks - let populateOther = this.$q.resolve(0); - if (otherContainer) { - populateOther = browser.bookmarks - .getSubTree(otherBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(otherBookmarksId, otherContainer.children, toolbarBookmarksId); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating other bookmarks.'); - throw err; - }); - } - - // Populate bookmarks toolbar if enabled - const populateToolbar = this.$q((resolve, reject) => { - if (!toolbarContainer) { - return resolve(0); - } - return this.settingsSvc - .syncBookmarksToolbar() - .then((syncBookmarksToolbar) => { + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const populatePromises = []; + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName = BookmarkContainer[containerEnumVal]; + // Get container + const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks); + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { this.logSvc.logInfo('Not populating toolbar'); - return resolve(); + continue; } - return browser.bookmarks.getSubTree(toolbarBookmarksId).then(() => { - return this.createNativeBookmarkTree(toolbarBookmarksId, toolbarContainer.children); - }); - }) - .then(resolve) - .catch((err) => { - this.logSvc.logInfo('Error populating bookmarks toolbar.'); - reject(err); - }); - }); + } + + // Populate bookmarks for the container + if (container) { + const populatePromise = browser.bookmarks + .getSubTree(nativeBookmarkNodeId) + .then(() => { + return this.createNativeBookmarkTree(nativeBookmarkNodeId, container.children); + }) + .catch((err) => { + this.logSvc.logInfo(`Error populating ${containerEnumVal}.`); + throw err; + }); + populatePromises.push(populatePromise); + } + } - return this.$q.all([populateMenu, populateMobile, populateOther, populateToolbar]); + return this.$q.all(populatePromises); + }); }) .then((totals) => { // Move native unsupported containers into the correct order return this.reorderUnsupportedContainers().then(() => { - return totals.filter(Boolean).reduce((a, b) => a + b, 0); + return totals.reduce((a, b) => a + b, 0); }); }); } - createNativeSeparator( - parentId: string - ): ng.IPromise { - return this.isNativeBookmarkIdOfToolbarContainer(parentId).then(inToolbar => { - const newSeparator: NativeBookmarks.CreateDetails = { - parentId, - title: - inToolbar - ? Globals.Bookmarks.VerticalSeparatorTitle - : Globals.Bookmarks.HorizontalSeparatorTitle, - url: this.platformSvc.getNewTabUrl() - }; - return browser.bookmarks.create(newSeparator); - }).catch((err) => { - this.logSvc.logInfo('Failed to create native separator'); - throw new Exceptions.FailedCreateNativeBookmarksException(undefined, err); - }); + createNativeSeparator(parentId: string): ng.IPromise { + return this.isNativeBookmarkIdOfToolbarContainer(parentId) + .then((inToolbar) => { + const newSeparator: NativeBookmarks.CreateDetails = { + parentId, + title: inToolbar ? Globals.Bookmarks.VerticalSeparatorTitle : Globals.Bookmarks.HorizontalSeparatorTitle, + url: this.platformSvc.getNewTabUrl() + }; + return browser.bookmarks.create(newSeparator); + }) + .catch((err) => { + this.logSvc.logInfo('Failed to create native separator'); + throw new Exceptions.FailedCreateNativeBookmarksException(undefined, err); + }); } disableEventListeners(): ng.IPromise { @@ -302,125 +259,81 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } getNativeBookmarksAsBookmarks(): ng.IPromise { - let allNativeBookmarks = []; + let allNativeBookmarks: NativeBookmarks.BookmarkTreeNode[] = []; // Get native container ids return this.getNativeContainerIds().then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Get menu bookmarks - const getMenuBookmarks = - menuBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(menuBookmarksId).then((subTree) => { - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( - this.getNativeBookmarksWithSeparators(subTree[0].children) - ); - }); - - // Get mobile bookmarks - const getMobileBookmarks = - mobileBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(mobileBookmarksId).then((subTree) => { - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( - this.getNativeBookmarksWithSeparators(subTree[0].children) - ); - }); - - // Get other bookmarks - const getOtherBookmarks = - otherBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(otherBookmarksId).then((subTree) => { - const otherBookmarks = subTree[0]; - if (otherBookmarks.children.length === 0) { - return; - } + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const getBookmarkPromises = new Array]>>(); + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName: BookmarkContainer = BookmarkContainer[containerEnumVal]; + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { + if (!syncBookmarksToolbar) { + // skip + continue; + } + } - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(otherBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); + // Map bookmarks of that type + if (nativeBookmarkNodeId) { + const getBookmarkPromise: Promise<[BookmarkContainer, Array]> = browser.bookmarks + .getSubTree(nativeBookmarkNodeId) + .then((subTree) => { + const bookmarksNode = subTree[0]; - // Remove any unsupported container folders present - const bookmarksWithoutContainers = this.bookmarkHelperSvc - .getNativeBookmarksAsBookmarks(this.getNativeBookmarksWithSeparators(otherBookmarks.children)) - .filter((x) => { - return !this.unsupportedContainers.find((y) => { - return y === x.title; - }); - }); - return bookmarksWithoutContainers; - }); + if (!bookmarksNode.children?.length) { + return [containerName, [] as Bookmark[]]; + } - // Get toolbar bookmarks if enabled - const getToolbarBookmarks = - toolbarBookmarksId === undefined - ? this.$q.resolve(undefined) - : browser.bookmarks.getSubTree(toolbarBookmarksId).then((results) => { - const toolbarBookmarks = results[0]; - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - if (syncBookmarksToolbar && toolbarBookmarks.children.length > 0) { - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(toolbarBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( - this.getNativeBookmarksWithSeparators(toolbarBookmarks.children) + let bookmarksNodeChildren: NativeBookmarks.BookmarkTreeNode[]; + // Skip over any unsupported container mount-point bookmark folders present, + // if we are now "in" the platform-default bookmark node. + // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry + if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + bookmarksNodeChildren = bookmarksNode.children.filter( + (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) ); + } else { + bookmarksNodeChildren = bookmarksNode.children; } - }); - }); - return this.$q - .all([getMenuBookmarks, getMobileBookmarks, getOtherBookmarks, getToolbarBookmarks]) - .then((results) => { - const menuBookmarks = results[0]; - const mobileBookmarks = results[1]; - const otherBookmarks = results[2]; - const toolbarBookmarks = results[3]; - const bookmarks: Bookmark[] = []; + // Add all native bookmarks (except the "unsupported containers" mount-point folders) into flat array + this.bookmarkHelperSvc.eachBookmark(bookmarksNodeChildren, (bookmark) => { + allNativeBookmarks.push(bookmark); + }); - // Add other container if bookmarks present - const otherContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarks, true); - if (otherBookmarks?.length > 0) { - otherContainer.children = otherBookmarks; - } + // Return all native bookmarks (except the "unsupported containers" mount-point folders) + // converted to "our" bookmarks. + const convertedBookmarks = this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( + this.getNativeBookmarksWithSeparators(bookmarksNodeChildren) + ); + return [containerName, convertedBookmarks]; + }); - // Add toolbar container if bookmarks present - const toolbarContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarks, true); - if (toolbarBookmarks?.length > 0) { - toolbarContainer.children = toolbarBookmarks; + getBookmarkPromises.push(getBookmarkPromise); } + } - // Add menu container if bookmarks present - let menuContainer: Bookmark; - if (menuBookmarksId !== undefined) { - menuContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Menu, bookmarks, true); - if (menuBookmarks?.length > 0) { - menuContainer.children = menuBookmarks; - } - } + return this.$q.all(getBookmarkPromises).then((containerBookmarksPairArray) => { + const bookmarks: Bookmark[] = []; - // Add mobile container if bookmarks present - let mobileContainer: Bookmark; - if (mobileBookmarksId !== undefined) { - mobileContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Mobile, bookmarks, true); - if (mobileBookmarks?.length > 0) { - mobileContainer.children = mobileBookmarks; - } - } + containerBookmarksPairArray.forEach((tuple) => { + const [containerName, convertedBookmarks] = tuple; - // Filter containers from flat array of bookmarks - [otherContainer, toolbarContainer, menuContainer, mobileContainer].forEach((container) => { - if (!container) { - return; + const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks, true); + if (convertedBookmarks.length > 0) { + container.children = convertedBookmarks; } + // Michal Kotoun's note: this is probably a no-op, since we only added to allNativeBookmarks: + // 1) children of non-default containers + // 2) children of default container except for virtual/unsupportedContainers + // Filter containers from flat array of bookmarks allNativeBookmarks = allNativeBookmarks.filter((bookmark) => { return bookmark.title !== container.title; }); @@ -453,6 +366,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { return bookmarks; }); + }); }); } diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 8ac9e1e03..1b7150698 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -123,94 +123,56 @@ export default abstract class WebExtBookmarkService { // Get native container ids return this.getNativeContainerIds() .then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Map menu bookmarks - const getMenuBookmarks = - menuBookmarksId == null - ? this.$q.resolve([] as BookmarkIdMapping[]) - : browser.bookmarks.getSubTree(menuBookmarksId).then((subTree) => { - const menuBookmarks = subTree[0]; - if (!menuBookmarks.children?.length) { - return [] as BookmarkIdMapping[]; - } - - // Map ids between nodes and synced container children - const menuBookmarksContainer = bookmarks.find((x) => { - return x.title === BookmarkContainer.Menu; - }); - return menuBookmarksContainer?.children?.length - ? mapIds(menuBookmarks.children, menuBookmarksContainer.children) - : ([] as BookmarkIdMapping[]); - }); + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const getBookmarkPromises = new Array>(); + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName = BookmarkContainer[containerEnumVal]; + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { + if (!syncBookmarksToolbar) { + this.logSvc.logInfo('Not mapping toolbar'); + continue; + } + } - // Map mobile bookmarks - const getMobileBookmarks = - mobileBookmarksId == null - ? this.$q.resolve([] as BookmarkIdMapping[]) - : browser.bookmarks.getSubTree(mobileBookmarksId).then((subTree) => { - const mobileBookmarks = subTree[0]; - if (!mobileBookmarks.children?.length) { + // Map bookmarks of that type + if (nativeBookmarkNodeId) { + const getBookmarkPromise = browser.bookmarks.getSubTree(nativeBookmarkNodeId).then((subTree) => { + const bookmarksNode = subTree[0]; + if (!bookmarksNode.children?.length) { return [] as BookmarkIdMapping[]; } - // Map ids between nodes and synced container children - const mobileBookmarksContainer = bookmarks.find((x) => { - return x.title === BookmarkContainer.Mobile; - }); - return mobileBookmarksContainer?.children?.length - ? mapIds(mobileBookmarks.children, mobileBookmarksContainer.children) - : ([] as BookmarkIdMapping[]); - }); - - // Map other bookmarks - const getOtherBookmarks = - otherBookmarksId == null - ? this.$q.resolve([] as BookmarkIdMapping[]) - : browser.bookmarks.getSubTree(otherBookmarksId).then((subTree) => { - const otherBookmarks = subTree[0]; - if (!otherBookmarks.children?.length) { - return [] as BookmarkIdMapping[]; + let bookmarksNodeChildren: NativeBookmarks.BookmarkTreeNode[]; + // Skip over any unsupported container mount-point bookmark folders present, + // if we are now "in" the platform-default bookmark node + // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry + if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + bookmarksNodeChildren = bookmarksNode.children.filter( + (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + ); + } else { + bookmarksNodeChildren = bookmarksNode.children; } - // Remove any unsupported container folders present - const nodes = otherBookmarks.children.filter((x) => !this.unsupportedContainers.includes(x.title)); - // Map ids between nodes and synced container children - const otherBookmarksContainer = bookmarks.find((x) => { - return x.title === BookmarkContainer.Other; + const container = bookmarks.find((x) => { + return x.title === containerName; }); - return otherBookmarksContainer?.children?.length - ? mapIds(nodes, otherBookmarksContainer.children) + return container?.children?.length + ? mapIds(bookmarksNodeChildren, container.children) : ([] as BookmarkIdMapping[]); }); + getBookmarkPromises.push(getBookmarkPromise); + } + } - // Map toolbar bookmarks if enabled - const getToolbarBookmarks = - toolbarBookmarksId == null - ? this.$q.resolve([] as BookmarkIdMapping[]) - : browser.bookmarks.getSubTree(toolbarBookmarksId).then((results) => { - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - const toolbarBookmarks = results[0]; - - if (!syncBookmarksToolbar || !toolbarBookmarks.children?.length) { - return [] as BookmarkIdMapping[]; - } - - // Map ids between nodes and synced container children - const toolbarBookmarksContainer = bookmarks.find((x) => { - return x.title === BookmarkContainer.Toolbar; - }); - return toolbarBookmarksContainer?.children?.length - ? mapIds(toolbarBookmarks.children, toolbarBookmarksContainer.children) - : ([] as BookmarkIdMapping[]); - }); - }); - - return this.$q.all([getMenuBookmarks, getMobileBookmarks, getOtherBookmarks, getToolbarBookmarks]); + return this.$q.all(getBookmarkPromises); + }); }) .then((results) => { // Combine all mappings @@ -381,28 +343,13 @@ export default abstract class WebExtBookmarkService { abstract ensureContainersExist(bookmarks: Bookmark[]): Bookmark[]; getContainerNameFromNativeId(nativeBookmarkId: string): ng.IPromise { - return this.getNativeContainerIds().then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - const nativeContainers = [ - { nativeId: otherBookmarksId, containerName: BookmarkContainer.Other }, - { nativeId: toolbarBookmarksId, containerName: BookmarkContainer.Toolbar } - ]; - - if (menuBookmarksId) { - nativeContainers.push({ nativeId: menuBookmarksId, containerName: BookmarkContainer.Menu }); - } + if (angular.isUndefined(nativeBookmarkId)) return this.$q.resolve(''); - if (mobileBookmarksId) { - nativeContainers.push({ nativeId: mobileBookmarksId, containerName: BookmarkContainer.Mobile }); - } - - // Check if the native bookmark id resolves to a container - const result = nativeContainers.find((x) => x.nativeId === nativeBookmarkId); - return result ? result.containerName : ''; + return this.getNativeContainerIds().then((nativeContainerIds) => { + for (const [nativeBookmarkNodeId, containerName] of nativeContainerIds.entries()) { + if (nativeBookmarkNodeId === nativeBookmarkId) return containerName; + }; + return ''; }); } From 143ea824d4f9b0969137bc9a3cdf253942174690 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 04:44:41 +0100 Subject: [PATCH 07/19] More notes --- .../webext/shared/webext-bookmark/webext-bookmark.service.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 1b7150698..3bb33481d 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -473,6 +473,7 @@ export default abstract class WebExtBookmarkService { return container.id as number; } + // Michal Kotoun's notes: what is the use-case for this??? If getContainerNameFromNativeId() can't return a result, I doubt this would either... // Get the synced parent id from id mappings and retrieve the synced parent bookmark return this.bookmarkIdMapperSvc.get(changeData.nativeBookmark.parentId).then((idMapping) => { if (!idMapping) { From c42d27f28003953f5c31419fcce4d5c63f67b8c2 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 05:18:47 +0100 Subject: [PATCH 08/19] Create mount-point bookmarks for unsupported containers in createNativeBookmarksFromBookmarks this functionality was originally hard-coded for menu/mobile containers and I forgot to implement it --- .../chromium-bookmark.service.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 6e402d5ee..f85b9e5a5 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -158,10 +158,22 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { // Populate bookmarks for the container if (container) { + let parentNodeId: string; + let childrenToCreate: Bookmark[]; + if (nativeBookmarkNodeId) + { + parentNodeId = nativeBookmarkNodeId; + childrenToCreate = container.children; + } + else + { // there is no nativeContainerId -> it must be one of unsupportedContainers -> create it now + parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; + childrenToCreate = [container]; + } const populatePromise = browser.bookmarks - .getSubTree(nativeBookmarkNodeId) + .getSubTree(parentNodeId) .then(() => { - return this.createNativeBookmarkTree(nativeBookmarkNodeId, container.children); + return this.createNativeBookmarkTree(parentNodeId, childrenToCreate); }) .catch((err) => { this.logSvc.logInfo(`Error populating ${containerEnumVal}.`); @@ -419,6 +431,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } // TODO: FINISH THIS! + // is related to createNativeBookmarksFromBookmarks // HACK!!!! this.unsupportedContainers = []; From d1813a2e0f1c32f793f96937e0b4a1e3d23ace22 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 14:17:22 +0100 Subject: [PATCH 09/19] Unify firefox interface with webext- and chromium- --- .../shared/firefox-bookmark/firefox-bookmark.service.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index d50d7a83d..7993cd579 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -530,7 +530,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { super.processNativeBookmarkEventsQueue(); } - syncNativeBookmarkChanged(id: string): ng.IPromise { + syncNativeBookmarkChanged(id?: string): ng.IPromise { // Retrieve full bookmark info return browser.bookmarks.getSubTree(id).then((results) => { const changedBookmark = results[0]; @@ -549,7 +549,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - syncNativeBookmarkCreated(id: string, nativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + syncNativeBookmarkCreated(id?: string, nativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { // Create change info const data: AddNativeBookmarkChangeData = { nativeBookmark @@ -583,7 +583,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - syncNativeBookmarkMoved(id: string, moveInfo: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise { + syncNativeBookmarkMoved(id?: string, moveInfo?: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise { // Create change info const data: MoveNativeBookmarkChangeData = { ...moveInfo, From b29ad13164ea1d1b285d4a57b2330fcf02336ef4 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 15:22:50 +0100 Subject: [PATCH 10/19] Share the refactored chromium code via webext --- .../chromium-bookmark.service.ts | 229 +----------- .../firefox-bookmark.service.ts | 334 +----------------- .../webext-bookmark.service.ts | 233 +++++++++++- 3 files changed, 237 insertions(+), 559 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index f85b9e5a5..b9c1c6c3f 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -30,59 +30,6 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { mobileBookmarksNodeTitle = 'Mobile bookmarks'; unsupportedContainers = [BookmarkContainer.Menu, BookmarkContainer.Mobile]; - clearNativeBookmarks(): ng.IPromise { - // Get native container ids - return this.getNativeContainerIds() - .then((nativeContainerIds) => { - // Get whether syncBookmarksToolbar - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - const clearPromises = []; - - for (const containerEnumVal of Object.keys(BookmarkContainer)) { - const containerName = BookmarkContainer[containerEnumVal]; - // Get native bookmark node id - const nativeBookmarkNodeId = nativeContainerIds.get(containerName); - - if (containerName === BookmarkContainer.Toolbar) { - if (!syncBookmarksToolbar) { - this.logSvc.logInfo('Not clearing toolbar'); - continue; - } - } - - // Clear bookmarks of that type - if (nativeBookmarkNodeId) { - const clearPromise = browser.bookmarks - .getChildren(nativeBookmarkNodeId) - .then((children) => { - // Do not remove the bookmark-folders that server as mount-point for unsupported containers - if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { - children = children.filter( - (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) - ); - } - return this.$q.all( - children.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }) - .catch((err) => { - this.logSvc.logWarning(`Error clearing ${containerEnumVal} bookmarks`); - throw err; - }); - clearPromises.push(clearPromise); - } - } - - return this.$q.all(clearPromises).then(() => {}); - }); - }) - .catch((err) => { - throw new Exceptions.FailedRemoveNativeBookmarksException(undefined, err); - }); - } - convertNativeBookmarkToSeparator( bookmark: NativeBookmarks.BookmarkTreeNode ): ng.IPromise { @@ -134,66 +81,6 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { }); } - createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise { - // Get native container ids - return this.getNativeContainerIds() - .then((nativeContainerIds) => { - // Get whether syncBookmarksToolbar - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - const populatePromises = []; - - for (const containerEnumVal of Object.keys(BookmarkContainer)) { - const containerName = BookmarkContainer[containerEnumVal]; - // Get container - const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks); - // Get native bookmark node id - const nativeBookmarkNodeId = nativeContainerIds.get(containerName); - - if (containerName === BookmarkContainer.Toolbar) { - if (!syncBookmarksToolbar) { - this.logSvc.logInfo('Not populating toolbar'); - continue; - } - } - - // Populate bookmarks for the container - if (container) { - let parentNodeId: string; - let childrenToCreate: Bookmark[]; - if (nativeBookmarkNodeId) - { - parentNodeId = nativeBookmarkNodeId; - childrenToCreate = container.children; - } - else - { // there is no nativeContainerId -> it must be one of unsupportedContainers -> create it now - parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; - childrenToCreate = [container]; - } - const populatePromise = browser.bookmarks - .getSubTree(parentNodeId) - .then(() => { - return this.createNativeBookmarkTree(parentNodeId, childrenToCreate); - }) - .catch((err) => { - this.logSvc.logInfo(`Error populating ${containerEnumVal}.`); - throw err; - }); - populatePromises.push(populatePromise); - } - } - - return this.$q.all(populatePromises); - }); - }) - .then((totals) => { - // Move native unsupported containers into the correct order - return this.reorderUnsupportedContainers().then(() => { - return totals.reduce((a, b) => a + b, 0); - }); - }); - } - createNativeSeparator(parentId: string): ng.IPromise { return this.isNativeBookmarkIdOfToolbarContainer(parentId) .then((inToolbar) => { @@ -247,6 +134,9 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { }); } + // TODO: unify with firefox somehow? + // probably auto-detect all supported containers in getNativeContainer (maybe call it explicitely for detection?) + // and then ensure that all supported containers are created ensureContainersExist(bookmarks: Bookmark[]): Bookmark[] { if (angular.isUndefined(bookmarks)) { return; @@ -270,118 +160,6 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { }); } - getNativeBookmarksAsBookmarks(): ng.IPromise { - let allNativeBookmarks: NativeBookmarks.BookmarkTreeNode[] = []; - - // Get native container ids - return this.getNativeContainerIds().then((nativeContainerIds) => { - // Get whether syncBookmarksToolbar - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - const getBookmarkPromises = new Array]>>(); - - for (const containerEnumVal of Object.keys(BookmarkContainer)) { - const containerName: BookmarkContainer = BookmarkContainer[containerEnumVal]; - // Get native bookmark node id - const nativeBookmarkNodeId = nativeContainerIds.get(containerName); - - if (containerName === BookmarkContainer.Toolbar) { - if (!syncBookmarksToolbar) { - // skip - continue; - } - } - - // Map bookmarks of that type - if (nativeBookmarkNodeId) { - const getBookmarkPromise: Promise<[BookmarkContainer, Array]> = browser.bookmarks - .getSubTree(nativeBookmarkNodeId) - .then((subTree) => { - const bookmarksNode = subTree[0]; - - if (!bookmarksNode.children?.length) { - return [containerName, [] as Bookmark[]]; - } - - let bookmarksNodeChildren: NativeBookmarks.BookmarkTreeNode[]; - // Skip over any unsupported container mount-point bookmark folders present, - // if we are now "in" the platform-default bookmark node. - // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry - if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { - bookmarksNodeChildren = bookmarksNode.children.filter( - (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) - ); - } else { - bookmarksNodeChildren = bookmarksNode.children; - } - - // Add all native bookmarks (except the "unsupported containers" mount-point folders) into flat array - this.bookmarkHelperSvc.eachBookmark(bookmarksNodeChildren, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - - // Return all native bookmarks (except the "unsupported containers" mount-point folders) - // converted to "our" bookmarks. - const convertedBookmarks = this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( - this.getNativeBookmarksWithSeparators(bookmarksNodeChildren) - ); - return [containerName, convertedBookmarks]; - }); - - getBookmarkPromises.push(getBookmarkPromise); - } - } - - return this.$q.all(getBookmarkPromises).then((containerBookmarksPairArray) => { - const bookmarks: Bookmark[] = []; - - containerBookmarksPairArray.forEach((tuple) => { - const [containerName, convertedBookmarks] = tuple; - - const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks, true); - if (convertedBookmarks.length > 0) { - container.children = convertedBookmarks; - } - - // Michal Kotoun's note: this is probably a no-op, since we only added to allNativeBookmarks: - // 1) children of non-default containers - // 2) children of default container except for virtual/unsupportedContainers - // Filter containers from flat array of bookmarks - allNativeBookmarks = allNativeBookmarks.filter((bookmark) => { - return bookmark.title !== container.title; - }); - }); - - // Sort by date added asc - allNativeBookmarks = allNativeBookmarks.sort((x, y) => { - return x.dateAdded - y.dateAdded; - }); - - // Iterate native bookmarks to add unique bookmark ids in correct order - allNativeBookmarks.forEach((nativeBookmark) => { - this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { - if ( - !bookmark.id && - ((!nativeBookmark.url && bookmark.title === nativeBookmark.title) || - (nativeBookmark.url && bookmark.url === nativeBookmark.url)) - ) { - bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); - } - }); - }); - - // Find and fix any bookmarks missing ids - this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { - if (!bookmark.id) { - bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); - } - }); - - return bookmarks; - }); - }); - }); - } - getNativeBookmarksWithSeparators( nativeBookmarks: NativeBookmarks.BookmarkTreeNode[] ): NativeBookmarks.BookmarkTreeNode[] { @@ -394,6 +172,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { return nativeBookmarks; } + // TODO: unify/share more code with firefox getNativeContainerIds(): ng.IPromise { return this.utilitySvc .isSyncEnabled() diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index 7993cd579..f47df2a82 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -20,183 +20,6 @@ import { NativeContainersInfo } from "../../../shared/webext-bookmark/NativeCont export default class FirefoxBookmarkService extends WebExtBookmarkService { unsupportedContainers = []; - clearNativeBookmarks(): ng.IPromise { - // Get native container ids - return this.getNativeContainerIds() - .then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Clear menu bookmarks - const clearMenu = browser.bookmarks - .getChildren(menuBookmarksId) - .then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }) - .catch((err) => { - this.logSvc.logWarning('Error clearing bookmarks menu'); - throw err; - }); - - // Clear mobile bookmarks - const clearMobile = browser.bookmarks - .getChildren(mobileBookmarksId) - .then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }) - .catch((err) => { - this.logSvc.logWarning('Error clearing mobile bookmarks'); - throw err; - }); - - // Clear other bookmarks - const clearOthers = browser.bookmarks - .getChildren(otherBookmarksId) - .then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }) - .catch((err) => { - this.logSvc.logWarning('Error clearing other bookmarks'); - throw err; - }); - - // Clear bookmarks toolbar if enabled - const clearToolbar = this.$q((resolve, reject) => { - return this.settingsSvc - .syncBookmarksToolbar() - .then((syncBookmarksToolbar) => { - if (!syncBookmarksToolbar) { - this.logSvc.logInfo('Not clearing toolbar'); - return resolve(); - } - return browser.bookmarks.getChildren(toolbarBookmarksId).then((results) => { - return this.$q.all( - results.map((child) => { - return this.removeNativeBookmarks(child.id); - }) - ); - }); - }) - .then(resolve) - .catch((err) => { - this.logSvc.logWarning('Error clearing bookmarks toolbar'); - reject(err); - }); - }); - - return this.$q.all([clearMenu, clearMobile, clearOthers, clearToolbar]).then(() => {}); - }) - .catch((err) => { - throw new Exceptions.FailedRemoveNativeBookmarksException(undefined, err); - }); - } - - createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise { - // Get containers - const menuContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Menu, bookmarks); - const mobileContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Mobile, bookmarks); - const otherContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarks); - const toolbarContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarks); - - // Get native container ids - return this.getNativeContainerIds() - .then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Populate menu bookmarks - let populateMenu = this.$q.resolve(0); - if (menuContainer) { - populateMenu = browser.bookmarks - .getSubTree(menuBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(menuBookmarksId, menuContainer.children); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating bookmarks menu.'); - throw err; - }); - } - - // Populate mobile bookmarks - let populateMobile = this.$q.resolve(0); - if (mobileContainer) { - populateMobile = browser.bookmarks - .getSubTree(mobileBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(mobileBookmarksId, mobileContainer.children); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating mobile bookmarks.'); - throw err; - }); - } - - // Populate other bookmarks - let populateOther = this.$q.resolve(0); - if (otherContainer) { - populateOther = browser.bookmarks - .getSubTree(otherBookmarksId) - .then(() => { - return this.createNativeBookmarkTree(otherBookmarksId, otherContainer.children); - }) - .catch((err) => { - this.logSvc.logInfo('Error populating other bookmarks.'); - throw err; - }); - } - - // Populate bookmarks toolbar if enabled - const populateToolbar = this.$q((resolve, reject) => { - if (!toolbarContainer) { - return resolve(0); - } - - return this.settingsSvc - .syncBookmarksToolbar() - .then((syncBookmarksToolbar) => { - if (!syncBookmarksToolbar) { - this.logSvc.logInfo('Not populating toolbar'); - return resolve(); - } - - return browser.bookmarks.getSubTree(toolbarBookmarksId).then(() => { - return this.createNativeBookmarkTree(toolbarBookmarksId, toolbarContainer.children); - }); - }) - .then(resolve) - .catch((err) => { - this.logSvc.logInfo('Error populating bookmarks toolbar.'); - reject(err); - }); - }); - - return this.$q.all([populateMenu, populateMobile, populateOther, populateToolbar]); - }) - .then((totals) => { - // Move native unsupported containers into the correct order - return this.reorderUnsupportedContainers().then(() => { - return totals.filter(Boolean).reduce((a, b) => a + b, 0); - }); - }); - } - createNativeSeparator(parentId: string): ng.IPromise { const newSeparator: NativeBookmarks.CreateDetails = { parentId, @@ -243,6 +66,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } + // TODO: unify, see chromium version ensureContainersExist(bookmarks: Bookmark[]): Bookmark[] { if (angular.isUndefined(bookmarks)) { return; @@ -313,160 +137,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { } } - getNativeBookmarksAsBookmarks(): ng.IPromise { - let allNativeBookmarks = []; - - // Get native container ids - return this.getNativeContainerIds() - .then((nativeContainerIds) => { - const menuBookmarksId = nativeContainerIds.get(BookmarkContainer.Menu); - const mobileBookmarksId = nativeContainerIds.get(BookmarkContainer.Mobile); - const otherBookmarksId = nativeContainerIds.get(BookmarkContainer.Other); - const toolbarBookmarksId = nativeContainerIds.get(BookmarkContainer.Toolbar); - - // Get menu bookmarks - const getMenuBookmarks = - menuBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(menuBookmarksId).then((subTree) => { - const menuBookmarks = subTree[0]; - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(menuBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks(menuBookmarks.children); - }); - - // Get mobile bookmarks - const getMobileBookmarks = - mobileBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(mobileBookmarksId).then((subTree) => { - const mobileBookmarks = subTree[0]; - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(mobileBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks(mobileBookmarks.children); - }); - - // Get other bookmarks - const getOtherBookmarks = - otherBookmarksId === undefined - ? Promise.resolve(undefined) - : browser.bookmarks.getSubTree(otherBookmarksId).then((subTree) => { - const otherBookmarks = subTree[0]; - if (otherBookmarks.children.length === 0) { - return; - } - - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(otherBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - - // Convert native bookmarks sub tree to bookmarks - const bookmarks = this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks(otherBookmarks.children); - - // Remove any unsupported container folders present - const bookmarksWithoutContainers = bookmarks.filter((x) => { - return !this.unsupportedContainers.find((y) => { - return y === x.title; - }); - }); - return bookmarksWithoutContainers; - }); - - // Get toolbar bookmarks if enabled - const getToolbarBookmarks = - toolbarBookmarksId === undefined - ? this.$q.resolve(undefined) - : browser.bookmarks.getSubTree(toolbarBookmarksId).then((results) => { - const toolbarBookmarks = results[0]; - return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - if (syncBookmarksToolbar && toolbarBookmarks.children.length > 0) { - // Add all bookmarks into flat array - this.bookmarkHelperSvc.eachBookmark(toolbarBookmarks.children, (bookmark) => { - allNativeBookmarks.push(bookmark); - }); - return this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks(toolbarBookmarks.children); - } - }); - }); - - return this.$q.all([getMenuBookmarks, getMobileBookmarks, getOtherBookmarks, getToolbarBookmarks]); - }) - .then((results) => { - const menuBookmarks = results[0]; - const mobileBookmarks = results[1]; - const otherBookmarks = results[2]; - const toolbarBookmarks = results[3]; - const bookmarks: Bookmark[] = []; - - // Add other container if bookmarks present - const otherContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarks, true); - if (otherBookmarks?.length > 0) { - otherContainer.children = otherBookmarks; - } - - // Add toolbar container if bookmarks present - const toolbarContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarks, true); - if (toolbarBookmarks?.length > 0) { - toolbarContainer.children = toolbarBookmarks; - } - - // Add menu container if bookmarks present - const menuContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Menu, bookmarks, true); - if (menuBookmarks?.length > 0) { - menuContainer.children = menuBookmarks; - } - - // Add mobile container if bookmarks present - const mobileContainer = this.bookmarkHelperSvc.getContainer(BookmarkContainer.Mobile, bookmarks, true); - if (mobileBookmarks?.length > 0) { - mobileContainer.children = mobileBookmarks; - } - - // Filter containers from flat array of bookmarks - [otherContainer, toolbarContainer, menuContainer, mobileContainer].forEach((container) => { - if (!container) { - return; - } - - allNativeBookmarks = allNativeBookmarks.filter((bookmark) => { - return bookmark.title !== container.title; - }); - }); - - // Sort by date added asc - allNativeBookmarks = allNativeBookmarks.sort((x, y) => { - return x.dateAdded - y.dateAdded; - }); - - // Iterate native bookmarks to add unique bookmark ids in correct order - allNativeBookmarks.forEach((nativeBookmark) => { - this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { - if ( - !bookmark.id && - ((!nativeBookmark.url && bookmark.title === nativeBookmark.title) || - (nativeBookmark.url && bookmark.url === nativeBookmark.url)) - ) { - bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); - } - }); - }); - - // Find and fix any bookmarks missing ids - this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { - if (!bookmark.id) { - bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); - } - }); - - return bookmarks; - }); - } - + // TODO: unify/share more code with chromium getNativeContainerIds(): ng.IPromise { return this.utilitySvc .isSyncEnabled() @@ -514,6 +185,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { } // Add container ids to result + containerIds.platformDefaultBookmarksNodeId = otherBookmarksNode.id; containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 3bb33481d..9bb726854 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -211,7 +211,59 @@ export default abstract class WebExtBookmarkService { }); } - abstract clearNativeBookmarks(): ng.IPromise; + clearNativeBookmarks(): ng.IPromise { + // Get native container ids + return this.getNativeContainerIds() + .then((nativeContainerIds) => { + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const clearPromises = []; + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName = BookmarkContainer[containerEnumVal]; + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { + if (!syncBookmarksToolbar) { + this.logSvc.logInfo('Not clearing toolbar'); + continue; + } + } + + // Clear bookmarks of that type + if (nativeBookmarkNodeId) { + const clearPromise = browser.bookmarks + .getChildren(nativeBookmarkNodeId) + .then((children) => { + // TODO: alternatively the other way arround... do not clear unsupported containers bookmark nodes but clear the default bookmark node completely + // Do not remove the bookmark-folders that server as mount-point for unsupported containers + if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + children = children.filter( + (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + ); + } + return this.$q.all( + children.map((child) => { + return this.removeNativeBookmarks(child.id); + }) + ); + }) + .catch((err) => { + this.logSvc.logWarning(`Error clearing ${containerEnumVal} bookmarks`); + throw err; + }); + clearPromises.push(clearPromise); + } + } + + return this.$q.all(clearPromises).then(() => {}); + }); + }) + .catch((err) => { + throw new Exceptions.FailedRemoveNativeBookmarksException(undefined, err); + }); + } convertNativeBookmarkToBookmark( nativeBookmark: NativeBookmarks.BookmarkTreeNode, @@ -295,7 +347,65 @@ export default abstract class WebExtBookmarkService { }); } - abstract createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise; + createNativeBookmarksFromBookmarks(bookmarks: Bookmark[]): ng.IPromise { + // Get native container ids + return this.getNativeContainerIds() + .then((nativeContainerIds) => { + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const populatePromises = []; + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName = BookmarkContainer[containerEnumVal]; + // Get container + const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks); + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { + if (!syncBookmarksToolbar) { + this.logSvc.logInfo('Not populating toolbar'); + continue; + } + } + + // Populate bookmarks for the container + if (container) { + let parentNodeId: string; + let childrenToCreate: Bookmark[]; + if (nativeBookmarkNodeId) + { + parentNodeId = nativeBookmarkNodeId; + childrenToCreate = container.children; + } + else + { // there is no nativeContainerId -> it must be one of unsupportedContainers -> create it now + parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; + childrenToCreate = [container]; + } + const populatePromise = browser.bookmarks + .getSubTree(parentNodeId) + .then(() => { + return this.createNativeBookmarkTree(parentNodeId, childrenToCreate); + }) + .catch((err) => { + this.logSvc.logInfo(`Error populating ${containerEnumVal}.`); + throw err; + }); + populatePromises.push(populatePromise); + } + } + + return this.$q.all(populatePromises); + }); + }) + .then((totals) => { + // Move native unsupported containers into the correct order + return this.reorderUnsupportedContainers().then(() => { + return totals.reduce((a, b) => a + b, 0); + }); + }); + } createNativeBookmarkTree(parentId: string, bookmarks: Bookmark[]): ng.IPromise { let processError: Error; @@ -375,7 +485,124 @@ export default abstract class WebExtBookmarkService { }); } - abstract getNativeBookmarksAsBookmarks(): ng.IPromise; + getNativeBookmarksAsBookmarks(): ng.IPromise { + let allNativeBookmarks: NativeBookmarks.BookmarkTreeNode[] = []; + + // Get native container ids + return this.getNativeContainerIds().then((nativeContainerIds) => { + // Get whether syncBookmarksToolbar + return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { + const getBookmarkPromises = new Array]>>(); + + for (const containerEnumVal of Object.keys(BookmarkContainer)) { + const containerName: BookmarkContainer = BookmarkContainer[containerEnumVal]; + // Get native bookmark node id + const nativeBookmarkNodeId = nativeContainerIds.get(containerName); + + if (containerName === BookmarkContainer.Toolbar) { + if (!syncBookmarksToolbar) { + // skip + continue; + } + } + + // Map bookmarks of that type + if (nativeBookmarkNodeId) { + const getBookmarkPromise: Promise<[BookmarkContainer, Array]> = browser.bookmarks + .getSubTree(nativeBookmarkNodeId) + .then((subTree) => { + const bookmarksNode = subTree[0]; + + if (!bookmarksNode.children?.length) { + return [containerName, [] as Bookmark[]]; + } + + let bookmarksNodeChildren: NativeBookmarks.BookmarkTreeNode[]; + // Skip over any unsupported container mount-point bookmark folders present, + // if we are now "in" the platform-default bookmark node. + // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry + if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + bookmarksNodeChildren = bookmarksNode.children.filter( + (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + ); + } else { + bookmarksNodeChildren = bookmarksNode.children; + } + + // Add all native bookmarks (except the "unsupported containers" mount-point folders) into flat array + this.bookmarkHelperSvc.eachBookmark(bookmarksNodeChildren, (bookmark) => { + allNativeBookmarks.push(bookmark); + }); + + // Return all native bookmarks (except the "unsupported containers" mount-point folders) + // converted to "our" bookmarks. + const convertedBookmarks = this.bookmarkHelperSvc.getNativeBookmarksAsBookmarks( + this.getNativeBookmarksWithSeparators(bookmarksNodeChildren) + ); + return [containerName, convertedBookmarks]; + }); + + getBookmarkPromises.push(getBookmarkPromise); + } + } + + return this.$q.all(getBookmarkPromises).then((containerBookmarksPairArray) => { + const bookmarks: Bookmark[] = []; + + containerBookmarksPairArray.forEach((tuple) => { + const [containerName, convertedBookmarks] = tuple; + + const container = this.bookmarkHelperSvc.getContainer(containerName, bookmarks, true); + if (convertedBookmarks.length > 0) { + container.children = convertedBookmarks; + } + + // Michal Kotoun's note: this is probably a no-op, since we only added to allNativeBookmarks: + // 1) children of non-default containers + // 2) children of default container except for virtual/unsupportedContainers + // Filter containers from flat array of bookmarks + allNativeBookmarks = allNativeBookmarks.filter((bookmark) => { + return bookmark.title !== container.title; + }); + }); + + // Sort by date added asc + allNativeBookmarks = allNativeBookmarks.sort((x, y) => { + return x.dateAdded - y.dateAdded; + }); + + // Iterate native bookmarks to add unique bookmark ids in correct order + allNativeBookmarks.forEach((nativeBookmark) => { + this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { + if ( + !bookmark.id && + ((!nativeBookmark.url && bookmark.title === nativeBookmark.title) || + (nativeBookmark.url && bookmark.url === nativeBookmark.url)) + ) { + bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); + } + }); + }); + + // Find and fix any bookmarks missing ids + this.bookmarkHelperSvc.eachBookmark(bookmarks, (bookmark) => { + if (!bookmark.id) { + bookmark.id = this.bookmarkHelperSvc.getNewBookmarkId(bookmarks); + } + }); + + return bookmarks; + }); + }); + }); + } + + // no-op by default (in firefox) -> maybe a slight reformatting to move the bookmark.type = Separator in chromium would be nice! + getNativeBookmarksWithSeparators( + nativeBookmarks: NativeBookmarks.BookmarkTreeNode[] + ): NativeBookmarks.BookmarkTreeNode[] { + return nativeBookmarks; + } abstract getNativeContainerIds(): ng.IPromise; From 1979c23b395feade5929fe19df3da8cfd34d4bff Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sat, 13 Feb 2021 15:24:43 +0100 Subject: [PATCH 11/19] Add notes --- src/modules/shared/bookmark/bookmark.enum.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/modules/shared/bookmark/bookmark.enum.ts b/src/modules/shared/bookmark/bookmark.enum.ts index 071e00039..e25724093 100644 --- a/src/modules/shared/bookmark/bookmark.enum.ts +++ b/src/modules/shared/bookmark/bookmark.enum.ts @@ -13,6 +13,8 @@ enum BookmarkContainer { Toolbar = '[xbs] Toolbar' } +// TODO: maybe remove this new code and use solely (un)supported bookmark detection at runtime +// OR! always create all containers, so that merging in the future is simpler? const MandatoryBookmarkContainers: Array = [ BookmarkContainer.Other, BookmarkContainer.Mobile, From 94e6f060cb102bf8b711a72dd16fcc56973e8408 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Sun, 14 Feb 2021 21:54:32 +0100 Subject: [PATCH 12/19] getNativeContainerIds() now only returns entries with defined values. In the original code, the return value of the getNativeContainerIds() can vary. If sync was not enabled (or synced bookmarks we not loaded yet) it would return a map of entries that always have a defined value. With the synced bookmarks present, it would returns entires with value=undefined for unsupported containers which did not currently have a mount-point bookmark folder (for example, due to the folder being deleted or renamed via the browser UI). This logic was used in app/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts Lines 1033 to 1044 in 317e697 method wasContainerChanged, which was called in processChangeType{Add,Modify.Remove}OnBookmarks methods and also directly in processChangeTypeMoveOnBookmarks. The code there checked whether the getNativeContainerIds had any undefined values, as a signal that a previously mapped natively unsupported container was somehow modified and not being detected by the getNativeContainerIds() any more. Now, getNativeContainers() returns only entires for containers for which a native bookmark node id was discovered. The code of wasContainerChanged mehod does the same check as before, but on its own - the logic (for now) remains equal to the old one! It looks for any (synced)bookmarks.children (all containers in the current syncdata) that are not present in the result of getNativeContainerIds(). Also, wasContainerChanged() is now also used in processChangeTypeMoveOnBookmarks (unifying it with the other three processChange* calls) --- .../chromium-bookmark.service.ts | 170 ++++++++---------- .../firefox-bookmark.service.ts | 92 +++++----- .../webext-bookmark.service.ts | 50 +++--- 3 files changed, 149 insertions(+), 163 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index b9c1c6c3f..df931dc58 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -173,101 +173,89 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } // TODO: unify/share more code with firefox - getNativeContainerIds(): ng.IPromise { - return this.utilitySvc - .isSyncEnabled() - .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) - .then((bookmarks) => { - // Initialise container ids object using containers defined in bookmarks - const containerIds = new NativeContainersInfo(); - if (!angular.isUndefined(bookmarks)) { - bookmarks.forEach((x) => { - containerIds.set(x.title as BookmarkContainer, undefined); - }); - } - - // Populate container ids - return browser.bookmarks.getTree().then((tree) => { - // Get the root child nodes - let otherBookmarksNode = tree[0].children.find((x) => { - return x.title === this.otherBookmarksNodeTitle; - }); - let toolbarBookmarksNode = tree[0].children.find((x) => { - return x.title === this.toolbarBookmarksNodeTitle; - }); - let menuBookmarksNode = tree[0].children.find((x) => { - return x.title === this.menuBookmarksNodeTitle; - }); - let mobileBookmarksNode = tree[0].children.find((x) => { - return x.title === this.mobileBookmarksNodeTitle; - }); - - // TODO: improve this logic - const defaultBookmarksNode = otherBookmarksNode || mobileBookmarksNode; - if (!defaultBookmarksNode) { - // coulnd not find a default container to create folders to place other containers in - throw new Exceptions.ContainerNotFoundException(); - } + getNativeContainerIds(): ng.IPromise { + const containerIds = new NativeContainersInfo(); + // Populate container ids + return browser.bookmarks.getTree().then((tree) => { + // Get the root child nodes + let otherBookmarksNode = tree[0].children.find((x) => { + return x.title === this.otherBookmarksNodeTitle; + }); + let toolbarBookmarksNode = tree[0].children.find((x) => { + return x.title === this.toolbarBookmarksNodeTitle; + }); + let menuBookmarksNode = tree[0].children.find((x) => { + return x.title === this.menuBookmarksNodeTitle; + }); + let mobileBookmarksNode = tree[0].children.find((x) => { + return x.title === this.mobileBookmarksNodeTitle; + }); - // TODO: FINISH THIS! - // is related to createNativeBookmarksFromBookmarks - // HACK!!!! - this.unsupportedContainers = []; + // TODO: improve this logic + const defaultBookmarksNode = otherBookmarksNode || mobileBookmarksNode; + if (!defaultBookmarksNode) { + // coulnd not find a default container to create folders to place other containers in + throw new Exceptions.ContainerNotFoundException(); + } - // Check for unsupported containers - if (!otherBookmarksNode) { - this.logSvc.logWarning('Unsupported container: other bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Other); - otherBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Other; - }); - } - if (!toolbarBookmarksNode) { - this.logSvc.logWarning('Unsupported container: toolbar bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Toolbar); - toolbarBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Toolbar; - }); - } - if (!menuBookmarksNode) { - this.logSvc.logWarning('Unsupported container: menu bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Menu); - menuBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Menu; - }); - } - if (!mobileBookmarksNode) { - this.logSvc.logWarning('Unsupported container: mobile bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Mobile); - mobileBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Mobile; - }); - } + // TODO: FINISH THIS! + // is related to createNativeBookmarksFromBookmarks + // HACK!!!! + this.unsupportedContainers = []; - // Add container ids to result - { - // must be always defined! - containerIds.platformDefaultBookmarksNodeId = defaultBookmarksNode.id; - } - if (!angular.isUndefined(otherBookmarksNode)) { - containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); - } - if (!angular.isUndefined(toolbarBookmarksNode)) { - containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); - } - if (!angular.isUndefined(menuBookmarksNode)) { - containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); - } - if (!angular.isUndefined(mobileBookmarksNode)) { - containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); - } - return containerIds; + // Check for unsupported containers + if (!otherBookmarksNode) { + this.logSvc.logWarning('Unsupported container: other bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Other); + otherBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Other; }); - }); + } + if (!toolbarBookmarksNode) { + this.logSvc.logWarning('Unsupported container: toolbar bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Toolbar); + toolbarBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Toolbar; + }); + } + if (!menuBookmarksNode) { + this.logSvc.logWarning('Unsupported container: menu bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Menu); + menuBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Menu; + }); + } + if (!mobileBookmarksNode) { + this.logSvc.logWarning('Unsupported container: mobile bookmarks'); + // HACK!!!! + this.unsupportedContainers.push(BookmarkContainer.Mobile); + mobileBookmarksNode = defaultBookmarksNode.children.find((x) => { + return x.title === BookmarkContainer.Mobile; + }); + } + + // Add container ids to result + { + // must be always defined! + containerIds.platformDefaultBookmarksNodeId = defaultBookmarksNode.id; + } + if (!angular.isUndefined(otherBookmarksNode)) { + containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); + } + if (!angular.isUndefined(toolbarBookmarksNode)) { + containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); + } + if (!angular.isUndefined(menuBookmarksNode)) { + containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); + } + if (!angular.isUndefined(mobileBookmarksNode)) { + containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); + } + return containerIds; + }); } isSeparator(nativeBookmark: NativeBookmarks.BookmarkTreeNode): boolean { diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index f47df2a82..fee80e08e 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -139,60 +139,48 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { // TODO: unify/share more code with chromium getNativeContainerIds(): ng.IPromise { - return this.utilitySvc - .isSyncEnabled() - .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) - .then((bookmarks) => { - // Initialise container ids object using containers defined in bookmarks - const containerIds = new NativeContainersInfo(); - if (!angular.isUndefined(bookmarks)) { - bookmarks.forEach((x) => { - containerIds.set(x.title as BookmarkContainer, undefined); - }); - } - - // Populate container ids - return browser.bookmarks.getTree().then((tree) => { - // Get the root child nodes - const menuBookmarksNode = tree[0].children.find((x) => { - return x.id === 'menu________'; - }); - const mobileBookmarksNode = tree[0].children.find((x) => { - return x.id === 'mobile______'; - }); - const otherBookmarksNode = tree[0].children.find((x) => { - return x.id === 'unfiled_____'; - }); - const toolbarBookmarksNode = tree[0].children.find((x) => { - return x.id === 'toolbar_____'; - }); + const containerIds = new NativeContainersInfo(); + // Populate container ids + return browser.bookmarks.getTree().then((tree) => { + // Get the root child nodes + const menuBookmarksNode = tree[0].children.find((x) => { + return x.id === 'menu________'; + }); + const mobileBookmarksNode = tree[0].children.find((x) => { + return x.id === 'mobile______'; + }); + const otherBookmarksNode = tree[0].children.find((x) => { + return x.id === 'unfiled_____'; + }); + const toolbarBookmarksNode = tree[0].children.find((x) => { + return x.id === 'toolbar_____'; + }); - // Throw an error if a native container is not found - if (!menuBookmarksNode || !mobileBookmarksNode || !otherBookmarksNode || !toolbarBookmarksNode) { - if (!menuBookmarksNode) { - this.logSvc.logWarning('Missing container: menu bookmarks'); - } - if (!mobileBookmarksNode) { - this.logSvc.logWarning('Missing container: mobile bookmarks'); - } - if (!otherBookmarksNode) { - this.logSvc.logWarning('Missing container: other bookmarks'); - } - if (!toolbarBookmarksNode) { - this.logSvc.logWarning('Missing container: toolbar bookmarks'); - } - throw new Exceptions.ContainerNotFoundException(); - } + // Throw an error if a native container is not found + if (!menuBookmarksNode || !mobileBookmarksNode || !otherBookmarksNode || !toolbarBookmarksNode) { + if (!menuBookmarksNode) { + this.logSvc.logWarning('Missing container: menu bookmarks'); + } + if (!mobileBookmarksNode) { + this.logSvc.logWarning('Missing container: mobile bookmarks'); + } + if (!otherBookmarksNode) { + this.logSvc.logWarning('Missing container: other bookmarks'); + } + if (!toolbarBookmarksNode) { + this.logSvc.logWarning('Missing container: toolbar bookmarks'); + } + throw new Exceptions.ContainerNotFoundException(); + } - // Add container ids to result - containerIds.platformDefaultBookmarksNodeId = otherBookmarksNode.id; - containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); - containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); - containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); - containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); - return containerIds; - }); - }); + // Add container ids to result + containerIds.platformDefaultBookmarksNodeId = otherBookmarksNode.id; + containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); + containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); + containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); + containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); + return containerIds; + }); } processNativeBookmarkEventsQueue(): void { diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 9bb726854..0132171df 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -374,12 +374,13 @@ export default abstract class WebExtBookmarkService { let parentNodeId: string; let childrenToCreate: Bookmark[]; if (nativeBookmarkNodeId) - { + { // this is a natively supported container parentNodeId = nativeBookmarkNodeId; childrenToCreate = container.children; } else - { // there is no nativeContainerId -> it must be one of unsupportedContainers -> create it now + { // there is no nativeContainerId -> it's not a natively supported container + // -> create it's mount-point bookmark folder now parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; childrenToCreate = [container]; } @@ -604,6 +605,13 @@ export default abstract class WebExtBookmarkService { return nativeBookmarks; } + /** + * Returns the mapping of BookmarkContainer to native BookmarkTreeNode ids. + * For natively supported containers, their ids are returned. + * For (natively) unsupported containers, it returns the id the bookmark-folder they are mapped to - the detection is based on: + * 1) they are children of the browser-default container; + * 2) the name of the folder equals to the name of the bookmark container. + */ abstract getNativeContainerIds(): ng.IPromise; getSupportedUrl(url: string): string { @@ -848,12 +856,14 @@ export default abstract class WebExtBookmarkService { bookmarks: Bookmark[], changeData: MoveNativeBookmarkChangeData ): ng.IPromise { - // Get native container ids - return this.getNativeContainerIds().then((nativeContainerIds) => { - // Check if container was moved to a different folder - if (Array.from(nativeContainerIds.values()).includes(undefined)) { + // Get native container ids + return Promise.all([ // TODO: use this.$q.all ? + this.wasContainerChanged(), + this.getNativeContainerIds() + ]).then(([changedBookmarkIsContainer, nativeContainerIds]) => { + // Check if container was changed + if (changedBookmarkIsContainer) throw new Exceptions.ContainerChangedException(); - } return browser.bookmarks.get(changeData.id).then((results) => { // If container moved to a different position in same folder, skip sync @@ -1196,24 +1206,24 @@ export default abstract class WebExtBookmarkService { return this.$q.resolve(); } - wasContainerChanged(changedNativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { - return this.getNativeContainerIds().then((nativeContainerIds) => { + wasContainerChanged(changedNativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + return Promise.all([ // TODO: use this.$q.all ? + this.getNativeContainerIds(), + this.utilitySvc.isSyncEnabled().then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) + ]).then(([nativeContainerIds, bookmarks]) => { // If parent is not platform-default bookmarks, no container was changed const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; - if (changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId) { + if (angular.isDefined(changedNativeBookmark) && changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId) { return false; } - // Michal Kotoun's note: this would not work before my changes and certainly will not work now! Replacement bellow. - // If any native container ids are undefined, container was removed - // if (Array.from(nativeContainerIds.values()).includes(undefined)) { - // return true; - // } - // If its an unsupported container bookmark native node && its native container ids is undefined, container was removed - const titleAsBookmarkContainer = changedNativeBookmark.title as BookmarkContainer; - const isUnsupportedContainerBookmark = this.unsupportedContainers.includes(titleAsBookmarkContainer); - if (isUnsupportedContainerBookmark && angular.isUndefined(nativeContainerIds.get(titleAsBookmarkContainer))) { - return true; + if (!angular.isUndefined(bookmarks)) { + // if a container is present in the sync-data, but its native bookmark node was not found in getNativeContainerIds() + // the previously-existing node was either removed, renamed or otherwise modified + const nativeBookmarkNodeDisappeared = 0 <= bookmarks.findIndex( + b => angular.isUndefined(nativeContainerIds.get(b.title as BookmarkContainer)) + ); + if (nativeBookmarkNodeDisappeared) return true } return browser.bookmarks From 870843f1300417d36d27e921bcd831af0b614d83 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Mon, 15 Feb 2021 05:12:51 +0100 Subject: [PATCH 13/19] Enable use of strictNullChecks in tsconfig.json without affecting the build process (still leaving strictNullChecks to default=false) --- tsconfig.build.json | 6 ++++++ tsconfig.json | 1 + webpack/base.config.js | 8 +++++++- 3 files changed, 14 insertions(+), 1 deletion(-) create mode 100644 tsconfig.build.json diff --git a/tsconfig.build.json b/tsconfig.build.json new file mode 100644 index 000000000..c8d06e998 --- /dev/null +++ b/tsconfig.build.json @@ -0,0 +1,6 @@ +{ + "extends": "./tsconfig.json", + "compilerOptions": { + "strictNullChecks": false + } +} \ No newline at end of file diff --git a/tsconfig.json b/tsconfig.json index 1ba4d291c..a6caac5c8 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -9,6 +9,7 @@ "resolveJsonModule": true, "sourceMap": true, "strictFunctionTypes": true, + //"strictNullChecks": true, "target": "ES2018" }, "exclude": ["node_modules"], diff --git a/webpack/base.config.js b/webpack/base.config.js index c328734b4..8e2b2687b 100644 --- a/webpack/base.config.js +++ b/webpack/base.config.js @@ -5,7 +5,13 @@ module.exports = { devtool: 'inline-cheap-module-source-map', module: { rules: [ - { test: /\.ts$/, loader: 'ts-loader' }, + { + test: /\.ts$/, + loader: 'ts-loader', + options: { + configFile: 'tsconfig.build.json' + } + }, { test: /\.(sa|sc|c)ss$/, use: [ From 74bc6d885a67c73087cb6214d6653238c4483c3d Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Mon, 15 Feb 2021 05:16:55 +0100 Subject: [PATCH 14/19] Fix formating and strictNullChecks errors --- .../chromium-bookmark.service.ts | 8 +- .../firefox-bookmark.service.ts | 7 +- .../webext-bookmark.service.ts | 110 ++++++++++-------- 3 files changed, 68 insertions(+), 57 deletions(-) diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index df931dc58..5d3702ce9 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -38,7 +38,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { .then((inToolbar) => { // Skip process if bookmark is not in toolbar and already native separator if ( - (bookmark.url === this.platformSvc.getNewTabUrl() && + (bookmark.url === this.platformSvc.getNewTabUrl!() && !inToolbar && bookmark.title === Globals.Bookmarks.HorizontalSeparatorTitle) || (inToolbar && bookmark.title === Globals.Bookmarks.VerticalSeparatorTitle) @@ -66,7 +66,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { index: bookmark.index, parentId: bookmark.parentId, title, - url: this.platformSvc.getNewTabUrl() + url: this.platformSvc.getNewTabUrl!() }; return browser.bookmarks.remove(bookmark.id).then(() => { return browser.bookmarks.create(separator); @@ -87,7 +87,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { const newSeparator: NativeBookmarks.CreateDetails = { parentId, title: inToolbar ? Globals.Bookmarks.VerticalSeparatorTitle : Globals.Bookmarks.HorizontalSeparatorTitle, - url: this.platformSvc.getNewTabUrl() + url: this.platformSvc.getNewTabUrl!() }; return browser.bookmarks.create(newSeparator); }) @@ -319,7 +319,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { }); } - syncNativeBookmarkCreated(id?: string, nativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + syncNativeBookmarkCreated(id: string, nativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { // If bookmark is separator update native bookmark properties return (this.isSeparator(nativeBookmark) ? this.convertNativeBookmarkToSeparator(nativeBookmark) diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index fee80e08e..304e16302 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -1,4 +1,3 @@ -import angular from 'angular'; import { Injectable } from 'angular-ts-decorators'; import autobind from 'autobind-decorator'; import { Bookmarks as NativeBookmarks, browser } from 'webextension-polyfill-ts'; @@ -190,7 +189,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { super.processNativeBookmarkEventsQueue(); } - syncNativeBookmarkChanged(id?: string): ng.IPromise { + syncNativeBookmarkChanged(id: string): ng.IPromise { // Retrieve full bookmark info return browser.bookmarks.getSubTree(id).then((results) => { const changedBookmark = results[0]; @@ -209,7 +208,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - syncNativeBookmarkCreated(id?: string, nativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + syncNativeBookmarkCreated(id: string, nativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise { // Create change info const data: AddNativeBookmarkChangeData = { nativeBookmark @@ -243,7 +242,7 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - syncNativeBookmarkMoved(id?: string, moveInfo?: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise { + syncNativeBookmarkMoved(id: string, moveInfo: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise { // Create change info const data: MoveNativeBookmarkChangeData = { ...moveInfo, diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 0132171df..02f015d93 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -112,11 +112,13 @@ export default abstract class WebExtBookmarkService { ): BookmarkIdMapping[] => { return nativeBookmarks.reduce((acc, val, index) => { // Create mapping for the current node - const mapping = this.bookmarkIdMapperSvc.createMapping(syncedBookmarks[index].id, val.id); + const mapping = this.bookmarkIdMapperSvc.createMapping(syncedBookmarks[index].id as number, val.id); acc.push(mapping); // Process child nodes - return val.children?.length ? acc.concat(mapIds(val.children, syncedBookmarks[index].children)) : acc; + return val.children?.length + ? acc.concat(mapIds(val.children, syncedBookmarks[index].children as Bookmark[])) + : acc; }, [] as BookmarkIdMapping[]); }; @@ -188,7 +190,7 @@ export default abstract class WebExtBookmarkService { checkIfBookmarkChangeShouldBeSynced(changedBookmark: Bookmark, bookmarks: Bookmark[]): ng.IPromise { return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { // If container is Toolbar, check if Toolbar sync is disabled - const container = this.bookmarkHelperSvc.getContainerByBookmarkId(changedBookmark.id, bookmarks); + const container = this.bookmarkHelperSvc.getContainerByBookmarkId(changedBookmark.id as number, bookmarks); if (!container) { throw new Exceptions.ContainerNotFoundException(); } @@ -305,7 +307,7 @@ export default abstract class WebExtBookmarkService { // Get parent bookmark and count containers return browser.bookmarks.getSubTree(parentId).then((subTree) => { - const numContainers = subTree[0].children.filter((child, childIndex) => { + const numContainers = subTree[0].children!.filter((child, childIndex) => { return childIndex < index && Array.from(nativeContainerIds.values()).includes(child.id); }).length; return numContainers; @@ -326,8 +328,8 @@ export default abstract class WebExtBookmarkService { createNativeBookmark( parentId: string, - title: string, - url: string, + title?: string, + url?: string, index?: number ): ng.IPromise { const nativeBookmarkInfo: NativeBookmarks.CreateDetails = { @@ -353,7 +355,7 @@ export default abstract class WebExtBookmarkService { .then((nativeContainerIds) => { // Get whether syncBookmarksToolbar return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { - const populatePromises = []; + const populatePromises: ng.IPromise[] = []; for (const containerEnumVal of Object.keys(BookmarkContainer)) { const containerName = BookmarkContainer[containerEnumVal]; @@ -373,13 +375,12 @@ export default abstract class WebExtBookmarkService { if (container) { let parentNodeId: string; let childrenToCreate: Bookmark[]; - if (nativeBookmarkNodeId) - { // this is a natively supported container + if (nativeBookmarkNodeId) { + // this is a natively supported container parentNodeId = nativeBookmarkNodeId; - childrenToCreate = container.children; - } - else - { // there is no nativeContainerId -> it's not a natively supported container + childrenToCreate = container.children!; + } else { + // there is no nativeContainerId -> it's not a natively supported container // -> create it's mount-point bookmark folder now parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; childrenToCreate = [container]; @@ -459,7 +460,7 @@ export default abstract class WebExtBookmarkService { return this.getNativeContainerIds().then((nativeContainerIds) => { for (const [nativeBookmarkNodeId, containerName] of nativeContainerIds.entries()) { if (nativeBookmarkNodeId === nativeBookmarkId) return containerName; - }; + } return ''; }); } @@ -596,9 +597,9 @@ export default abstract class WebExtBookmarkService { }); }); }); - } + } - // no-op by default (in firefox) -> maybe a slight reformatting to move the bookmark.type = Separator in chromium would be nice! + // no-op by default (in Firefox) -> maybe a slight reformatting to move the bookmark.type = Separator assignment in chromium would be nice! getNativeBookmarksWithSeparators( nativeBookmarks: NativeBookmarks.BookmarkTreeNode[] ): NativeBookmarks.BookmarkTreeNode[] { @@ -614,22 +615,23 @@ export default abstract class WebExtBookmarkService { */ abstract getNativeContainerIds(): ng.IPromise; - getSupportedUrl(url: string): string { + getSupportedUrl(url?: string): string { if (angular.isUndefined(url ?? undefined)) { return ''; } + url = url!; // If url is not supported, use new tab url instead let returnUrl = url; if (!this.platformSvc.urlIsSupported(url)) { this.logSvc.logInfo(`Bookmark url unsupported: ${url}`); - returnUrl = this.platformSvc.getNewTabUrl(); + returnUrl = this.platformSvc.getNewTabUrl!(); } return returnUrl; } - isNativeBookmarkIdOfToolbarContainer(nativeBookmarkId: string): ng.IPromise { + isNativeBookmarkIdOfToolbarContainer(nativeBookmarkId?: string): ng.IPromise { return this.getNativeContainerIds().then((nativeContainerIds) => { return nativeBookmarkId === nativeContainerIds.get(BookmarkContainer.Toolbar); }); @@ -855,20 +857,20 @@ export default abstract class WebExtBookmarkService { processChangeTypeMoveOnBookmarks( bookmarks: Bookmark[], changeData: MoveNativeBookmarkChangeData - ): ng.IPromise { - // Get native container ids - return Promise.all([ // TODO: use this.$q.all ? - this.wasContainerChanged(), + ): ng.IPromise { + // Get native container ids + return Promise.all([ + // TODO: use this.$q.all ? + this.wasContainerChanged(), this.getNativeContainerIds() ]).then(([changedBookmarkIsContainer, nativeContainerIds]) => { - // Check if container was changed - if (changedBookmarkIsContainer) - throw new Exceptions.ContainerChangedException(); + // Check if container was changed + if (changedBookmarkIsContainer) throw new Exceptions.ContainerChangedException(); return browser.bookmarks.get(changeData.id).then((results) => { // If container moved to a different position in same folder, skip sync const movedBookmark = results[0]; - if (Array.from(nativeContainerIds.values()).includes(movedBookmark.id)) { + if ([...nativeContainerIds.values()].includes(movedBookmark.id)) { return; } @@ -1059,15 +1061,23 @@ export default abstract class WebExtBookmarkService { const currentEvent = this.nativeBookmarkEventsQueue.shift(); switch (currentEvent.changeType) { case BookmarkChangeType.Add: - return this.syncNativeBookmarkCreated(...currentEvent.eventArgs); + return this.syncNativeBookmarkCreated( + ...(currentEvent.eventArgs as [string, NativeBookmarks.BookmarkTreeNode]) + ); case BookmarkChangeType.ChildrenReordered: - return this.syncNativeBookmarkChildrenReordered(...currentEvent.eventArgs); + return this.syncNativeBookmarkChildrenReordered( + ...(currentEvent.eventArgs as [string, OnChildrenReorderedReorderInfoType]) + ); case BookmarkChangeType.Remove: - return this.syncNativeBookmarkRemoved(...currentEvent.eventArgs); + return this.syncNativeBookmarkRemoved( + ...(currentEvent.eventArgs as [string, NativeBookmarks.OnRemovedRemoveInfoType]) + ); case BookmarkChangeType.Move: - return this.syncNativeBookmarkMoved(...currentEvent.eventArgs); + return this.syncNativeBookmarkMoved( + ...(currentEvent.eventArgs as [string, NativeBookmarks.OnMovedMoveInfoType]) + ); case BookmarkChangeType.Modify: - return this.syncNativeBookmarkChanged(...currentEvent.eventArgs); + return this.syncNativeBookmarkChanged(...(currentEvent.eventArgs as [string])); default: throw new Exceptions.AmbiguousSyncRequestException(); } @@ -1163,12 +1173,9 @@ export default abstract class WebExtBookmarkService { }); } - abstract syncNativeBookmarkChanged(id?: string): ng.IPromise; + abstract syncNativeBookmarkChanged(id: string): ng.IPromise; - syncNativeBookmarkChildrenReordered( - id?: string, - reorderInfo?: OnChildrenReorderedReorderInfoType - ): ng.IPromise { + syncNativeBookmarkChildrenReordered(id: string, reorderInfo: OnChildrenReorderedReorderInfoType): ng.IPromise { // Create change info const data: ReorderNativeBookmarkChangeData = { childIds: reorderInfo.childIds, @@ -1184,11 +1191,11 @@ export default abstract class WebExtBookmarkService { return this.$q.resolve(); } - abstract syncNativeBookmarkCreated(id?: string, nativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise; + abstract syncNativeBookmarkCreated(id: string, nativeBookmark: NativeBookmarks.BookmarkTreeNode): ng.IPromise; - abstract syncNativeBookmarkMoved(id?: string, moveInfo?: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise; + abstract syncNativeBookmarkMoved(id: string, moveInfo: NativeBookmarks.OnMovedMoveInfoType): ng.IPromise; - syncNativeBookmarkRemoved(id?: string, removeInfo?: NativeBookmarks.OnRemovedRemoveInfoType): ng.IPromise { + syncNativeBookmarkRemoved(id: string, removeInfo: NativeBookmarks.OnRemovedRemoveInfoType): ng.IPromise { // Create change info const data: RemoveNativeBookmarkChangeData = { nativeBookmark: { @@ -1206,24 +1213,29 @@ export default abstract class WebExtBookmarkService { return this.$q.resolve(); } - wasContainerChanged(changedNativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { - return Promise.all([ // TODO: use this.$q.all ? - this.getNativeContainerIds(), - this.utilitySvc.isSyncEnabled().then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) + wasContainerChanged(changedNativeBookmark?: NativeBookmarks.BookmarkTreeNode): ng.IPromise { + return Promise.all([ + // TODO: use this.$q.all ? + this.getNativeContainerIds(), + this.utilitySvc + .isSyncEnabled() + .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) ]).then(([nativeContainerIds, bookmarks]) => { // If parent is not platform-default bookmarks, no container was changed const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; - if (angular.isDefined(changedNativeBookmark) && changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId) { + if ( + angular.isDefined(changedNativeBookmark) && + changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId + ) { return false; } if (!angular.isUndefined(bookmarks)) { // if a container is present in the sync-data, but its native bookmark node was not found in getNativeContainerIds() // the previously-existing node was either removed, renamed or otherwise modified - const nativeBookmarkNodeDisappeared = 0 <= bookmarks.findIndex( - b => angular.isUndefined(nativeContainerIds.get(b.title as BookmarkContainer)) - ); - if (nativeBookmarkNodeDisappeared) return true + const nativeBookmarkNodeDisappeared = + bookmarks.findIndex((b) => angular.isUndefined(nativeContainerIds.get(b.title as BookmarkContainer))) >= 0; + if (nativeBookmarkNodeDisappeared) return true; } return browser.bookmarks From 47ea7349ce7527f77826a099346742763663434e Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Mon, 15 Feb 2021 05:17:40 +0100 Subject: [PATCH 15/19] Silence eslint errors about for(.. of ..) loops --- .../webext-bookmark/webext-bookmark.service.ts | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index 02f015d93..f4d2d380e 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -129,6 +129,7 @@ export default abstract class WebExtBookmarkService { return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { const getBookmarkPromises = new Array>(); + // eslint-disable-next-line no-restricted-syntax for (const containerEnumVal of Object.keys(BookmarkContainer)) { const containerName = BookmarkContainer[containerEnumVal]; // Get native bookmark node id @@ -137,6 +138,7 @@ export default abstract class WebExtBookmarkService { if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { this.logSvc.logInfo('Not mapping toolbar'); + // eslint-disable-next-line no-continue continue; } } @@ -221,6 +223,7 @@ export default abstract class WebExtBookmarkService { return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { const clearPromises = []; + // eslint-disable-next-line no-restricted-syntax for (const containerEnumVal of Object.keys(BookmarkContainer)) { const containerName = BookmarkContainer[containerEnumVal]; // Get native bookmark node id @@ -229,6 +232,7 @@ export default abstract class WebExtBookmarkService { if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { this.logSvc.logInfo('Not clearing toolbar'); + // eslint-disable-next-line no-continue continue; } } @@ -357,6 +361,7 @@ export default abstract class WebExtBookmarkService { return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { const populatePromises: ng.IPromise[] = []; + // eslint-disable-next-line no-restricted-syntax for (const containerEnumVal of Object.keys(BookmarkContainer)) { const containerName = BookmarkContainer[containerEnumVal]; // Get container @@ -367,6 +372,7 @@ export default abstract class WebExtBookmarkService { if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { this.logSvc.logInfo('Not populating toolbar'); + // eslint-disable-next-line no-continue continue; } } @@ -458,7 +464,8 @@ export default abstract class WebExtBookmarkService { if (angular.isUndefined(nativeBookmarkId)) return this.$q.resolve(''); return this.getNativeContainerIds().then((nativeContainerIds) => { - for (const [nativeBookmarkNodeId, containerName] of nativeContainerIds.entries()) { + // eslint-disable-next-line no-restricted-syntax + for (const [containerName, nativeBookmarkNodeId] of nativeContainerIds.entries()) { if (nativeBookmarkNodeId === nativeBookmarkId) return containerName; } return ''; @@ -496,6 +503,7 @@ export default abstract class WebExtBookmarkService { return this.settingsSvc.syncBookmarksToolbar().then((syncBookmarksToolbar) => { const getBookmarkPromises = new Array]>>(); + // eslint-disable-next-line no-restricted-syntax for (const containerEnumVal of Object.keys(BookmarkContainer)) { const containerName: BookmarkContainer = BookmarkContainer[containerEnumVal]; // Get native bookmark node id @@ -504,6 +512,7 @@ export default abstract class WebExtBookmarkService { if (containerName === BookmarkContainer.Toolbar) { if (!syncBookmarksToolbar) { // skip + // eslint-disable-next-line no-continue continue; } } @@ -512,6 +521,7 @@ export default abstract class WebExtBookmarkService { if (nativeBookmarkNodeId) { const getBookmarkPromise: Promise<[BookmarkContainer, Array]> = browser.bookmarks .getSubTree(nativeBookmarkNodeId) + // eslint-disable-next-line @typescript-eslint/no-loop-func .then((subTree) => { const bookmarksNode = subTree[0]; From 7cc1b74e6dcc734b25621c0d4ee3c483e0c22eaf Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Mon, 15 Feb 2021 05:14:05 +0100 Subject: [PATCH 16/19] Add more browser detection features to utility.services --- src/modules/shared/utility/utility.service.ts | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/modules/shared/utility/utility.service.ts b/src/modules/shared/utility/utility.service.ts index 9cc20aa85..77f4ed65e 100644 --- a/src/modules/shared/utility/utility.service.ts +++ b/src/modules/shared/utility/utility.service.ts @@ -16,6 +16,10 @@ import NetworkService from '../network/network.service'; import { StoreKey } from '../store/store.enum'; import StoreService from '../store/store.service'; +declare global { + const opr: any; +} + @autobind @Injectable('UtilityService') export default class UtilityService { @@ -157,6 +161,27 @@ export default class UtilityService { return !angular.isUndefined(window.navigator.brave); } + // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 + // Opera 20.0+ + isOperaBrowser(): boolean { + const windowsAny: any = window; + // eslint-disable-next-line no-undef + return (!!windowsAny.opr && !!opr.addons) || navigator.userAgent.indexOf(' OPR/') >= 0; + } + + // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 + // Chrome 1 - 79 + isChromeLikeBrowser(): boolean { + const windowsAny: any = window; + return !!windowsAny.chrome && (!!windowsAny.chrome.webstore || !!windowsAny.chrome.runtime); + } + + // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 + // Edge (based on chromium) detection + isEdgeChromiumBrowser(): boolean { + return this.isChromeLikeBrowser() && navigator.userAgent.indexOf('Edg') !== -1; + } + isMobilePlatform(platformName: string): boolean { return platformName === PlatformType.Android; } From 018fe8b94eec3d874698af522efa87511d2d172d Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Mon, 15 Feb 2021 05:11:06 +0100 Subject: [PATCH 17/19] Refactor bookmark-service - getNativeContainerIds into a shared webext- impl. Created two new abstract methods: getNativeContainerInfo() -> returns browser-specific info (id + throwIfNotFound) for a given BookmarkContainer getDefaultNativeContainerCandidates() -> returns the candidates for being the default containers for creation of unsupported containers Field unsupportedContainers was refactored into getUnsupportedContainers() method. Added getSupportedContainers(). NativeContainersInfo.platformDefaultBookmarksNodeId renamed to .defaultNativeContainerId Refactored and moved ensureContainersExist(...) to webext- impl. It uses the getSupportedContainers() Created a new webext- impl. method identifySupportedContainers(). This method initializes the supported containers cache. This method must be called at least once after the extension is loaded, before a call to getSupportedContainers(), getUnsupportedContainers() or ensureContainersExist (...) is attempted. Currently, identifySupportedContainers() is called before any process*Sync() in bookmark-sync-provider can take place, but should be probably moved to some module/extension init code. Maybe the whole "supported-containers" logic should be moved to different (new?) service. --- .../android-bookmark.service.ts | 4 + src/modules/shared/bookmark/bookmark.enum.ts | 10 +- .../shared/bookmark/bookmark.interface.ts | 1 + .../bookmark-sync-provider.service.ts | 38 ++-- .../chromium-bookmark.service.ts | 190 +++++++----------- .../firefox-bookmark.service.ts | 89 ++------ .../webext-bookmark/NativeContainersInfo.ts | 2 +- .../webext-bookmark.service.ts | 164 ++++++++++++--- 8 files changed, 259 insertions(+), 239 deletions(-) diff --git a/src/modules/android/android-shared/android-bookmark/android-bookmark.service.ts b/src/modules/android/android-shared/android-bookmark/android-bookmark.service.ts index e009b6c42..3abd0c8a9 100644 --- a/src/modules/android/android-shared/android-bookmark/android-bookmark.service.ts +++ b/src/modules/android/android-shared/android-bookmark/android-bookmark.service.ts @@ -30,6 +30,10 @@ export default class AndroidBookmarkService implements BookmarkService { return bookmarks; } + identifySupportedContainers(): ng.IPromise { + return this.methodNotApplicable(); + } + methodNotApplicable(): ng.IPromise { // Unused for this platform return this.$q.resolve(); diff --git a/src/modules/shared/bookmark/bookmark.enum.ts b/src/modules/shared/bookmark/bookmark.enum.ts index e25724093..c7d8a2399 100644 --- a/src/modules/shared/bookmark/bookmark.enum.ts +++ b/src/modules/shared/bookmark/bookmark.enum.ts @@ -13,14 +13,6 @@ enum BookmarkContainer { Toolbar = '[xbs] Toolbar' } -// TODO: maybe remove this new code and use solely (un)supported bookmark detection at runtime -// OR! always create all containers, so that merging in the future is simpler? -const MandatoryBookmarkContainers: Array = [ - BookmarkContainer.Other, - BookmarkContainer.Mobile, - BookmarkContainer.Toolbar -]; - enum BookmarkType { Bookmark = 'bookmark', Container = 'container', @@ -28,4 +20,4 @@ enum BookmarkType { Separator = 'separator' } -export { BookmarkChangeType, BookmarkContainer, BookmarkType, MandatoryBookmarkContainers }; +export { BookmarkChangeType, BookmarkContainer, BookmarkType }; diff --git a/src/modules/shared/bookmark/bookmark.interface.ts b/src/modules/shared/bookmark/bookmark.interface.ts index 2e4297ac1..b6e899c2e 100644 --- a/src/modules/shared/bookmark/bookmark.interface.ts +++ b/src/modules/shared/bookmark/bookmark.interface.ts @@ -41,6 +41,7 @@ export interface BookmarkService { clearNativeBookmarks: () => ng.IPromise; createNativeBookmarksFromBookmarks: (bookmarks: Bookmark[]) => ng.IPromise; ensureContainersExist: (bookmarks: Bookmark[]) => Bookmark[]; + identifySupportedContainers(): ng.IPromise; processNativeChangeOnBookmarks: (changeInfo: BookmarkChange, bookmarks: Bookmark[]) => ng.IPromise; processChangeOnNativeBookmarks: ( id: number, diff --git a/src/modules/shared/sync/bookmark-sync-provider/bookmark-sync-provider.service.ts b/src/modules/shared/sync/bookmark-sync-provider/bookmark-sync-provider.service.ts index e3d6d6c37..0bffb8b1f 100644 --- a/src/modules/shared/sync/bookmark-sync-provider/bookmark-sync-provider.service.ts +++ b/src/modules/shared/sync/bookmark-sync-provider/bookmark-sync-provider.service.ts @@ -137,24 +137,26 @@ export default class BookmarkSyncProviderService implements SyncProvider { } processSync(sync: Sync): ng.IPromise { - // Process sync - switch (sync.type) { - // Sync native bookmarks to service - case SyncType.Remote: - return this.processRemoteSync(sync); - // Overwrite native bookmarks with synced bookmarks - case SyncType.Local: - return this.processLocalSync(sync); - // Sync bookmarks to service and overwrite native bookmarks - case SyncType.LocalAndRemote: - return this.processLocalAndRemoteSync(sync); - // Upgrade sync to current version - case SyncType.Upgrade: - return this.processUpgradeSync(); - // Ambiguous sync - default: - throw new Exceptions.AmbiguousSyncRequestException(); - } + return this.bookmarkSvc.identifySupportedContainers().then(() => { + // Process sync + switch (sync.type) { + // Sync native bookmarks to service + case SyncType.Remote: + return this.processRemoteSync(sync); + // Overwrite native bookmarks with synced bookmarks + case SyncType.Local: + return this.processLocalSync(sync); + // Sync bookmarks to service and overwrite native bookmarks + case SyncType.LocalAndRemote: + return this.processLocalAndRemoteSync(sync); + // Upgrade sync to current version + case SyncType.Upgrade: + return this.processUpgradeSync(); + // Ambiguous sync + default: + throw new Exceptions.AmbiguousSyncRequestException(); + } + }); } processLocalAndRemoteSync(sync: Sync): ng.IPromise { diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index 5d3702ce9..f692f0279 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -2,15 +2,9 @@ import angular from 'angular'; import { Injectable } from 'angular-ts-decorators'; import autobind from 'autobind-decorator'; import { Bookmarks as NativeBookmarks, browser } from 'webextension-polyfill-ts'; -import { - BookmarkChangeType, - BookmarkContainer, - BookmarkType, - MandatoryBookmarkContainers -} from '../../../../shared/bookmark/bookmark.enum'; +import { BookmarkChangeType, BookmarkContainer, BookmarkType } from '../../../../shared/bookmark/bookmark.enum'; import { AddNativeBookmarkChangeData, - Bookmark, BookmarkChange, ModifyNativeBookmarkChangeData, MoveNativeBookmarkChangeData @@ -18,18 +12,11 @@ import { import * as Exceptions from '../../../../shared/exception/exception'; import Globals from '../../../../shared/global-shared.constants'; import { WebpageMetadata } from '../../../../shared/global-shared.interface'; -import { NativeContainersInfo } from '../../../shared/webext-bookmark/NativeContainersInfo'; import WebExtBookmarkService from '../../../shared/webext-bookmark/webext-bookmark.service'; @autobind @Injectable('BookmarkService') export default class ChromiumBookmarkService extends WebExtBookmarkService { - otherBookmarksNodeTitle = 'Other bookmarks'; - toolbarBookmarksNodeTitle = 'Bookmarks bar'; - menuBookmarksNodeTitle = 'Menu bookmarks'; - mobileBookmarksNodeTitle = 'Mobile bookmarks'; - unsupportedContainers = [BookmarkContainer.Menu, BookmarkContainer.Mobile]; - convertNativeBookmarkToSeparator( bookmark: NativeBookmarks.BookmarkTreeNode ): ng.IPromise { @@ -134,32 +121,6 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { }); } - // TODO: unify with firefox somehow? - // probably auto-detect all supported containers in getNativeContainer (maybe call it explicitely for detection?) - // and then ensure that all supported containers are created - ensureContainersExist(bookmarks: Bookmark[]): Bookmark[] { - if (angular.isUndefined(bookmarks)) { - return; - } - - // Add supported containers - const bookmarksToReturn = angular.copy(bookmarks); - MandatoryBookmarkContainers.forEach((element) => { - this.bookmarkHelperSvc.getContainer(element, bookmarksToReturn, true); - }); - - // Return sorted containers - return bookmarksToReturn.sort((x, y) => { - if (x.title < y.title) { - return -1; - } - if (x.title > y.title) { - return 1; - } - return 0; - }); - } - getNativeBookmarksWithSeparators( nativeBookmarks: NativeBookmarks.BookmarkTreeNode[] ): NativeBookmarks.BookmarkTreeNode[] { @@ -172,90 +133,83 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { return nativeBookmarks; } - // TODO: unify/share more code with firefox - getNativeContainerIds(): ng.IPromise { - const containerIds = new NativeContainersInfo(); - // Populate container ids - return browser.bookmarks.getTree().then((tree) => { - // Get the root child nodes - let otherBookmarksNode = tree[0].children.find((x) => { - return x.title === this.otherBookmarksNodeTitle; - }); - let toolbarBookmarksNode = tree[0].children.find((x) => { - return x.title === this.toolbarBookmarksNodeTitle; - }); - let menuBookmarksNode = tree[0].children.find((x) => { - return x.title === this.menuBookmarksNodeTitle; - }); - let mobileBookmarksNode = tree[0].children.find((x) => { - return x.title === this.mobileBookmarksNodeTitle; - }); + browserDetection: { isOpera: boolean }; + getBrowserDetection() { + if (this.browserDetection) return this.browserDetection; - // TODO: improve this logic - const defaultBookmarksNode = otherBookmarksNode || mobileBookmarksNode; - if (!defaultBookmarksNode) { - // coulnd not find a default container to create folders to place other containers in - throw new Exceptions.ContainerNotFoundException(); - } + const browserDetection: any = {}; + browserDetection.isChromeLike = this.utilitySvc.isChromeLikeBrowser(); + browserDetection.isOpera = this.utilitySvc.isOperaBrowser(); + browserDetection.isEdgeChromium = this.utilitySvc.isEdgeChromiumBrowser(); - // TODO: FINISH THIS! - // is related to createNativeBookmarksFromBookmarks - // HACK!!!! - this.unsupportedContainers = []; + this.browserDetection = browserDetection; + return this.browserDetection; + } - // Check for unsupported containers - if (!otherBookmarksNode) { - this.logSvc.logWarning('Unsupported container: other bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Other); - otherBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Other; - }); - } - if (!toolbarBookmarksNode) { - this.logSvc.logWarning('Unsupported container: toolbar bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Toolbar); - toolbarBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Toolbar; - }); - } - if (!menuBookmarksNode) { - this.logSvc.logWarning('Unsupported container: menu bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Menu); - menuBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Menu; - }); - } - if (!mobileBookmarksNode) { - this.logSvc.logWarning('Unsupported container: mobile bookmarks'); - // HACK!!!! - this.unsupportedContainers.push(BookmarkContainer.Mobile); - mobileBookmarksNode = defaultBookmarksNode.children.find((x) => { - return x.title === BookmarkContainer.Mobile; + chromiumSupportedContainersInfo = { + map: new Map([ + [BookmarkContainer.Toolbar, { id: '1', throwIfNotFound: false }], + [BookmarkContainer.Other, { id: '2', throwIfNotFound: false }], + [BookmarkContainer.Mobile, { id: '3', throwIfNotFound: false }] + ]), + default: [BookmarkContainer.Other, BookmarkContainer.Mobile] + }; + + operaSupportedContainersInfo = { + map: new Map([ + [BookmarkContainer.Toolbar, { id: 'bookmarks_bar', throwIfNotFound: false }], + [BookmarkContainer.Other, { id: 'other', throwIfNotFound: true }] + // [, { id: 'unsorted', throwIfNotFound: false }], + // [, { id: 'user_root', throwIfNotFound: false }], + // [, { id: 'shared', throwIfNotFound: false }], + // [, { id: 'trash', throwIfNotFound: false }], + // [, { id: 'speed_dial', throwIfNotFound: false }], + ]), + default: [BookmarkContainer.Other] + }; + + getNativeContainerInfo(containerName: BookmarkContainer): ng.IPromise<{ id?: string; throwIfNotFound: boolean }> { + const browserDetection = this.getBrowserDetection(); + if (browserDetection.isOpera) { + const getByName: ( + id: string, + callback: (node: NativeBookmarks.BookmarkTreeNode) => void + ) => void = (browser as any).bookmarks.getRootByName; + + const baseInfo = this.operaSupportedContainersInfo.map.get(containerName); + if (baseInfo) { + return this.$q((resolve) => { + getByName(baseInfo.id, (node) => { + resolve({ id: node.id, throwIfNotFound: baseInfo.throwIfNotFound }); + }); }); + // eslint-disable-next-line no-else-return + } else { + return this.$q.resolve({ id: undefined, throwIfNotFound: false }); } + // eslint-disable-next-line no-else-return + } else { + return browser.bookmarks.getTree().then((tree) => { + const baseInfo = this.chromiumSupportedContainersInfo.map.get(containerName); + let info: { id?: string; throwIfNotFound: boolean }; + if (baseInfo && tree[0].children!.find((x) => x.id === baseInfo.id)) { + info = { ...baseInfo }; // make a copy + } else { + info = { id: undefined, throwIfNotFound: false }; + } + return info; + }); + } + } - // Add container ids to result - { - // must be always defined! - containerIds.platformDefaultBookmarksNodeId = defaultBookmarksNode.id; - } - if (!angular.isUndefined(otherBookmarksNode)) { - containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); - } - if (!angular.isUndefined(toolbarBookmarksNode)) { - containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); - } - if (!angular.isUndefined(menuBookmarksNode)) { - containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); - } - if (!angular.isUndefined(mobileBookmarksNode)) { - containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); - } - return containerIds; - }); + getDefaultNativeContainerCandidates(): BookmarkContainer[] { + const browserDetection = this.getBrowserDetection(); + if (browserDetection.isOpera) { + return this.operaSupportedContainersInfo.default; + // eslint-disable-next-line no-else-return + } else { + return this.chromiumSupportedContainersInfo.default; + } } isSeparator(nativeBookmark: NativeBookmarks.BookmarkTreeNode): boolean { diff --git a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts index 304e16302..eb61b2e5d 100644 --- a/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts +++ b/src/modules/webext/firefox/shared/firefox-bookmark/firefox-bookmark.service.ts @@ -4,7 +4,6 @@ import { Bookmarks as NativeBookmarks, browser } from 'webextension-polyfill-ts' import { BookmarkChangeType, BookmarkContainer } from '../../../../shared/bookmark/bookmark.enum'; import { AddNativeBookmarkChangeData, - Bookmark, BookmarkChange, ModifyNativeBookmarkChangeData, MoveNativeBookmarkChangeData @@ -12,13 +11,10 @@ import { import * as Exceptions from '../../../../shared/exception/exception'; import { WebpageMetadata } from '../../../../shared/global-shared.interface'; import WebExtBookmarkService from '../../../shared/webext-bookmark/webext-bookmark.service'; -import { NativeContainersInfo } from "../../../shared/webext-bookmark/NativeContainersInfo"; @autobind @Injectable('BookmarkService') export default class FirefoxBookmarkService extends WebExtBookmarkService { - unsupportedContainers = []; - createNativeSeparator(parentId: string): ng.IPromise { const newSeparator: NativeBookmarks.CreateDetails = { parentId, @@ -65,31 +61,6 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { }); } - // TODO: unify, see chromium version - ensureContainersExist(bookmarks: Bookmark[]): Bookmark[] { - if (angular.isUndefined(bookmarks)) { - return; - } - - // Add supported containers - const bookmarksToReturn = angular.copy(bookmarks); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Menu, bookmarksToReturn, true); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Mobile, bookmarksToReturn, true); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Other, bookmarksToReturn, true); - this.bookmarkHelperSvc.getContainer(BookmarkContainer.Toolbar, bookmarksToReturn, true); - - // Return sorted containers - return bookmarksToReturn.sort((x, y) => { - if (x.title < y.title) { - return -1; - } - if (x.title > y.title) { - return 1; - } - return 0; - }); - } - fixMultipleMoveOldIndexes(): void { const processBatch = (batch) => { // Adjust oldIndexes if bookmarks moved to different parent or to higher indexes @@ -136,52 +107,30 @@ export default class FirefoxBookmarkService extends WebExtBookmarkService { } } - // TODO: unify/share more code with chromium - getNativeContainerIds(): ng.IPromise { - const containerIds = new NativeContainersInfo(); - // Populate container ids - return browser.bookmarks.getTree().then((tree) => { - // Get the root child nodes - const menuBookmarksNode = tree[0].children.find((x) => { - return x.id === 'menu________'; - }); - const mobileBookmarksNode = tree[0].children.find((x) => { - return x.id === 'mobile______'; - }); - const otherBookmarksNode = tree[0].children.find((x) => { - return x.id === 'unfiled_____'; - }); - const toolbarBookmarksNode = tree[0].children.find((x) => { - return x.id === 'toolbar_____'; - }); + supportedContainersInfo = new Map([ + [BookmarkContainer.Menu, { id: 'menu________', throwIfNotFound: true }], + [BookmarkContainer.Other, { id: 'unfiled_____', throwIfNotFound: true }], + [BookmarkContainer.Toolbar, { id: 'toolbar_____', throwIfNotFound: true }], + [BookmarkContainer.Mobile, { id: 'mobile______', throwIfNotFound: true }] + ]); - // Throw an error if a native container is not found - if (!menuBookmarksNode || !mobileBookmarksNode || !otherBookmarksNode || !toolbarBookmarksNode) { - if (!menuBookmarksNode) { - this.logSvc.logWarning('Missing container: menu bookmarks'); - } - if (!mobileBookmarksNode) { - this.logSvc.logWarning('Missing container: mobile bookmarks'); - } - if (!otherBookmarksNode) { - this.logSvc.logWarning('Missing container: other bookmarks'); - } - if (!toolbarBookmarksNode) { - this.logSvc.logWarning('Missing container: toolbar bookmarks'); - } - throw new Exceptions.ContainerNotFoundException(); + getNativeContainerInfo(containerName: BookmarkContainer): ng.IPromise<{ id?: string; throwIfNotFound: boolean }> { + return browser.bookmarks.getTree().then((tree) => { + const baseInfo = this.supportedContainersInfo.get(containerName); + let info: { id?: string; throwIfNotFound: boolean }; + if (baseInfo && tree[0].children!.find((x) => x.id === baseInfo.id)) { + info = { ...baseInfo }; // make a copy + } else { + info = { id: undefined, throwIfNotFound: false }; } - - // Add container ids to result - containerIds.platformDefaultBookmarksNodeId = otherBookmarksNode.id; - containerIds.set(BookmarkContainer.Menu, menuBookmarksNode.id); - containerIds.set(BookmarkContainer.Mobile, mobileBookmarksNode.id); - containerIds.set(BookmarkContainer.Other, otherBookmarksNode.id); - containerIds.set(BookmarkContainer.Toolbar, toolbarBookmarksNode.id); - return containerIds; + return info; }); } + getDefaultNativeContainerCandidates(): BookmarkContainer[] { + return [BookmarkContainer.Other]; + } + processNativeBookmarkEventsQueue(): void { // Fix incorrect oldIndex values for multiple moves // https://bugzilla.mozilla.org/show_bug.cgi?id=1556427 diff --git a/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts b/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts index d5502551f..e0f52663b 100644 --- a/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts +++ b/src/modules/webext/shared/webext-bookmark/NativeContainersInfo.ts @@ -1,5 +1,5 @@ import { BookmarkContainer } from '../../../shared/bookmark/bookmark.enum'; export class NativeContainersInfo extends Map { - platformDefaultBookmarksNodeId: string; + defaultNativeContainerId: string; } diff --git a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts index f4d2d380e..45649e9af 100644 --- a/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts +++ b/src/modules/webext/shared/webext-bookmark/webext-bookmark.service.ts @@ -7,6 +7,7 @@ import { Bookmark, BookmarkChange, BookmarkMetadata, + BookmarkService, ModifyNativeBookmarkChangeData, MoveNativeBookmarkChangeData, OnChildrenReorderedReorderInfoType, @@ -30,7 +31,7 @@ import BookmarkIdMapperService from '../bookmark-id-mapper/bookmark-id-mapper.se import { NativeContainersInfo } from './NativeContainersInfo'; @autobind -export default abstract class WebExtBookmarkService { +export default abstract class WebExtBookmarkService implements BookmarkService { $injector: ng.auto.IInjectorService; $q: ng.IQService; $timeout: ng.ITimeoutService; @@ -45,7 +46,6 @@ export default abstract class WebExtBookmarkService { nativeBookmarkEventsQueue: any[] = []; processNativeBookmarkEventsTimeout: ng.IPromise; - unsupportedContainers: BookmarkContainer[] = []; static $inject = [ '$injector', @@ -155,9 +155,9 @@ export default abstract class WebExtBookmarkService { // Skip over any unsupported container mount-point bookmark folders present, // if we are now "in" the platform-default bookmark node // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry - if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + if (nativeBookmarkNodeId === nativeContainerIds.defaultNativeContainerId) { bookmarksNodeChildren = bookmarksNode.children.filter( - (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + (x) => !this.getUnsupportedContainers().includes(x.title as BookmarkContainer) ); } else { bookmarksNodeChildren = bookmarksNode.children; @@ -244,9 +244,9 @@ export default abstract class WebExtBookmarkService { .then((children) => { // TODO: alternatively the other way arround... do not clear unsupported containers bookmark nodes but clear the default bookmark node completely // Do not remove the bookmark-folders that server as mount-point for unsupported containers - if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + if (nativeBookmarkNodeId === nativeContainerIds.defaultNativeContainerId) { children = children.filter( - (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + (x) => !this.getUnsupportedContainers().includes(x.title as BookmarkContainer) ); } return this.$q.all( @@ -305,7 +305,7 @@ export default abstract class WebExtBookmarkService { // Get native container ids return this.getNativeContainerIds().then((nativeContainerIds) => { // No containers to adjust for if parent is not platform-default bookmarks node - if (parentId !== nativeContainerIds.platformDefaultBookmarksNodeId) { + if (parentId !== nativeContainerIds.defaultNativeContainerId) { return 0; } @@ -388,7 +388,7 @@ export default abstract class WebExtBookmarkService { } else { // there is no nativeContainerId -> it's not a natively supported container // -> create it's mount-point bookmark folder now - parentNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; + parentNodeId = nativeContainerIds.defaultNativeContainerId; childrenToCreate = [container]; } const populatePromise = browser.bookmarks @@ -458,7 +458,28 @@ export default abstract class WebExtBookmarkService { abstract enableEventListeners(): ng.IPromise; - abstract ensureContainersExist(bookmarks: Bookmark[]): Bookmark[]; + ensureContainersExist(bookmarks: Bookmark[]): Bookmark[] { + if (angular.isUndefined(bookmarks)) { + return undefined!; + } + + // Add supported containers + const bookmarksToReturn = angular.copy(bookmarks); + this.getSupportedContainers().forEach((element) => { + this.bookmarkHelperSvc.getContainer(element, bookmarksToReturn, true); + }); + + // Return sorted containers + return bookmarksToReturn.sort((x, y) => { + if (x.title! < y.title!) { + return -1; + } + if (x.title! > y.title!) { + return 1; + } + return 0; + }); + } getContainerNameFromNativeId(nativeBookmarkId: string): ng.IPromise { if (angular.isUndefined(nativeBookmarkId)) return this.$q.resolve(''); @@ -533,9 +554,9 @@ export default abstract class WebExtBookmarkService { // Skip over any unsupported container mount-point bookmark folders present, // if we are now "in" the platform-default bookmark node. // The skipped bookmarks will be processed in this loop for their own nativeContainerIds entry - if (nativeBookmarkNodeId === nativeContainerIds.platformDefaultBookmarksNodeId) { + if (nativeBookmarkNodeId === nativeContainerIds.defaultNativeContainerId) { bookmarksNodeChildren = bookmarksNode.children.filter( - (x) => !this.unsupportedContainers.includes(x.title as BookmarkContainer) + (x) => !this.getUnsupportedContainers().includes(x.title as BookmarkContainer) ); } else { bookmarksNodeChildren = bookmarksNode.children; @@ -616,6 +637,70 @@ export default abstract class WebExtBookmarkService { return nativeBookmarks; } + /** + * to be overridden; used in getNativeContainerIds() + * + * id: the native id, if it is supported \ + * throwIfNotFound: whether getNativeContainerIds should throw an exception, when the id is undefined + */ + abstract getNativeContainerInfo( + containerName: BookmarkContainer + ): ng.IPromise<{ id?: string; throwIfNotFound: boolean }>; + + /** + * to be overridden; used in getNativeContainerIds() + */ + abstract getDefaultNativeContainerCandidates(): BookmarkContainer[]; + + unsupportedNativeContainerCache: BookmarkContainer[]; + supportedNativeContainerCache: BookmarkContainer[]; + supportedNativeContainerIdsCache: Map; + + /** wrapper for unsupportedNativeContainerCache */ + getUnsupportedContainers(): BookmarkContainer[] { + return this.unsupportedNativeContainerCache; + } + + /** wrapper for supportedNativeContainerCache */ + getSupportedContainers(): BookmarkContainer[] { + return this.supportedNativeContainerCache; + } + + /** + * must be called before any getSupportedContainers() / getUnsupportedContainers() calls + */ + identifySupportedContainers(): ng.IPromise { + let promise: ng.IPromise; + if (this.supportedNativeContainerCache === undefined) { + // initialize + this.supportedNativeContainerCache = []; + this.supportedNativeContainerIdsCache = new Map(); + + const promises = Object.values(BookmarkContainer).map((containerName) => { + return this.getNativeContainerInfo(containerName).then((info) => { + if (info.id) { + // add to supported cache + this.supportedNativeContainerCache.push(containerName); + this.supportedNativeContainerIdsCache.set(containerName, info.id); + } else { + this.logSvc.logWarning(`Missing container for: ${containerName}`); + if (info.throwIfNotFound) { + throw new Exceptions.ContainerNotFoundException(); + } + } + }); + }); + promise = this.$q.all(promises).then(() => { + this.unsupportedNativeContainerCache = Object.values(BookmarkContainer).filter( + (bc) => !this.supportedNativeContainerCache.includes(bc) + ); + }); + return promise; + } + // else + return this.$q.resolve(); + } + /** * Returns the mapping of BookmarkContainer to native BookmarkTreeNode ids. * For natively supported containers, their ids are returned. @@ -623,7 +708,44 @@ export default abstract class WebExtBookmarkService { * 1) they are children of the browser-default container; * 2) the name of the folder equals to the name of the bookmark container. */ - abstract getNativeContainerIds(): ng.IPromise; + getNativeContainerIds(): ng.IPromise { + return this.identifySupportedContainers().then(() => { + const containerIds = new NativeContainersInfo(this.supportedNativeContainerIdsCache); + + // Throw an error if a default container is not found + let defaultNativeContainerId: string | undefined; + // eslint-disable-next-line no-restricted-syntax + for (const candidate of this.getDefaultNativeContainerCandidates()) { + defaultNativeContainerId = containerIds.get(candidate); + if (defaultNativeContainerId) break; + } + + if (!defaultNativeContainerId) { + // could not find a default container to create folders to mount natively unsupported containers into + throw new Exceptions.ContainerNotFoundException(); + } + containerIds.defaultNativeContainerId = defaultNativeContainerId; + + // if all BookmarkContainer have now associated IDs, return + if (!Object.values(BookmarkContainer).find((containerName) => containerIds.get(containerName) === undefined)) { + return containerIds; + } + + return browser.bookmarks.getTree().then((tree) => { + const defaultBookmarksNode = tree[0].children!.find((x) => { + return x.id === defaultNativeContainerId; + })!; + // eslint-disable-next-line no-restricted-syntax + for (const containerName of Object.values(BookmarkContainer)) { + if (!containerIds.get(containerName)) { + const mountPointNode = defaultBookmarksNode.children!.find((x) => x.title === containerName); + if (mountPointNode) containerIds.set(containerName, mountPointNode.id); + } + } + return containerIds; + }); + }); + } getSupportedUrl(url?: string): string { if (angular.isUndefined(url ?? undefined)) { @@ -807,8 +929,7 @@ export default abstract class WebExtBookmarkService { // Create native bookmark in platform default bookmarks container return this.getNativeContainerIds() .then((nativeContainerIds) => { - const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; - return this.createNativeBookmark(platformDefaultBookmarksNodeId, createInfo.title, createInfo.url); + return this.createNativeBookmark(nativeContainerIds.defaultNativeContainerId, createInfo.title, createInfo.url); }) .then((newNativeBookmark) => { // Add id mapping for new bookmark @@ -1153,7 +1274,7 @@ export default abstract class WebExtBookmarkService { reorderUnsupportedContainers(): ng.IPromise { // Get unsupported containers - return this.$q.all(this.unsupportedContainers.map(this.getNativeBookmarkByTitle)).then((results) => { + return this.$q.all(this.getUnsupportedContainers().map(this.getNativeBookmarkByTitle)).then((results) => { return this.$q .all( results @@ -1231,12 +1352,9 @@ export default abstract class WebExtBookmarkService { .isSyncEnabled() .then((syncEnabled) => (syncEnabled ? this.bookmarkHelperSvc.getCachedBookmarks() : undefined)) ]).then(([nativeContainerIds, bookmarks]) => { - // If parent is not platform-default bookmarks, no container was changed - const platformDefaultBookmarksNodeId = nativeContainerIds.platformDefaultBookmarksNodeId; - if ( - angular.isDefined(changedNativeBookmark) && - changedNativeBookmark.parentId !== platformDefaultBookmarksNodeId - ) { + // If parent is not browser-default container, no (natively unsupported) container was changed + const defaultNativeContainerId = nativeContainerIds.defaultNativeContainerId; + if (angular.isDefined(changedNativeBookmark) && changedNativeBookmark.parentId !== defaultNativeContainerId) { return false; } @@ -1249,11 +1367,11 @@ export default abstract class WebExtBookmarkService { } return browser.bookmarks - .getChildren(platformDefaultBookmarksNodeId) + .getChildren(defaultNativeContainerId) .then((children) => { // Get all native bookmarks - in platform-default bookmarks node - that are unsupported containers and check for duplicates const containers = children - .filter((x) => this.unsupportedContainers.includes(x.title as BookmarkContainer)) + .filter((x) => this.getUnsupportedContainers().includes(x.title as BookmarkContainer)) .map((x) => x.title); return containers.length !== new Set(containers).size; }) From cbda400716d5bbbcc7b936c598f87c227b25ea0a Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Tue, 16 Feb 2021 00:11:44 +0100 Subject: [PATCH 18/19] Remove remaining direct BookmarkContainer references --- .../backup-restore-settings.component.ts | 2 +- .../bookmark-helper.service.ts | 37 +------------------ src/modules/shared/bookmark/bookmark.enum.ts | 3 ++ 3 files changed, 6 insertions(+), 36 deletions(-) diff --git a/src/modules/app/app-settings/backup-restore-settings/backup-restore-settings.component.ts b/src/modules/app/app-settings/backup-restore-settings/backup-restore-settings.component.ts index 644987596..759dd265d 100644 --- a/src/modules/app/app-settings/backup-restore-settings/backup-restore-settings.component.ts +++ b/src/modules/app/app-settings/backup-restore-settings/backup-restore-settings.component.ts @@ -287,7 +287,7 @@ export default class BackupRestoreSettingsComponent implements OnInit { }) .then((bookmarks) => { // Clean bookmarks for export - return cleanRecursive(this.bookmarkHelperSvc.removeEmptyContainers(bookmarks)); + return cleanRecursive(bookmarks.filter((container) => container.children?.length)); }); } diff --git a/src/modules/shared/bookmark/bookmark-helper/bookmark-helper.service.ts b/src/modules/shared/bookmark/bookmark-helper/bookmark-helper.service.ts index 984132706..ab7c62eb7 100644 --- a/src/modules/shared/bookmark/bookmark-helper/bookmark-helper.service.ts +++ b/src/modules/shared/bookmark/bookmark-helper/bookmark-helper.service.ts @@ -213,15 +213,8 @@ export default class BookmarkHelperService { } getBookmarkType(bookmark: Bookmark): BookmarkType { - const bookmarkType = BookmarkType.Bookmark; - // Check if container - if ( - bookmark.title === BookmarkContainer.Menu || - bookmark.title === BookmarkContainer.Mobile || - bookmark.title === BookmarkContainer.Other || - bookmark.title === BookmarkContainer.Toolbar - ) { + if (Object.values(BookmarkContainer).includes(bookmark.title as BookmarkContainer)) { return BookmarkType.Container; } @@ -235,7 +228,7 @@ export default class BookmarkHelperService { return BookmarkType.Separator; } - return bookmarkType; + return BookmarkType.Bookmark; } getCachedBookmarks(): ng.IPromise { @@ -522,32 +515,6 @@ export default class BookmarkHelperService { return this.$q.resolve(updatedBookmarks); } - removeEmptyContainers(bookmarks: Bookmark[]): Bookmark[] { - const menuContainer = this.getContainer(BookmarkContainer.Menu, bookmarks); - const mobileContainer = this.getContainer(BookmarkContainer.Mobile, bookmarks); - const otherContainer = this.getContainer(BookmarkContainer.Other, bookmarks); - const toolbarContainer = this.getContainer(BookmarkContainer.Toolbar, bookmarks); - const removeArr: Bookmark[] = []; - - if (!menuContainer?.children?.length) { - removeArr.push(menuContainer); - } - - if (!mobileContainer?.children?.length) { - removeArr.push(mobileContainer); - } - - if (!otherContainer?.children?.length) { - removeArr.push(otherContainer); - } - - if (!toolbarContainer?.children?.length) { - removeArr.push(toolbarContainer); - } - - return bookmarks.filter((x) => !removeArr.includes(x)); - } - searchBookmarks(query: any): ng.IPromise { if (!query) { query = { keywords: [] }; diff --git a/src/modules/shared/bookmark/bookmark.enum.ts b/src/modules/shared/bookmark/bookmark.enum.ts index c7d8a2399..f0a628dfc 100644 --- a/src/modules/shared/bookmark/bookmark.enum.ts +++ b/src/modules/shared/bookmark/bookmark.enum.ts @@ -6,6 +6,9 @@ enum BookmarkChangeType { Remove = 'remove' } +// when adding a new container, add a translation to bookmark-helper.service.ts getBookmarkTitleForDisplay(...) +// do NOT reference any of these constants unless you absolutely have to +// (e.g. .Toolbar in conjunction with SettingsSvc.syncBookmarksToolbar() ) enum BookmarkContainer { Menu = '[xbs] Menu', Mobile = '[xbs] Mobile', From 80200964173f195cd6c3451d89fff4af8bab93e6 Mon Sep 17 00:00:00 2001 From: Michal Kotoun Date: Tue, 16 Feb 2021 00:12:09 +0100 Subject: [PATCH 19/19] Browser detection comments/logging --- src/modules/shared/utility/utility.service.ts | 8 ++------ .../shared/chromium-bookmark/chromium-bookmark.service.ts | 2 ++ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/modules/shared/utility/utility.service.ts b/src/modules/shared/utility/utility.service.ts index 77f4ed65e..657b73508 100644 --- a/src/modules/shared/utility/utility.service.ts +++ b/src/modules/shared/utility/utility.service.ts @@ -162,21 +162,17 @@ export default class UtilityService { } // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 - // Opera 20.0+ + // Opera 20 - 74 isOperaBrowser(): boolean { const windowsAny: any = window; // eslint-disable-next-line no-undef return (!!windowsAny.opr && !!opr.addons) || navigator.userAgent.indexOf(' OPR/') >= 0; } - - // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 - // Chrome 1 - 79 + // Chrome 1 - 88 isChromeLikeBrowser(): boolean { const windowsAny: any = window; return !!windowsAny.chrome && (!!windowsAny.chrome.webstore || !!windowsAny.chrome.runtime); } - - // as per https://stackoverflow.com/questions/9847580/how-to-detect-safari-chrome-ie-firefox-and-opera-browser/9851769#9851769 // Edge (based on chromium) detection isEdgeChromiumBrowser(): boolean { return this.isChromeLikeBrowser() && navigator.userAgent.indexOf('Edg') !== -1; diff --git a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts index f692f0279..855d248a6 100644 --- a/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts +++ b/src/modules/webext/chromium/shared/chromium-bookmark/chromium-bookmark.service.ts @@ -171,6 +171,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { getNativeContainerInfo(containerName: BookmarkContainer): ng.IPromise<{ id?: string; throwIfNotFound: boolean }> { const browserDetection = this.getBrowserDetection(); if (browserDetection.isOpera) { + this.logSvc.logInfo('detected browser: Opera'); const getByName: ( id: string, callback: (node: NativeBookmarks.BookmarkTreeNode) => void @@ -189,6 +190,7 @@ export default class ChromiumBookmarkService extends WebExtBookmarkService { } // eslint-disable-next-line no-else-return } else { + this.logSvc.logInfo('detected browser: generic Chromium'); return browser.bookmarks.getTree().then((tree) => { const baseInfo = this.chromiumSupportedContainersInfo.map.get(containerName); let info: { id?: string; throwIfNotFound: boolean };