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

Commit 1bfa653

Browse files
bbondybbondy
authored
Merge pull request #5583 from bsclifton/topsites
Refactoring, Bug fixes, and tests for top sites on about:newtab
2 parents 849abde + c65f271 commit 1bfa653

File tree

5 files changed

+207
-270
lines changed

5 files changed

+207
-270
lines changed

‎app/common/state/aboutNewTabState.js

+66-65
Original file line numberDiff line numberDiff line change
@@ -5,92 +5,93 @@
55
const Immutable = require('immutable')
66
const {makeImmutable} = require('./immutableUtil')
77
const siteUtil = require('../../../js/state/siteUtil')
8+
const aboutNewTabMaxEntries = 100
89

9-
const excludeSiteDetail = (siteDetail) => {
10-
return !siteUtil.isBookmark(siteDetail) && !siteUtil.isHistoryEntry(siteDetail)
10+
const compareSites = (site1, site2) => {
11+
if (!site1 || !site2) return false
12+
return site1.get('location') === site2.get('location') &&
13+
site1.get('partitionNumber') === site2.get('partitionNumber')
1114
}
12-
13-
const removeDuplicateSites = (sites) => {
14-
// Filter out duplicate entries by location
15-
return sites.filter((element, index, list) => {
16-
if (!element) return false
17-
return index === list.findIndex((site) => site && site.get('location') === element.get('location'))
18-
})
15+
const pinnedTopSites = (state) => {
16+
return (state.getIn(['about', 'newtab', 'pinnedTopSites']) || Immutable.List()).setSize(18)
17+
}
18+
const ignoredTopSites = (state) => {
19+
return state.getIn(['about', 'newtab', 'ignoredTopSites']) || Immutable.List()
20+
}
21+
const isPinned = (state, siteProps) => {
22+
return pinnedTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0
23+
}
24+
const isIgnored = (state, siteProps) => {
25+
return ignoredTopSites(state).filter((site) => compareSites(site, siteProps)).size > 0
26+
}
27+
const sortCountDescending = (left, right) => {
28+
if (left.get('count') < right.get('count')) return 1
29+
if (left.get('count') > right.get('count')) return -1
30+
return 0
1931
}
2032

21-
const aboutNewTabState = {
22-
mergeDetails: (state, props) => {
23-
state = makeImmutable(state)
24-
if (!props) {
25-
return state
33+
/**
34+
* topSites are defined by users. Pinned sites are attached to their positions
35+
* in the grid, and the non pinned indexes are populated with newly accessed sites
36+
*/
37+
const getTopSites = (state) => {
38+
// remove folders; sort by visit count; enforce a max limit
39+
const sites = (state.get('sites') || new Immutable.List())
40+
.filter((site) => !siteUtil.isFolder(site))
41+
.sort(sortCountDescending)
42+
.slice(-aboutNewTabMaxEntries)
43+
44+
// Filter out pinned and ignored sites
45+
let unpinnedSites = sites.filter((site) => !(isPinned(state, site) || isIgnored(state, site)))
46+
47+
// TODO(bsclifton): de-dupe here
48+
// ..
49+
50+
// Merge the pinned and unpinned lists together
51+
// Pinned items have priority because the position is important
52+
let gridSites = pinnedTopSites(state).map((pinnedSite) => {
53+
// Fetch latest siteDetail objects from appState.sites using location/partition
54+
if (pinnedSite) {
55+
const matches = sites.filter((site) => compareSites(site, pinnedSite))
56+
if (matches.size > 0) return matches.first()
2657
}
58+
// Default to unpinned items
59+
const firstSite = unpinnedSites.first()
60+
unpinnedSites = unpinnedSites.shift()
61+
return firstSite
62+
})
2763

28-
state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
29-
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
30-
},
31-
32-
addSite: (state, props) => {
33-
state = makeImmutable(state)
34-
if (!props) {
35-
return state
36-
}
64+
// Include up to [aboutNewTabMaxEntries] entries so that folks
65+
// can ignore sites and have new items fill those empty spaces
66+
if (unpinnedSites.size > 0) {
67+
gridSites = gridSites.concat(unpinnedSites)
68+
}
3769

38-
// Add timestamp if missing (ex: this is a visit, not a bookmark)
39-
let siteDetail = makeImmutable(props.siteDetail)
40-
siteDetail = siteDetail.set('lastAccessedTime', siteDetail.get('lastAccessedTime') || new Date().getTime())
70+
return gridSites.filter((site) => site != null)
71+
}
4172

42-
// Only bookmarks and history items should be considered
43-
if (excludeSiteDetail(siteDetail)) {
44-
return state
45-
}
73+
const aboutNewTabState = {
74+
maxSites: aboutNewTabMaxEntries,
4675

47-
// Keep track of the last 18 visited sites
48-
let sites = state.getIn(['about', 'newtab', 'sites']) || new Immutable.List()
49-
sites = sites.unshift(siteDetail)
50-
sites = removeDuplicateSites(sites)
51-
sites = sites.take(18)
52-
// TODO(cezaraugusto): Sort should respect unshift and don't prioritize bookmarks
53-
// |
54-
// V
55-
// .sort(suggestion.sortByAccessCountWithAgeDecay)
56-
sites = siteUtil.addSite(sites, siteDetail, props.tag, props.originalSiteDetail)
57-
state = state.setIn(['about', 'newtab', 'sites'], sites)
58-
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
76+
getSites: (state) => {
77+
return state.getIn(['about', 'newtab', 'sites'])
5978
},
6079

61-
removeSite: (state, props) => {
80+
mergeDetails: (state, props) => {
6281
state = makeImmutable(state)
6382
if (!props) {
6483
return state
6584
}
6685

67-
// Only bookmarks and history items should be considered
68-
let siteDetail = makeImmutable(props.siteDetail)
69-
if (excludeSiteDetail(siteDetail)) {
70-
return state
71-
}
72-
73-
// Remove tags if this is a history item.
74-
// NOTE: siteUtil.removeSite won't delete the entry unless tags are missing
75-
if (siteDetail.get('tags') && siteDetail.get('tags').size === 0) {
76-
siteDetail = siteDetail.delete('tags')
77-
}
78-
79-
const sites = state.getIn(['about', 'newtab', 'sites'])
80-
state = state.setIn(['about', 'newtab', 'sites'], siteUtil.removeSite(sites, siteDetail, undefined))
86+
state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail)
8187
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
8288
},
8389

84-
updateSiteFavicon: (state, props) => {
90+
setSites: (state) => {
8591
state = makeImmutable(state)
86-
props = makeImmutable(props)
87-
if (!props || !props.get('frameProps') || !props.getIn(['frameProps', 'location'])) {
88-
return state
89-
}
9092

91-
const sites = state.getIn(['about', 'newtab', 'sites'])
92-
const sitesWithFavicon = siteUtil.updateSiteFavicon(sites, props.getIn(['frameProps', 'location']), props.get('favicon'))
93-
state = state.setIn(['about', 'newtab', 'sites'], sitesWithFavicon)
93+
// return a filtered version of the sites array
94+
state = state.setIn(['about', 'newtab', 'sites'], getTopSites(state))
9495
return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime())
9596
}
9697
}

‎js/about/aboutActions.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -166,10 +166,11 @@ const aboutActions = {
166166
ipc.send(messages.SET_CLIPBOARD, text)
167167
},
168168

169-
setNewTabDetail: function (newTabPageDetail) {
169+
setNewTabDetail: function (newTabPageDetail, refresh) {
170170
aboutActions.dispatchAction({
171171
actionType: appConstants.APP_CHANGE_NEW_TAB_DETAIL,
172-
newTabPageDetail
172+
newTabPageDetail,
173+
refresh
173174
})
174175
},
175176

‎js/about/newtab.js

+25-62
Original file line numberDiff line numberDiff line change
@@ -51,86 +51,49 @@ class NewTabPage extends React.Component {
5151
})
5252
})
5353
}
54-
5554
get randomBackgroundImage () {
5655
const image = Object.assign({}, backgrounds[Math.floor(Math.random() * backgrounds.length)])
5756
image.style = {backgroundImage: 'url(' + image.source + ')'}
5857
return image
5958
}
60-
6159
get fallbackImage () {
6260
const image = Object.assign({}, config.newtab.fallbackImage)
6361
const pathToImage = path.join(__dirname, '..', '..', image.source)
6462
image.style = {backgroundImage: 'url(' + `${pathToImage}` + ')'}
6563
return image
6664
}
67-
68-
get sites () {
69-
return this.state.newTabData.getIn(['newTabDetail', 'sites'])
65+
get topSites () {
66+
return this.state.newTabData.getIn(['newTabDetail', 'sites']) || Immutable.List()
7067
}
71-
7268
get pinnedTopSites () {
73-
return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']).setSize(18)
69+
return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites']) || Immutable.List().setSize(18)
7470
}
75-
7671
get ignoredTopSites () {
77-
return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites'])
72+
return this.state.newTabData.getIn(['newTabDetail', 'ignoredTopSites']) || Immutable.List()
7873
}
79-
8074
get gridLayoutSize () {
81-
return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize'])
75+
return this.state.newTabData.getIn(['newTabDetail', 'gridLayoutSize']) || 'small'
8276
}
83-
8477
isPinned (siteProps) {
85-
return this.pinnedTopSites.includes(siteProps)
86-
}
87-
88-
isIgnored (siteProps) {
89-
return this.ignoredTopSites.includes(siteProps)
78+
return this.pinnedTopSites.filter((site) => {
79+
if (!site || !site.get) return false
80+
return site.get('location') === siteProps.get('location') &&
81+
site.get('partitionNumber') === siteProps.get('partitionNumber')
82+
}).size > 0
9083
}
91-
9284
isBookmarked (siteProps) {
9385
return siteUtil.isSiteBookmarked(this.topSites, siteProps)
9486
}
95-
96-
/**
97-
* topSites are defined by users. Pinned sites are attached to their positions
98-
* in the grid, and the non pinned indexes are populated with newly accessed sites
99-
*/
100-
get topSites () {
101-
const pinnedTopSites = this.pinnedTopSites
102-
const sites = this.sites
103-
let unpinnedTopSites = sites.filter((site) => !this.isPinned(site) ? site : null)
104-
let gridSites
105-
106-
const getUnpinned = () => {
107-
const firstSite = unpinnedTopSites.first()
108-
unpinnedTopSites = unpinnedTopSites.shift()
109-
return firstSite
110-
}
111-
112-
gridSites = pinnedTopSites.map(pinned => pinned || getUnpinned())
113-
114-
// Remove from grid all ignored sites
115-
gridSites = gridSites.filter((site) => !this.isIgnored(site))
116-
117-
return gridSites.filter(site => site != null)
118-
}
119-
12087
get gridLayout () {
121-
const gridLayoutSize = this.gridLayoutSize
122-
const gridSizes = {large: 18, medium: 12, small: 6}
123-
const sitesRow = gridSizes[gridLayoutSize]
124-
125-
return this.topSites.take(sitesRow)
88+
const sizeToCount = {large: 18, medium: 12, small: 6}
89+
const count = sizeToCount[this.gridLayoutSize]
90+
return this.topSites.take(count)
12691
}
127-
12892
showSiteRemovalNotification () {
12993
this.setState({
13094
showSiteRemovalNotification: true
13195
})
13296
}
133-
13497
hideSiteRemovalNotification () {
13598
this.setState({
13699
showSiteRemovalNotification: false
@@ -160,8 +123,6 @@ class NewTabPage extends React.Component {
160123

161124
onDraggedSite (currentId, finalId) {
162125
let gridSites = this.topSites
163-
let pinnedTopSites = this.pinnedTopSites
164-
165126
const currentPosition = gridSites.filter((site) => site.get('location') === currentId).get(0)
166127
const finalPosition = gridSites.filter((site) => site.get('location') === finalId).get(0)
167128

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

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

@@ -199,13 +161,11 @@ class NewTabPage extends React.Component {
199161
}
200162

201163
onPinnedTopSite (siteProps) {
202-
const gridSites = this.topSites
203-
const currentPosition = gridSites.filter((site) => siteProps.get('location') === site.get('location')).get(0)
204-
const currentPositionIndex = gridSites.indexOf(currentPosition)
164+
const currentPosition = this.topSites.filter((site) => siteProps.get('location') === site.get('location')).get(0)
165+
const currentPositionIndex = this.topSites.indexOf(currentPosition)
205166

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

210170
aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites})
211171
}
@@ -221,24 +181,27 @@ class NewTabPage extends React.Component {
221181
const currentPositionIndex = gridSites.indexOf(currentPosition)
222182
const pinnedTopSites = this.pinnedTopSites.splice(currentPositionIndex, 1, null)
223183
newTabState.pinnedTopSites = pinnedTopSites
184+
185+
// TODO: ignoring an item sometimes was removing a pin for a different site
186+
// I think the merge is not working properly.
224187
}
225188

226189
newTabState.ignoredTopSites = this.ignoredTopSites.push(siteProps)
227-
aboutActions.setNewTabDetail(newTabState)
190+
aboutActions.setNewTabDetail(newTabState, true)
228191
}
229192

230193
onUndoIgnoredTopSite () {
231194
// Remove last List's entry
232195
const ignoredTopSites = this.ignoredTopSites.splice(-1, 1)
233-
aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites})
196+
aboutActions.setNewTabDetail({ignoredTopSites: ignoredTopSites}, true)
234197
this.hideSiteRemovalNotification()
235198
}
236199

237200
/**
238201
* Clear ignoredTopSites and pinnedTopSites list
239202
*/
240203
onRestoreAll () {
241-
aboutActions.setNewTabDetail({ignoredTopSites: [], pinnedTopSites: []})
204+
aboutActions.setNewTabDetail({ignoredTopSites: []}, true)
242205
this.hideSiteRemovalNotification()
243206
}
244207

@@ -261,14 +224,14 @@ class NewTabPage extends React.Component {
261224
return null
262225
}
263226

264-
const gridLayout = this.gridLayout
265-
266227
const getLetterFromUrl = (url) => {
267228
const hostname = urlutils.getHostname(url.get('location'), true)
268229
const name = url.get('title') || hostname || '?'
269230
return name.charAt(0).toUpperCase()
270231
}
271232

233+
const gridLayout = this.gridLayout
234+
272235
return <div className='dynamicBackground' style={this.state.backgroundImage.style}>
273236
{
274237
this.state.backgroundImage

‎js/stores/appStore.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -431,6 +431,9 @@ const handleAppAction = (action) => {
431431
break
432432
case AppConstants.APP_CHANGE_NEW_TAB_DETAIL:
433433
appState = aboutNewTabState.mergeDetails(appState, action)
434+
if (action.refresh) {
435+
appState = aboutNewTabState.setSites(appState, action)
436+
}
434437
break
435438
case AppConstants.APP_ADD_SITE:
436439
const oldSiteSize = appState.get('sites').size
@@ -448,11 +451,11 @@ const handleAppAction = (action) => {
448451
if (oldSiteSize !== appState.get('sites').size) {
449452
filterOutNonRecents()
450453
}
451-
appState = aboutNewTabState.addSite(appState, action)
454+
appState = aboutNewTabState.setSites(appState, action)
452455
break
453456
case AppConstants.APP_REMOVE_SITE:
454457
appState = appState.set('sites', siteUtil.removeSite(appState.get('sites'), action.siteDetail, action.tag))
455-
appState = aboutNewTabState.removeSite(appState, action)
458+
appState = aboutNewTabState.setSites(appState, action)
456459
break
457460
case AppConstants.APP_MOVE_SITE:
458461
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) => {
775778
break
776779
case WindowConstants.WINDOW_SET_FAVICON:
777780
appState = appState.set('sites', siteUtil.updateSiteFavicon(appState.get('sites'), action.frameProps.get('location'), action.favicon))
778-
appState = aboutNewTabState.updateSiteFavicon(appState, action)
781+
appState = aboutNewTabState.setSites(appState, action)
779782
break
780783
case WindowConstants.WINDOW_SET_NAVIGATED:
781784
if (!action.isNavigatedInPage) {

0 commit comments

Comments
 (0)