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

Improves perf of bookmark hanger #9710

Merged
merged 1 commit into from
Jul 11, 2017
Merged

Improves perf of bookmark hanger #9710

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions app/browser/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -677,6 +677,8 @@ const doAction = (state, action) => {
}
break
case appConstants.APP_ADD_SITE:
case appConstants.APP_ADD_BOOKMARK:
case appConstants.APP_EDIT_BOOKMARK:
{
if (action.tag === siteTags.BOOKMARK || action.tag === siteTags.BOOKMARK_FOLDER) {
createMenu(state)
Expand Down
64 changes: 43 additions & 21 deletions app/browser/reducers/sitesReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,35 +54,57 @@ const sitesReducer = (state, action, immutableAction) => {
break
}
case appConstants.APP_ADD_SITE:
const isSyncEnabled = syncEnabled()
if (Immutable.List.isList(action.siteDetail)) {
action.siteDetail.forEach((s) => {
state = siteUtil.addSite(state, s, action.tag, undefined, action.skipSync)
{
const isSyncEnabled = syncEnabled()
if (Immutable.List.isList(action.siteDetail)) {
action.siteDetail.forEach((s) => {
state = siteUtil.addSite(state, s, action.tag, action.skipSync)
if (isSyncEnabled) {
state = syncUtil.updateSiteCache(state, s)
}
})
} else {
let sites = state.get('sites')
if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {
action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
state = siteUtil.addSite(state, action.siteDetail, action.tag, action.skipSync)
if (isSyncEnabled) {
state = syncUtil.updateSiteCache(state, s)
state = syncUtil.updateSiteCache(state, action.siteDetail)
}
})
} else {
let sites = state.get('sites')
if (!action.siteDetail.get('folderId') && siteUtil.isFolder(action.siteDetail)) {
action.siteDetail = action.siteDetail.set('folderId', siteUtil.getNextFolderId(sites))
}
state = siteUtil.addSite(state, action.siteDetail, action.tag, action.originalSiteDetail, action.skipSync)
if (action.destinationDetail) {
const sourceKey = siteUtil.getSiteKey(action.siteDetail)
const destinationKey = siteUtil.getSiteKey(action.destinationDetail)
break
}
case appConstants.APP_ADD_BOOKMARK:
case appConstants.APP_EDIT_BOOKMARK:
{
const isSyncEnabled = syncEnabled()
Copy link
Member

Choose a reason for hiding this comment

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

nitpick for this section- it has enough logic and branches (if/else) that it would be nice to pull it out to a function so that it can be easily tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This section is fully covered with tests. I wouldn't move it into the function that is exportable. What we can do is move this block into the new function but keep it in the same file for internal use.

const sites = state.get('sites')
const closestKey = action.closestKey
let site = action.siteDetail

if (sourceKey != null) {
state = siteUtil.moveSite(state,
sourceKey, destinationKey, false, false, true)
}
if (site == null || action.tag == null) {
break
}

if (!site.get('folderId') && action.tag === siteTags.BOOKMARK_FOLDER) {
site = site.set('folderId', siteUtil.getNextFolderId(sites))
}

state = siteUtil.addSite(state, site, action.tag, action.editKey)

if (closestKey != null) {
const sourceKey = siteUtil.getSiteKey(site)
state = siteUtil.moveSite(state, sourceKey, closestKey, false, false, true)
}

if (isSyncEnabled) {
state = syncUtil.updateSiteCache(state, action.destinationDetail || action.siteDetail)
state = syncUtil.updateSiteCache(state, site)
}

state = updateActiveTabBookmarked(state)
break
}
state = updateActiveTabBookmarked(state)
break
case appConstants.APP_REMOVE_SITE:
const removeSiteSyncCallback = action.skipSync ? undefined : syncActions.removeSite
state = siteUtil.removeSite(state, action.siteDetail, action.tag, true, removeSiteSyncCallback)
Expand Down
2 changes: 2 additions & 0 deletions app/browser/reducers/urlBarSuggestionsReducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ const tabState = require('../../common/state/tabState')
const urlBarSuggestionsReducer = (state, action) => {
switch (action.actionType) {
case appConstants.APP_ADD_SITE:
case appConstants.APP_ADD_BOOKMARK:
case appConstants.APP_EDIT_BOOKMARK:
if (Immutable.List.isList(action.siteDetail)) {
action.siteDetail.forEach((s) => {
add(s)
Expand Down
24 changes: 13 additions & 11 deletions app/common/lib/bookmarkUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,20 @@ const {calculateTextWidth} = require('../../../js/lib/textCalculator')
const {iconSize} = require('../../../js/constants/config')
const {getSetting} = require('../../../js/settings')

const bookmarkHangerHeading = (detail, isFolder, shouldShowLocation) => {
function bookmarkHangerHeading (editMode, isFolder, isAdded) {
if (isFolder) {
return shouldShowLocation
return editMode
? 'bookmarkFolderEditing'
: 'bookmarkFolderAdding'
}
return shouldShowLocation
? (!detail || !detail.has('location'))
? 'bookmarkCreateNew'
: 'bookmarkEdit'
: 'bookmarkAdded'

if (isAdded) {
return 'bookmarkAdded'
}

return editMode
? 'bookmarkEdit'
: 'bookmarkCreateNew'
}

const displayBookmarkName = (detail) => {
Expand All @@ -37,11 +40,10 @@ const displayBookmarkName = (detail) => {
return detail.get('title') || ''
}

const isBookmarkNameValid = (detail, isFolder) => {
const title = detail.get('title') || detail.get('customTitle')
const location = detail.get('location')
const isBookmarkNameValid = (title, location, isFolder, customTitle) => {
const newTitle = title || customTitle
return isFolder
? (title != null && title !== 0) && title.trim().length > 0
? (newTitle != null && newTitle !== 0) && newTitle.trim().length > 0
: location != null && location.trim().length > 0
}

Expand Down
Loading