Refactoring, Bug fixes, and tests for top sites on about:newtab

app/common/state/aboutNewTabState.js

@@ -5,92 +5,93 @@
 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 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'))
-  })
+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 aboutNewTabState = {
-  mergeDetails: (state, props) => {
-    state = makeImmutable(state)
-    if (!props) {
-      return state
+/**
+ * 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
+  let 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
+  })
 
-    state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
-    return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
-  },
-
-  addSite: (state, props) => {
-    state = makeImmutable(state)
-    if (!props) {
-      return state
-    }
+  // Include up to [aboutNewTabMaxEntries] entries so that folks
+  // can ignore sites and have new items fill those empty spaces
+  if (unpinnedSites.size > 0) {
+    gridSites = gridSites.concat(unpinnedSites)
+  }
 
-    // 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())
+  return gridSites.filter((site) => site != null)
+}
 
-    // Only bookmarks and history items should be considered
-    if (excludeSiteDetail(siteDetail)) {
-      return state
-    }
+const aboutNewTabState = {
+  maxSites: aboutNewTabMaxEntries,
 
-    // 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())
   }
 }

js/about/aboutActions.js

@@ -166,10 +166,11 @@ const aboutActions = {
     ipc.send(messages.SET_CLIPBOARD, text)
   },
 
-  setNewTabDetail: function (newTabPageDetail) {
+  setNewTabDetail: function (newTabPageDetail, refresh) {
     aboutActions.dispatchAction({
       actionType: appConstants.APP_CHANGE_NEW_TAB_DETAIL,
-      newTabPageDetail
+      newTabPageDetail,
+      refresh
     })
   },
 

js/about/newtab.js

@@ -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
@@ -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)
 
@@ -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)
 
@@ -199,13 +161,11 @@ 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})
   }
@@ -221,24 +181,27 @@ class NewTabPage extends React.Component {
       const currentPositionIndex = gridSites.indexOf(currentPosition)
       const pinnedTopSites = this.pinnedTopSites.splice(currentPositionIndex, 1, null)
       newTabState.pinnedTopSites = pinnedTopSites
+
+      // TODO: ignoring an item sometimes was removing a pin for a different site
+      // I think the merge is not working properly.
     }
 
     newTabState.ignoredTopSites = this.ignoredTopSites.push(siteProps)
-    aboutActions.setNewTabDetail(newTabState)
+    aboutActions.setNewTabDetail(newTabState, true)
   }
 
   onUndoIgnoredTopSite () {
     // Remove last List's entry
     const ignoredTopSites = this.ignoredTopSites.splice(-1, 1)
-    aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites})
+    aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites}, true)
     this.hideSiteRemovalNotification()
   }
 
   /**
    * Clear ignoredTopSites and pinnedTopSites list
    */
   onRestoreAll () {
-    aboutActions.setNewTabDetail({ignoredTopSites: [], pinnedTopSites: []})
+    aboutActions.setNewTabDetail({ignoredTopSites: []}, true)
     this.hideSiteRemovalNotification()
   }
 
@@ -261,14 +224,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

js/stores/appStore.js

@@ -431,6 +431,9 @@ const handleAppAction = (action) => {
       break
     case AppConstants.APP_CHANGE_NEW_TAB_DETAIL:
       appState = aboutNewTabState.mergeDetails(appState, action)
+      if (action.refresh) {
+        appState = aboutNewTabState.setSites(appState, action)
+      }
       break
     case AppConstants.APP_ADD_SITE:
       const oldSiteSize = appState.get('sites').size
@@ -448,11 +451,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))
@@ -775,7 +778,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) {