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

Change sites from List to Map #5531

Merged
merged 5 commits into from
Nov 11, 2016
Merged

Change sites from List to Map #5531

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
20034b2
Change sites from List to Map
darkdh Nov 10, 2016
d8d2b66
Address @bsclifton's feedback
darkdh Nov 11, 2016
aa16552
Apply changes to aboutNewTabState
darkdh Nov 11, 2016
9143a70
- Added migration for newtab data (and tested manually several times)
bsclifton Nov 11, 2016
f42afbf
removed tags (not needed)
bsclifton Nov 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/importer.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with the above 😄 It's super important to have migrations for the session data

// 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
5 changes: 3 additions & 2 deletions docs/state.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ AppStore
}
}
},
sites: [{
sites: {
[siteKey]: { // Calculated by siteUtil.getSiteKey()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you just put the format of the key? like folder specifies folderId, URLs use location/partition/folderId?

location: string,
title: string,
customTitle: string, // User provided title for bookmark; overrides title
Expand All @@ -46,7 +47,7 @@ AppStore
partitionNumber: number, // Optionally specifies a specific session
folderId: number, // Set for bookmark folders only
parentFolderId: number // Set for bookmarks and bookmark folders only
}],
}},
downloads: [{
[downloadId]: {
startTime: number, // datetime.getTime()
Expand Down
132 changes: 76 additions & 56 deletions js/state/siteUtil.js
View file
Open in desktop
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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with this refactor, the tags field really isn't needed (which is nice). Because you can determine if it's a folder or not by the folderId. It might clean things up to remove it

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'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point (doesn't have to be now, something for all of us to keep in mind 😄), I think it might be worth creating util methods to help with type conversion / default values. Something like this:

const safeNumber = (value, default) => {
  const num = Number(value)
  return isNaN(num) ? default : num
}

}

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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to pull this re-ordering out to a common method?

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
Open in desktop
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))
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much cleaner + easier to read 😄

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