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

Commit

Permalink
Refactoring, Bug fixes, and tests for top sites on about:newtab
Browse files Browse the repository at this point in the history 
Bookmarking items does not affect position
Fixes #5413

Sites are ordered by most visited (count) DESC
Fixes #5322

Groundwork laid for #5565,
but the task is unfinished as-is.

-----
These commits moving existing logic from newtab.js into the session helper.
Tests were then added and I manually tested each scenario and tried to make
sure tests cover those. Things which are not covered are marked with  TODO(bsclifton)

The important thing:
appState.about.newtab.sites no longer persists a separate copy of the sites array.
It instead uses the location/partion of items saved to lookup the real object from appState.sites.
-----

Auditors: @cezaraugusto, @bbondy
  • Loading branch information
@bsclifton @bbondy
bsclifton authored and bbondy committed Nov 14, 2016
1 parent c9d80a9 commit ec3342c
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 267 deletions.
127 changes: 62 additions & 65 deletions app/common/state/aboutNewTabState.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,92 +5,89 @@
const Immutable = require('immutable')
const {makeImmutable} = require('./immutableUtil')
const siteUtil = require('../../../js/state/siteUtil')
const aboutNewTabMaxEntries = 100

const excludeSiteDetail = (siteDetail) => {
return !siteUtil.isBookmark(siteDetail) && !siteUtil.isHistoryEntry(siteDetail)
const compareSites = (site1, site2) => {
if (!site1 || !site2) return false
return site1.get('location') === site2.get('location') &&
site1.get('partitionNumber') === site2.get('partitionNumber')
}
const pinnedTopSites = (state) => {
return (state.getIn(['about', 'newtab', 'pinnedTopSites']) || Immutable.List()).setSize(18)
}
const ignoredTopSites = (state) => {
return state.getIn(['about', 'newtab', 'ignoredTopSites']) || Immutable.List()
}
const isPinned = (state, siteProps) => {
return pinnedTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0
}
const isIgnored = (state, siteProps) => {
return ignoredTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0
}
const sortCountDescending = (left, right) => {
if (left.get('count') < right.get('count')) return 1
if (left.get('count') > right.get('count')) return -1
return 0
}

const removeDuplicateSites = (sites) => {
// Filter out duplicate entries by location
return sites.filter((element, index, list) => {
if (!element) return false
return index === list.findIndex((site) => site && site.get('location') === element.get('location'))
/**
* topSites are defined by users. Pinned sites are attached to their positions
* in the grid, and the non pinned indexes are populated with newly accessed sites
*/
const getTopSites = (state) => {
// remove folders; sort by visit count; enforce a max limit
const sites = (state.get('sites') || new Immutable.List())
.filter((site) => !siteUtil.isFolder(site))
.sort(sortCountDescending)
.slice(-aboutNewTabMaxEntries)

// Filter out pinned and ignored sites
let unpinnedSites = sites.filter((site) => !(isPinned(state, site) || isIgnored(state, site)))

// TODO(bsclifton): de-dupe here
// ..

// Merge the pinned and unpinned lists together
// Pinned items have priority because the position is important
const gridSites = pinnedTopSites(state).map((pinnedSite) => {
// Fetch latest siteDetail objects from appState.sites using location/partition
if (pinnedSite) {
const matches = sites.filter((site) => compareSites(site, pinnedSite))
if (matches.size > 0) return matches.first()
}
// Default to unpinned items
const firstSite = unpinnedSites.first()
unpinnedSites = unpinnedSites.shift()
return firstSite
})

return gridSites.filter((site) => site != null)
}

const aboutNewTabState = {
mergeDetails: (state, props) => {
state = makeImmutable(state)
if (!props) {
return state
}

state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
getGridLayoutSize: (state) => {
return state.getIn(['about', 'newtab', 'gridLayoutSize'])
},

addSite: (state, props) => {
state = makeImmutable(state)
if (!props) {
return state
}

// Add timestamp if missing (ex: this is a visit, not a bookmark)
let siteDetail = makeImmutable(props.siteDetail)
siteDetail = siteDetail.set('lastAccessedTime', siteDetail.get('lastAccessedTime') || new Date().getTime())

// Only bookmarks and history items should be considered
if (excludeSiteDetail(siteDetail)) {
return state
}

// Keep track of the last 18 visited sites
let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List()
sites = sites.unshift(siteDetail)
sites = removeDuplicateSites(sites)
sites = sites.take(18)
// TODO(cezaraugusto): Sort should respect unshift and don't prioritize bookmarks
// |
// V
// .sort(suggestion.sortByAccessCountWithAgeDecay)
sites = siteUtil.addSite(sites, siteDetail, props.tag, props.originalSiteDetail)
state = state.setIn(['about', 'newtab', 'sites'], sites)
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
getSites: (state) => {
return state.getIn(['about', 'newtab', 'sites'])
},

removeSite: (state, props) => {
mergeDetails: (state, props) => {
state = makeImmutable(state)
if (!props) {
return state
}

// Only bookmarks and history items should be considered
let siteDetail = makeImmutable(props.siteDetail)
if (excludeSiteDetail(siteDetail)) {
return state
}

// Remove tags if this is a history item.
// NOTE: siteUtil.removeSite won't delete the entry unless tags are missing
if (siteDetail.get('tags') && siteDetail.get('tags').size === 0) {
siteDetail = siteDetail.delete('tags')
}

const sites = state.getIn(['about', 'newtab', 'sites'])
state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, siteDetail, undefined))
state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
},

updateSiteFavicon: (state, props) => {
setSites: (state) => {
state = makeImmutable(state)
props = makeImmutable(props)
if (!props || !props.get('frameProps') || !props.getIn(['frameProps', 'location'])) {
return state
}

const sites = state.getIn(['about', 'newtab', 'sites'])
const sitesWithFavicon = siteUtil.updateSiteFavicon(sites, props.getIn(['frameProps', 'location']), props.get('favicon'))
state = state.setIn(['about', 'newtab', 'sites'], sitesWithFavicon)
// return a filtered version of the sites array
state = state.setIn(['about', 'newtab', 'sites'], getTopSites(state))
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
}
}
Expand Down
80 changes: 21 additions & 59 deletions js/about/newtab.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,86 +51,49 @@ class NewTabPage extends React.Component {
})
})
}

get randomBackgroundImage () {
const image = Object.assign({}, backgrounds[Math.floor(Math.random() * backgrounds.length)])
image.style = {backgroundImage: 'url(' + image.source + ')'}
return image
}

get fallbackImage () {
const image = Object.assign({}, config.newtab.fallbackImage)
const pathToImage = path.join(__dirname, '..', '..', image.source)
image.style = {backgroundImage: 'url(' + `${pathToImage}` + ')'}
return image
}

get sites () {
return this.state.newTabData.getIn(['newTabDetail', 'sites'])
get topSites () {
return this.state.newTabData.getIn(['newTabDetail', 'sites']) || Immutable.List()
}

get pinnedTopSites () {
return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']).setSize(18)
return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']) || Immutable.List().setSize(18)
}

get ignoredTopSites () {
return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites'])
return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites']) || Immutable.List()
}

get gridLayoutSize () {
return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize'])
return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize']) || 'small'
}

isPinned (siteProps) {
return this.pinnedTopSites.includes(siteProps)
}

isIgnored (siteProps) {
return this.ignoredTopSites.includes(siteProps)
return this.pinnedTopSites.filter((site) => {
if (!site || !site.get) return false
return site.get('location') === siteProps.get('location') &&
site.get('partitionNumber') === siteProps.get('partitionNumber')
}).size > 0
}

isBookmarked (siteProps) {
return siteUtil.isSiteBookmarked(this.topSites, siteProps)
}

/**
* topSites are defined by users. Pinned sites are attached to their positions
* in the grid, and the non pinned indexes are populated with newly accessed sites
*/
get topSites () {
const pinnedTopSites = this.pinnedTopSites
const sites = this.sites
let unpinnedTopSites = sites.filter((site) => !this.isPinned(site) ? site : null)
let gridSites

const getUnpinned = () => {
const firstSite = unpinnedTopSites.first()
unpinnedTopSites = unpinnedTopSites.shift()
return firstSite
}

gridSites = pinnedTopSites.map(pinned => pinned || getUnpinned())

// Remove from grid all ignored sites
gridSites = gridSites.filter((site) => !this.isIgnored(site))

return gridSites.filter(site => site != null)
}

get gridLayout () {
const gridLayoutSize = this.gridLayoutSize
const gridSizes = {large: 18, medium: 12, small: 6}
const sitesRow = gridSizes[gridLayoutSize]

return this.topSites.take(sitesRow)
const sizeToCount = {large: 18, medium: 12, small: 6}
const count = sizeToCount[this.gridLayoutSize]
return this.topSites.take(count)
}

showSiteRemovalNotification () {
this.setState({
showSiteRemovalNotification: true
})
}

hideSiteRemovalNotification () {
this.setState({
showSiteRemovalNotification: false
Expand Down Expand Up @@ -160,8 +123,6 @@ class NewTabPage extends React.Component {

onDraggedSite (currentId, finalId) {
let gridSites = this.topSites
let pinnedTopSites = this.pinnedTopSites

const currentPosition = gridSites.filter((site) => site.get('location') === currentId).get(0)
const finalPosition = gridSites.filter((site) => site.get('location') === finalId).get(0)

Expand All @@ -173,6 +134,7 @@ class NewTabPage extends React.Component {
gridSites = gridSites.splice(finalPositionIndex, 0, currentPosition)

// If the reordered site is pinned, update pinned order as well
let pinnedTopSites = this.pinnedTopSites
pinnedTopSites = pinnedTopSites.splice(currentPositionIndex, 1)
pinnedTopSites = pinnedTopSites.splice(finalPositionIndex, 0, currentPosition)

Expand All @@ -199,20 +161,20 @@ class NewTabPage extends React.Component {
}

onPinnedTopSite (siteProps) {
const gridSites = this.topSites
const currentPosition = gridSites.filter((site) => siteProps.get('location') === site.get('location')).get(0)
const currentPositionIndex = gridSites.indexOf(currentPosition)
const currentPosition = this.topSites.filter((site) => siteProps.get('location') === site.get('location')).get(0)
const currentPositionIndex = this.topSites.indexOf(currentPosition)

// If pinned, leave it null. Otherwise stores site on ignoredTopSites list, retaining the same position
let pinnedTopSites = this.pinnedTopSites
pinnedTopSites = pinnedTopSites.splice(currentPositionIndex, 1, this.isPinned(siteProps) ? null : siteProps)
let pinnedTopSites = this.pinnedTopSites.splice(currentPositionIndex, 1, this.isPinned(siteProps) ? null : siteProps)

aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites})
}

onIgnoredTopSite (siteProps) {
this.showSiteRemovalNotification()

// TODO: this is not working :(

// If a pinnedTopSite is ignored, remove it from the pinned list as well
const newTabState = {}
if (this.isPinned(siteProps)) {
Expand Down Expand Up @@ -261,14 +223,14 @@ class NewTabPage extends React.Component {
return null
}

const gridLayout = this.gridLayout

const getLetterFromUrl = (url) => {
const hostname = urlutils.getHostname(url.get('location'), true)
const name = url.get('title') || hostname || '?'
return name.charAt(0).toUpperCase()
}

const gridLayout = this.gridLayout

return <div className='dynamicBackground' style={this.state.backgroundImage.style}>
{
this.state.backgroundImage
Expand Down
6 changes: 3 additions & 3 deletions js/stores/appStore.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -448,11 +448,11 @@ const handleAppAction = (action) => {
if (oldSiteSize !== appState.get('sites').size) {
filterOutNonRecents()
}
appState = aboutNewTabState.addSite(appState, action)
appState = aboutNewTabState.setSites(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)
appState = aboutNewTabState.setSites(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))
Expand Down Expand Up @@ -775,7 +775,7 @@ const handleAppAction = (action) => {
break
case WindowConstants.WINDOW_SET_FAVICON:
appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon))
appState = aboutNewTabState.updateSiteFavicon(appState, action)
appState = aboutNewTabState.setSites(appState, action)
break
case WindowConstants.WINDOW_SET_NAVIGATED:
if (!action.isNavigatedInPage) {
Expand Down
Loading

0 comments on commit ec3342c

Please sign in to comment.