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

Commit

Permalink
Change sites from List to Map
Browse files Browse the repository at this point in the history 
fix #4879

Auditors: @bbondy, @bsclifton, @cezaraugusto

Test Plan:
1. Import bulk bookmarks (4000+) from other browsers
2. The process should finish instantly
  • Loading branch information
@darkdh
darkdh committed Nov 10, 2016
1 parent d0b88e1 commit b5349f6
Show file tree
Hide file tree
Showing 6 changed files with 338 additions and 409 deletions.
4 changes: 3 additions & 1 deletion app/importer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ const locale = require('./locale')
var isMergeFavorites = false
var isImportingBookmarks = false
var hasBookmarks
var importedSites

exports.init = () => {
importer.initialize()
Expand Down Expand Up @@ -171,6 +172,7 @@ importer.on('add-bookmarks', (e, bookmarks, topLevelFolder) => {
sites.push(site)
}
}
importedSites = Immutable.fromJS(sites)
appActions.addSite(Immutable.fromJS(sites))
})

Expand All @@ -187,7 +189,7 @@ importer.on('add-favicons', (e, detail) => {
}
}
})
let sites = AppStore.getState().get('sites')
let sites = importedSites
sites = sites.map((site) => {
if ((site.get('favicon') === undefined && site.get('location') !== undefined &&
faviconMap[site.get('location')] !== undefined) ||
Expand Down
22 changes: 16 additions & 6 deletions app/renderer/components/bookmarksToolbar.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -319,27 +319,37 @@ class BookmarksToolbar extends ImmutableComponent {

// Loop through until we fill up the entire bookmark toolbar width
let i
for (i = 0; i < noParentItems.size; i++) {
let noParentItemsList = noParentItems.toList()
for (i = 0; i < noParentItemsList.size; i++) {
let iconWidth = props.showFavicon ? iconSize : 0
// font-awesome file icons are 3px smaller
if (props.showFavicon && !noParentItems.getIn([i, 'folderId']) && !noParentItems.getIn([i, 'favicon'])) {
if (props.showFavicon && !noParentItemsList.getIn([i, 'folderId']) && !noParentItemsList.getIn([i, 'favicon'])) {
iconWidth -= 3
}
const chevronWidth = props.showFavicon && noParentItems.getIn([i, 'folderId']) ? this.chevronWidth : 0
const chevronWidth = props.showFavicon && noParentItemsList.getIn([i, 'folderId']) ? this.chevronWidth : 0
if (props.showFavicon && props.showOnlyFavicon) {
widthAccountedFor += this.padding + iconWidth + chevronWidth
} else {
const text = noParentItems.getIn([i, 'customTitle']) || noParentItems.getIn([i, 'title']) || noParentItems.getIn([i, 'location'])
const text = noParentItemsList.getIn([i, 'customTitle']) || noParentItemsList.getIn([i, 'title']) || noParentItemsList.getIn([i, 'location'])
widthAccountedFor += Math.min(calculateTextWidth(text, `${this.fontSize} ${this.fontFamily}`) + this.padding + iconWidth + chevronWidth, this.maxWidth)
}
widthAccountedFor += margin
if (widthAccountedFor >= props.windowWidth - overflowButtonWidth) {
break
}
}
this.bookmarksForToolbar = noParentItems.take(i)
const bookmarkSort = (x, y) => {
if (x.get('order') < y.get('order')) {
return -1
} else if (x.get('order') > y.get('order')) {
return 1
} else {
return 0
}
}
this.bookmarksForToolbar = noParentItems.take(i).sort(bookmarkSort)
// Show at most 100 items in the overflow menu
this.overflowBookmarkItems = noParentItems.skip(i).take(100)
this.overflowBookmarkItems = noParentItems.skip(i).take(100).sort(bookmarkSort)
}
componentWillMount () {
this.updateBookmarkData(this.props)
Expand Down
12 changes: 11 additions & 1 deletion app/sessionStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,16 @@ module.exports.loadAppState = () => {
}
}

// sites refactoring migration
if (Array.isArray(data.sites) && data.sites.length) {
let sites = {}
data.sites.forEach((site) => {
let key = siteUtil.getSiteKey(Immutable.fromJS(site), site.tags)
sites[key] = site
})
data.sites = sites
}

// version information (shown on about:brave)
const os = require('os')
const versionInformation = [
Expand Down Expand Up @@ -466,7 +476,7 @@ module.exports.loadAppState = () => {
module.exports.defaultAppState = () => {
return {
firstRunTimestamp: new Date().getTime(),
sites: [],
sites: {},
tabs: [],
extensions: {},
visits: [],
Expand Down
132 changes: 76 additions & 56 deletions js/state/siteUtil.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,22 +25,20 @@ const isBookmarkFolder = (tags) => {
tags && typeof tags !== 'string' && tags.includes(siteTags.BOOKMARK_FOLDER)
}

/**
* Obtains the index of the location in sites
*
* @param sites The application state's Immutable sites list
* @param siteDetail The siteDetails entry to get the index of
* @param tags Tag for siteDetail (ex: bookmark). Folders are searched differently than other entries
* @return index of the siteDetail or -1 if not found.
*/
module.exports.getSiteIndex = function (sites, siteDetail, tags) {
if (!sites || !siteDetail) {
return -1
module.exports.getSiteKey = function (siteDetail, tags) {
if (!siteDetail) {
return null
}
if (isBookmarkFolder(tags)) {
return sites.findIndex((site) => isBookmarkFolder(site.get('tags')) && site.get('folderId') === siteDetail.get('folderId'))
const folderId = siteDetail.get('folderId')
const location = siteDetail.get('location')
if (isBookmarkFolder(tags) && folderId) {
return folderId.toString()
} else if (location) {
return location +
(siteDetail.get('partitionNumber') || 0) +
(siteDetail.get('parentFolderId') || 0)
}
return sites.findIndex((site) => site.get('location') === siteDetail.get('location') && (site.get('partitionNumber') || 0) === (siteDetail.get('partitionNumber') || 0))
return null
}

/**
Expand All @@ -51,11 +49,14 @@ module.exports.getSiteIndex = function (sites, siteDetail, tags) {
* @return true if the location is already bookmarked
*/
module.exports.isSiteBookmarked = function (sites, siteDetail) {
const index = module.exports.getSiteIndex(sites, siteDetail, siteTags.BOOKMARK)
if (index === -1) {
if (!sites) {
return false
}
const key = module.exports.getSiteKey(siteDetail, siteTags.BOOKMARK)
if (key === null) {
return false
}
return isBookmark(sites.get(index).get('tags'))
return isBookmark(sites.getIn([key, 'tags']))
}

const getNextFolderIdItem = (sites) =>
Expand Down Expand Up @@ -85,7 +86,7 @@ module.exports.getNextFolderId = (sites) => {

// Some details can be copied from the existing siteDetail if null
// ex: parentFolderId, partitionNumber, and favicon
const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => {
const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId, order) => {
let tags = oldSiteDetail && oldSiteDetail.get('tags') || new Immutable.List()
if (tag) {
tags = tags.toSet().add(tag).toList()
Expand All @@ -105,9 +106,14 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => {
let site = Immutable.fromJS({
lastAccessedTime: lastAccessedTime,
tags,
title: newSiteDetail.get('title')
title: newSiteDetail.get('title'),
order
})

if (oldSiteDetail && oldSiteDetail.get('order') !== undefined) {
site = site.set('order', oldSiteDetail.get('order'))
}

if (newSiteDetail.get('location')) {
site = site.set('location', newSiteDetail.get('location'))
}
Expand Down Expand Up @@ -162,8 +168,13 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
tag = siteDetail.getIn(['tags', 0])
}

const index = module.exports.getSiteIndex(sites, originalSiteDetail || siteDetail, tag)
const oldSite = index !== -1 ? sites.getIn([index]) : null
let originalSiteKey
if (originalSiteDetail) {
originalSiteKey = module.exports.getSiteKey(originalSiteDetail, originalSiteDetail.get('tags'))
}

const oldKey = originalSiteKey || module.exports.getSiteKey(siteDetail, tag)
const oldSite = oldKey !== null ? sites.get(oldKey) : null
let folderId = siteDetail.get('folderId')

if (tag === siteTags.BOOKMARK_FOLDER) {
Expand All @@ -173,21 +184,21 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
site.get('parentFolderId') === siteDetail.get('parentFolderId') &&
site.get('customTitle') === siteDetail.get('customTitle'))
if (dupFolder) {
sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER)
sites = module.exports.removeSite(sites, dupFolder, siteTags.BOOKMARK_FOLDER, true)
}
} else if (!folderId) {
// Assign an id if this is a new folder
folderId = module.exports.getNextFolderId(sites)
}
}

let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId)
if (index === -1) {
// Insert new entry
return sites.push(site)
let site = mergeSiteDetails(oldSite, siteDetail, tag, folderId, sites.size)

const key = originalSiteKey || module.exports.getSiteKey(site, tag)
if (key === null) {
return sites
}
// Update existing entry
return sites.setIn([index], site)
return sites.set(key, site)
}

/**
Expand All @@ -197,36 +208,32 @@ module.exports.addSite = function (sites, siteDetail, tag, originalSiteDetail) {
* @param siteDetail The siteDetail to remove a tag from
* @return The new sites Immutable object
*/
module.exports.removeSite = function (sites, siteDetail, tag) {
const index = module.exports.getSiteIndex(sites, siteDetail, tag)
if (index === -1) {
return sites
}
module.exports.removeSite = function (sites, siteDetail, tag, reorder) {
const key = module.exports.getSiteKey(siteDetail, tag)

const tags = sites.getIn([index, 'tags'])
const tags = sites.getIn([key, 'tags'])
if (isBookmarkFolder(tags)) {
const folderId = sites.getIn([index, 'folderId'])
const folderId = sites.getIn([key, 'folderId'])
const childSites = sites.filter((site) => site.get('parentFolderId') === folderId)
childSites.forEach((site) => {
const tags = site.get('tags')
tags.forEach((tag) => {
sites = module.exports.removeSite(sites, site, tag)
sites = module.exports.removeSite(sites, site, tag, false)
})
})
}
if (tags.size === 0 && !tag) {
// If called without tags and entry has no tags, remove the entry
return sites.splice(index, 1)
} else if (tags.size > 0 && !tag) {
// If called without tags BUT entry has tags, null out lastAccessedTime.
// This is a bookmark entry that we want to clear history for (but NOT delete/untag bookmark)
return sites.setIn([index, 'lastAccessedTime'], null)
}
// Else, remove the specified tag
return sites
.setIn([index, 'parentFolderId'], 0)
.deleteIn([index, 'customTitle'])
.setIn([index, 'tags'], tags.toSet().remove(tag).toList())
if (reorder) {
const order = sites.getIn([key, 'order'])
sites = sites.map((site) => {
const siteOrder = site.get('order')
if (siteOrder > order) {
return site.set('order', siteOrder - 1)
}
return site
})
}

return sites.delete(key)
}

/**
Expand Down Expand Up @@ -277,28 +284,40 @@ module.exports.isMoveAllowed = (sites, sourceDetail, destinationDetail) => {
* @param disallowReparent If set to true, parent folder will not be set
* @return The new sites Immutable object
*/
module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend, destinationIsParent, disallowReparent) {
module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prepend,
destinationIsParent, disallowReparent) {
if (!module.exports.isMoveAllowed(sites, sourceDetail, destinationDetail)) {
return sites
}

const sourceSiteIndex = module.exports.getSiteIndex(sites, sourceDetail, sourceDetail.get('tags'))
let sourceKey = module.exports.getSiteKey(sourceDetail, sourceDetail.get('tags'))
let destinationKey = module.exports.getSiteKey(destinationDetail, destinationDetail.get('tags'))

const sourceSiteIndex = sites.getIn([sourceKey, 'order'])
let destinationSiteIndex
if (destinationIsParent) {
// When the destination is the parent we want to put it at the end
destinationSiteIndex = sites.size - 1
prepend = false
} else {
destinationSiteIndex = module.exports.getSiteIndex(sites, destinationDetail, destinationDetail.get('tags'))
destinationSiteIndex = sites.getIn([destinationKey, 'order'])
}

let newIndex = destinationSiteIndex + (prepend ? 0 : 1)
let sourceSite = sites.get(sourceSiteIndex)
let destinationSite = sites.get(destinationSiteIndex)
sites = sites.splice(sourceSiteIndex, 1)
let sourceSite = sites.get(sourceKey)
let destinationSite = sites.get(destinationKey)
sites = sites.delete(sourceKey)
sites = sites.map((site) => {
const siteOrder = site.get('order')
if (siteOrder > sourceSiteIndex) {
return site.set('order', siteOrder - 1)
}
return site
})
if (newIndex > sourceSiteIndex) {
newIndex--
}
sourceSite = sourceSite.set('order', newIndex)

if (!disallowReparent) {
if (destinationIsParent && destinationDetail.get('folderId') !== sourceSite.get('folderId')) {
Expand All @@ -309,7 +328,8 @@ module.exports.moveSite = function (sites, sourceDetail, destinationDetail, prep
sourceSite = sourceSite.set('parentFolderId', destinationSite.get('parentFolderId'))
}
}
return sites.splice(newIndex, 0, sourceSite)
sourceKey = module.exports.getSiteKey(sourceSite, sourceSite.get('tags'))
return sites.set(sourceKey, sourceSite)
}

module.exports.getDetailFromFrame = function (frame, tag) {
Expand Down
25 changes: 18 additions & 7 deletions js/stores/appStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -439,10 +439,15 @@ const handleAppAction = (action) => {
appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), s, action.tag))
})
} else {
appState = appState.set('sites', siteUtil.addSite(appState.get('sites'), action.siteDetail, action.tag, action.originalSiteDetail))
let sites = appState.get('sites')
if (!action.siteDetail.get('folderId')) {
action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
appState = appState.set('sites', siteUtil.addSite(sites, action.siteDetail, action.tag))
}
if (action.destinationDetail) {
appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.siteDetail, action.destinationDetail, false, false, true))
appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
action.siteDetail, action.destinationDetail, false, false, true))
}
// If there was an item added then clear out the old history entries
if (oldSiteSize !== appState.get('sites').size) {
Expand All @@ -451,12 +456,18 @@ const handleAppAction = (action) => {
appState = aboutNewTabState.addSite(appState, action)
break
case AppConstants.APP_REMOVE_SITE:
appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag))
appState = aboutNewTabState.removeSite(appState, action)
break
{
appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag, true))
appState = aboutNewTabState.removeSite(appState, action)
break
}
case AppConstants.APP_MOVE_SITE:
appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'), action.sourceDetail, action.destinationDetail, action.prepend, action.destinationIsParent, false))
break
{
appState = appState.set('sites', siteUtil.moveSite(appState.get('sites'),
action.sourceDetail, action.destinationDetail, action.prepend,
action.destinationIsParent, false))
break
}
case AppConstants.APP_MERGE_DOWNLOAD_DETAIL:
if (action.downloadDetail) {
appState = appState.mergeIn(['downloads', action.downloadId], action.downloadDetail)
Expand Down
Loading

0 comments on commit b5349f6

Please sign in to comment.