Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Commit

Permalink
Fix bookmark importer and import sequence (#10272)
Browse files Browse the repository at this point in the history
* Fix bookmark importer by making bookmarkList immutable
* Fix importer to import in order
  • Loading branch information
ayumi authored and NejcZdovc committed Aug 4, 2017
1 parent 8c96197 commit 0b6e7be
Show file tree
Hide file tree
Showing 4 changed files with 88 additions and 46 deletions.
3 changes: 2 additions & 1 deletion app/common/state/bookmarksState.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ const bookmarksState = {
}

const key = bookmarkUtil.getKey(bookmarkDetail)
let dataItem = historyState.getSite(state, key)
const historyKey = key.slice(0, -2)
let dataItem = historyState.getSite(state, historyKey)

if (dataItem.isEmpty()) {
// check if we have data in tabs
Expand Down
58 changes: 35 additions & 23 deletions app/importer.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ const locale = require('./locale')
const tabMessageBox = require('./browser/tabMessageBox')
const {makeImmutable} = require('./common/state/immutableUtil')
const bookmarkFoldersUtil = require('./common/lib/bookmarkFoldersUtil')
const FunctionBuffer = require('../js/lib/functionBuffer')

let isImportingBookmarks = false
let hasBookmarks
Expand Down Expand Up @@ -92,7 +93,7 @@ importer.on('add-history-page', (e, history) => {
importer.on('add-homepage', (e, detail) => {
})

const getParentFolderId = (path, pathMap, folders, topLevelFolderId, nextFolderIdObject) => {
const getParentFolderId = (path, pathMap, addFolderFunction, topLevelFolderId, nextFolderIdObject) => {
const pathLen = path.length
if (!pathLen) {
return topLevelFolderId
Expand All @@ -103,10 +104,10 @@ const getParentFolderId = (path, pathMap, folders, topLevelFolderId, nextFolderI
if (parentFolderId === undefined) {
parentFolderId = nextFolderIdObject.id++
pathMap[parentFolder] = parentFolderId
folders.push({
addFolderFunction({
title: parentFolder,
folderId: parentFolderId,
parentFolderId: getParentFolderId(path, pathMap, folders, topLevelFolderId, nextFolderIdObject)
parentFolderId: getParentFolderId(path, pathMap, addFolderFunction, topLevelFolderId, nextFolderIdObject)
})
}
return parentFolderId
Expand All @@ -121,36 +122,47 @@ importer.on('add-bookmarks', (e, importedBookmarks, topLevelFolder) => {
let folders = []
let bookmarks = []
let topLevelFolderId = nextFolderIdObject.id++
const functionBuffer = new FunctionBuffer((args) => makeImmutable(args), this)
const bufferedAddFolder = (folder) => {
functionBuffer.buffer(appActions.addBookmarkFolder, folder)
folders.push(folder)
}

folders.push({
const importTopLevelFolder = {
title: bookmarkFoldersUtil.getNextFolderName(bookmarkFolders, topLevelFolder),
folderId: topLevelFolderId,
parentFolderId: 0
})
}
bufferedAddFolder(importTopLevelFolder)

for (let i = 0; i < importedBookmarks.length; ++i) {
let path = importedBookmarks[i].path
let parentFolderId = getParentFolderId(path, pathMap, folders, topLevelFolderId, nextFolderIdObject)
if (importedBookmarks[i].is_folder) {
const importedBookmark = importedBookmarks[i]
const path = importedBookmark.path
const title = importedBookmark.title
const parentFolderId = getParentFolderId(path, pathMap, bufferedAddFolder, topLevelFolderId, nextFolderIdObject)
if (importedBookmark.is_folder) {
const folderId = nextFolderIdObject.id++
pathMap[importedBookmarks[i].title] = folderId
folders.push({
title: importedBookmarks[i].title,
folderId: folderId,
parentFolderId: parentFolderId
})
pathMap[title] = folderId
const folder = {
title,
folderId,
parentFolderId
}
functionBuffer.buffer(appActions.addBookmarkFolder, folder)
folders.push(folder)
} else {
bookmarks.push({
title: importedBookmarks[i].title,
location: importedBookmarks[i].url,
parentFolderId: parentFolderId
})
const location = importedBookmark.url
const bookmark = {
title,
location,
parentFolderId
}
functionBuffer.buffer(appActions.addBookmark, bookmark)
bookmarks.push(bookmark)
}
}

bookmarkList = bookmarks
appActions.addBookmarkFolder(makeImmutable(folders))
appActions.addBookmark(makeImmutable(bookmarks))
functionBuffer.flush()
bookmarkList = makeImmutable(bookmarks)
})

importer.on('add-favicons', (e, detail) => {
Expand Down
42 changes: 42 additions & 0 deletions js/lib/functionBuffer.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

/**
* Perform a sequence of function calls, grouping together contiguous function
* calls for the same function different args.
* See syncUtil.applySyncRecords() for a usage example.
*/
module.exports = class FunctionBuffer {
/**
* @param {function=} prepareArguments Before calling a function, run this function to prepare the buffered arguments.
*/
constructor (prepareArguments) {
this.argumentsBuffer = []
this.previousFunction = () => {}
this.prepareArguments = prepareArguments || ((args) => args)
}

/**
* @param {function} fn Function to call. NOTE: this supports only calling with the first argument.
* @param {any} arg Arg to buffer
*/
buffer (fn, arg) {
if (fn !== this.previousFunction) {
this.flush()
this.previousFunction = fn
}
this.argumentsBuffer.push(arg)
}

/**
* Call previousFunction with buffered arguments and empty buffer.
*/
flush () {
if (!this.argumentsBuffer.length) { return }
const preparedArgs = this.prepareArguments(this.argumentsBuffer)
const returnValue = this.previousFunction(preparedArgs)
this.argumentsBuffer = []
return returnValue
}
}
31 changes: 9 additions & 22 deletions js/state/syncUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const crypto = require('crypto')
const writeActions = require('../constants/sync/proto').actions
const {getSetting} = require('../settings')
const {isDataUrl} = require('../lib/urlutil')
const FunctionBuffer = require('../lib/functionBuffer')
const bookmarkUtil = require('../../app/common/lib/bookmarkUtil')
const bookmarkFoldersState = require('../../app/common/state/bookmarkFoldersState')
const bookmarkFoldersUtil = require('../../app/common/lib/bookmarkFoldersUtil')
Expand Down Expand Up @@ -282,21 +283,7 @@ module.exports.applySyncRecords = (records) => {
* bookmark sites), then when the apply action changes (e.g. to folders) we
* first flush the buffer (e.g. apply the first sequence of sites).
*/
let buffer = []
let previousAppAction = () => {}
const flushBufferedAppActions = () => {
if (!buffer.length) { return }
previousAppAction(new Immutable.List(buffer))
buffer = []
}
const bufferAppAction = (appAction, item) => {
if (previousAppAction !== appAction) {
flushBufferedAppActions()
previousAppAction = appAction
}
buffer.push(item)
}

const functionBuffer = new FunctionBuffer((args) => new Immutable.List(args))
records.forEach((record) => {
if (!record) {
return true
Expand All @@ -305,32 +292,32 @@ module.exports.applySyncRecords = (records) => {
const siteData = module.exports.getSiteDataFromRecord(record, appState, records).set('skipSync', true)
if (shouldAddRecord(record)) {
if (record.bookmark.isFolder) {
bufferAppAction(appActions.addBookmarkFolder, siteData)
functionBuffer.buffer(appActions.addBookmarkFolder, siteData)
} else {
bufferAppAction(appActions.addBookmark, siteData)
functionBuffer.buffer(appActions.addBookmark, siteData)
}
} else if (shouldRemoveRecord(record)) {
if (record.bookmark.isFolder) {
const folderKey = bookmarkFoldersUtil.getKey(siteData)
bufferAppAction(appActions.removeBookmarkFolder, folderKey)
functionBuffer.buffer(appActions.removeBookmarkFolder, folderKey)
} else {
const siteKey = bookmarkUtil.getKey(siteData)
bufferAppAction(appActions.removeBookmark, siteKey)
functionBuffer.buffer(appActions.removeBookmark, siteKey)
}
}
} else if (record.objectData === 'historySite') {
const siteData = module.exports.getSiteDataFromRecord(record, appState, records)
if (shouldAddRecord(record)) {
bufferAppAction(appActions.addHistorySite, siteData)
functionBuffer.buffer(appActions.addHistorySite, siteData)
} else if (shouldRemoveRecord(record)) {
const siteKey = historyUtil.getKey(siteData)
bufferAppAction(appActions.removeHistorySite, siteKey)
functionBuffer.buffer(appActions.removeHistorySite, siteKey)
}
} else {
otherRecords.push(record)
}
})
flushBufferedAppActions()
functionBuffer.flush()
applyNonBatchedRecords(otherRecords)
}

Expand Down

0 comments on commit 0b6e7be

Please sign in to comment.