From fdd7e38682d4efd651da10eaca47708c7dcfdba3 Mon Sep 17 00:00:00 2001 From: Cezar Augusto Date: Tue, 2 Jan 2018 22:48:14 -0200 Subject: [PATCH 1/5] pinned sites should retain position --- app/browser/api/topSites.js | 29 +++++++++++++++++++++++++--- app/common/state/aboutNewTabState.js | 4 +++- js/about/newtab.js | 28 +++++++++++++-------------- 3 files changed, 42 insertions(+), 19 deletions(-) diff --git a/app/browser/api/topSites.js b/app/browser/api/topSites.js index f960ac1e4f8..ff0e28841c1 100644 --- a/app/browser/api/topSites.js +++ b/app/browser/api/topSites.js @@ -20,7 +20,12 @@ let minAccessOfTopSites const staticData = Immutable.fromJS(newTabData.topSites) const isPinned = (state, siteKey) => { - return aboutNewTabState.getPinnedTopSites(state).find(site => site.get('key') === siteKey) + return aboutNewTabState.getPinnedTopSites(state).some(site => { + if (!site || !site.get) { + return false + } + return site.get('key') === siteKey + }) } const isIgnored = (state, siteKey) => { @@ -117,7 +122,7 @@ const getTopSiteData = () => { if (sites.size < 18) { const preDefined = staticData .filter((site) => { - return !isPinned(state, site.get('key')) && !isIgnored(state, site.get('key')) + return !isIgnored(state, site.get('key')) }) .map(site => { const bookmarkKey = bookmarkLocationCache.getCacheKey(state, site.get('location')) @@ -126,7 +131,25 @@ const getTopSiteData = () => { sites = sites.concat(preDefined) } - appActions.topSiteDataAvailable(sites) + sites = removeDuplicateDomains(sites) + + // TODO: newer sites should skip pinned position + let gridSites = aboutNewTabState.getPinnedTopSites(state) + + sites.forEach((site) => { + const siteExists = gridSites.some(pinnedSite => { + if (!pinnedSite) { + return false + } + return site.get('key') === pinnedSite.get('key') + }) + + if (!siteExists) { + gridSites.unshift(site) + } + }) + + appActions.topSiteDataAvailable(gridSites) } /** diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index ae16b8044c3..8eb4884b59b 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -16,7 +16,9 @@ const aboutNewTabState = { }, getPinnedTopSites: (state) => { - return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()) + // we need null spaces in order to proper pin a topSite in the right position. + // historically defined with 3 rows of 6 and kept as-is for parity with other areas. + return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()).setSize(18) }, getIgnoredTopSites: (state) => { diff --git a/js/about/newtab.js b/js/about/newtab.js index e63d0d97cfa..112955909de 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -109,20 +109,19 @@ class NewTabPage extends React.Component { } isPinned (siteKey) { - return this.pinnedTopSites.find(site => site.get('key') === siteKey) + return this.pinnedTopSites.some(site => { + if (!site || !site.get) { + return false + } + return site.get('key') === siteKey + }) } get gridLayout () { const sizeToCount = {large: 18, medium: 12, small: 6} const count = sizeToCount[this.gridLayoutSize] - let sites = this.pinnedTopSites.take(count) - - if (sites.size < count) { - sites = sites.concat(this.topSites.take(count - sites.size)) - } - - return sites + return this.topSites.take(count) } showNotification () { @@ -178,14 +177,13 @@ class NewTabPage extends React.Component { } onPinnedTopSite (siteKey) { - let pinnedTopSites = this.pinnedTopSites + const currentSiteIndex = this.topSites.findIndex(site => site.get('key') === siteKey) const siteProps = this.topSites.find(site => site.get('key') === siteKey) - - if (this.isPinned(siteKey)) { - pinnedTopSites = pinnedTopSites.filter(site => site.get('key') !== siteKey) - } else { - pinnedTopSites = pinnedTopSites.push(siteProps) - } + // ensure pinned sites are pinned in the right order + // do not edit these lines unless you've manually tested site visits + // after pinning and after dnd reordering + let pinnedTopSites = this.pinnedTopSites + .splice(currentSiteIndex, 1, this.isPinned(siteKey) ? null : siteProps) aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites}, true) } From ae0a1a4cb680b2df16a0aa38c2557ef2f8168e19 Mon Sep 17 00:00:00 2001 From: Cezar Augusto Date: Wed, 3 Jan 2018 02:22:53 -0200 Subject: [PATCH 2/5] keep pinned topsite position after new site visit --- app/browser/api/topSites.js | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/app/browser/api/topSites.js b/app/browser/api/topSites.js index ff0e28841c1..e5037add77f 100644 --- a/app/browser/api/topSites.js +++ b/app/browser/api/topSites.js @@ -121,6 +121,7 @@ const getTopSiteData = () => { if (sites.size < 18) { const preDefined = staticData + // TODO: this doesn't work properly .filter((site) => { return !isIgnored(state, site.get('key')) }) @@ -131,24 +132,24 @@ const getTopSiteData = () => { sites = sites.concat(preDefined) } - sites = removeDuplicateDomains(sites) - - // TODO: newer sites should skip pinned position - let gridSites = aboutNewTabState.getPinnedTopSites(state) - - sites.forEach((site) => { - const siteExists = gridSites.some(pinnedSite => { - if (!pinnedSite) { - return false - } - return site.get('key') === pinnedSite.get('key') - }) - - if (!siteExists) { - gridSites.unshift(site) + const pinnedTopSites = aboutNewTabState.getPinnedTopSites(state) + let gridSites = pinnedTopSites.map(pinned => { + // topsites are populated once user visit a new site. + // pinning a site to a given index is a user decision + // and should be taken as priority. If there's an empty + // space we just fill it with visited sites. Otherwise + // fallback to the pinned item. + if (!pinned) { + const firstSite = sites.first() + sites = sites.shift() + return firstSite } + return pinned }) + gridSites = gridSites.filter(site => site != null) + gridSites = removeDuplicateDomains(gridSites) + appActions.topSiteDataAvailable(gridSites) } From 06e44b3f3683cc1fd0db6c27e36eea9d8587bb89 Mon Sep 17 00:00:00 2001 From: Cezar Augusto Date: Wed, 3 Jan 2018 18:59:51 -0200 Subject: [PATCH 3/5] fix broken topsites tests --- app/common/state/aboutNewTabState.js | 8 +++-- test/unit/app/browser/api/topSitesTest.js | 38 +++++++++-------------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 8eb4884b59b..2ede58f28b5 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -4,7 +4,7 @@ const Immutable = require('immutable') const {makeImmutable} = require('./immutableUtil') - +const topSites = require('../../browser/api/topSites') /** * 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 @@ -16,9 +16,11 @@ const aboutNewTabState = { }, getPinnedTopSites: (state) => { + // add same number as fallback to avoid race condition on startup + const maxEntries = topSites.aboutNewTabMaxEntries || 100 // we need null spaces in order to proper pin a topSite in the right position. - // historically defined with 3 rows of 6 and kept as-is for parity with other areas. - return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()).setSize(18) + // so let's set it to the same number as max new tab entries. + return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()).setSize(maxEntries) }, getIgnoredTopSites: (state) => { diff --git a/test/unit/app/browser/api/topSitesTest.js b/test/unit/app/browser/api/topSitesTest.js index 9faaf1d9fa8..9babd794e4c 100644 --- a/test/unit/app/browser/api/topSitesTest.js +++ b/test/unit/app/browser/api/topSitesTest.js @@ -154,38 +154,28 @@ describe('topSites api', function () { ]) it('respects position of pinned items when populating results', function () { - const allPinned = Immutable.fromJS([site1, site4]) - const stateWithPinnedSites = defaultAppState + const allPinned = Immutable.fromJS([null, site2, null, site4]) + let stateWithPinnedSites = defaultAppState .set(STATE_SITES.HISTORY_SITES, generateMap(site1, site2, site3, site4)) .setIn(['about', 'newtab', 'pinnedTopSites'], allPinned) this.topSites.calculateTopSites(stateWithPinnedSites) // checks: - // - pinned item are in their expected order + // - pinned item are in their expected order (site 2 at i-1 and site4 at i-3) // - unpinned items fill the rest of the spots (starting w/ highest # visits first) + this.topSites.calculateTopSites(stateWithPinnedSites) getStateValue = stateWithPinnedSites this.clock.tick(calculateTopSitesClockTime) assert.equal(this.appActions.topSiteDataAvailable.callCount, 1) const newSitesData = this.appActions.topSiteDataAvailable.getCall(0).args[0] - const expectedSites = Immutable.fromJS([ - { - location: 'https://example3.com/', - title: 'sample 3', - parentFolderId: 0, - count: 23, - lastAccessedTime: 123, - bookmarked: false, - key: 'https://example3.com/|0' - }, - { - location: 'https://example2.com/', - title: 'sample 2', - parentFolderId: 0, - count: 5, - bookmarked: false, - key: 'https://example2.com/|0' - } - ]) - assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS()) + + // assert that first site is populated + assert.deepEqual(newSitesData.get(0).isEmpty(), false) + // assert that site 2 is at i-1 as planned + assert.deepEqual(newSitesData.get(1), site2) + // assert that second site is populated + assert.deepEqual(newSitesData.get(2).isEmpty(), false) + // assert that site 4 is at i-3 as planned + assert.deepEqual(newSitesData.get(3), site4) }) it('only includes one result for a domain (the one with the highest count)', function () { @@ -294,7 +284,7 @@ describe('topSites api', function () { assert.deepEqual(newSitesData.toJS(), expectedSites.concat(staticNewData).toJS()) }) - it('only returns the last `maxSites` results', function () { + it('only returns the last maxSites results', function () { const maxSites = this.topSites.aboutNewTabMaxEntries let tooManySites = Immutable.Map() for (let i = 0; i < maxSites + 1; i++) { From 5531c0f6734527b2b7b10c4796a27c7ffc3ef4c8 Mon Sep 17 00:00:00 2001 From: Cezar Augusto Date: Thu, 4 Jan 2018 16:06:35 -0200 Subject: [PATCH 4/5] fix unwanted persisted pinned top site - default pinned top site was persistent even after restart. changes address it --- app/browser/api/topSites.js | 10 ++++++---- app/common/state/aboutNewTabState.js | 14 +++++++++++--- app/sessionStore.js | 3 +-- js/about/newtab.js | 3 +-- 4 files changed, 19 insertions(+), 11 deletions(-) diff --git a/app/browser/api/topSites.js b/app/browser/api/topSites.js index e5037add77f..dfe0c905ece 100644 --- a/app/browser/api/topSites.js +++ b/app/browser/api/topSites.js @@ -123,7 +123,7 @@ const getTopSiteData = () => { const preDefined = staticData // TODO: this doesn't work properly .filter((site) => { - return !isIgnored(state, site.get('key')) + return !isPinned(state, site.get('key')) && !isIgnored(state, site.get('key')) }) .map(site => { const bookmarkKey = bookmarkLocationCache.getCacheKey(state, site.get('location')) @@ -132,8 +132,11 @@ const getTopSiteData = () => { sites = sites.concat(preDefined) } - const pinnedTopSites = aboutNewTabState.getPinnedTopSites(state) - let gridSites = pinnedTopSites.map(pinned => { + let gridSites = aboutNewTabState.getPinnedTopSites(state).map(pinned => { + // do not allow duplicates + if (pinned) { + sites = sites.filter(site => site.get('key') !== pinned.get('key')) + } // topsites are populated once user visit a new site. // pinning a site to a given index is a user decision // and should be taken as priority. If there's an empty @@ -148,7 +151,6 @@ const getTopSiteData = () => { }) gridSites = gridSites.filter(site => site != null) - gridSites = removeDuplicateDomains(gridSites) appActions.topSiteDataAvailable(gridSites) } diff --git a/app/common/state/aboutNewTabState.js b/app/common/state/aboutNewTabState.js index 2ede58f28b5..e322a3028d9 100644 --- a/app/common/state/aboutNewTabState.js +++ b/app/common/state/aboutNewTabState.js @@ -5,11 +5,12 @@ const Immutable = require('immutable') const {makeImmutable} = require('./immutableUtil') const topSites = require('../../browser/api/topSites') +const newTabData = require('../../../js/data/newTabData') /** * 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 defaultPinnedSite = Immutable.fromJS(newTabData.pinnedTopSites) const aboutNewTabState = { getSites: (state) => { return state.getIn(['about', 'newtab', 'sites']) @@ -18,9 +19,12 @@ const aboutNewTabState = { getPinnedTopSites: (state) => { // add same number as fallback to avoid race condition on startup const maxEntries = topSites.aboutNewTabMaxEntries || 100 + // we need null spaces in order to proper pin a topSite in the right position. // so let's set it to the same number as max new tab entries. - return state.getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()).setSize(maxEntries) + return state + .getIn(['about', 'newtab', 'pinnedTopSites'], Immutable.List()) + .setSize(maxEntries) }, getIgnoredTopSites: (state) => { @@ -32,7 +36,11 @@ const aboutNewTabState = { if (!props) { return state } - + // list is only empty if there's no pinning interaction. + // in this case we include the default pinned top sites list + if (state.getIn(['about', 'newtab', 'pinnedTopSites']).isEmpty()) { + state = state.setIn(['about', 'newtab', 'pinnedTopSites'], defaultPinnedSite) + } state = state.mergeIn(['about', 'newtab'], props.newTabPageDetail) return state.setIn(['about', 'newtab', 'updatedStamp'], new Date().getTime()) }, diff --git a/app/sessionStore.js b/app/sessionStore.js index 5d7cd0af298..57fb0f68f94 100644 --- a/app/sessionStore.js +++ b/app/sessionStore.js @@ -32,7 +32,6 @@ const windowState = require('./common/state/windowState') // Utils const locale = require('./locale') -const {pinnedTopSites} = require('../js/data/newTabData') const {defaultSiteSettingsList} = require('../js/data/siteSettingsList') const filtering = require('./filtering') const autofill = require('./autofill') @@ -1019,7 +1018,7 @@ module.exports.defaultAppState = () => { gridLayoutSize: 'small', sites: [], ignoredTopSites: [], - pinnedTopSites: pinnedTopSites + pinnedTopSites: [] }, welcome: { showOnLoad: !['test', 'development'].includes(process.env.NODE_ENV) diff --git a/js/about/newtab.js b/js/about/newtab.js index 112955909de..9857c75476b 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -97,7 +97,7 @@ class NewTabPage extends React.Component { } get pinnedTopSites () { - return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List()) + return this.state.newTabData.getIn(['newTabDetail', 'pinnedTopSites'], Immutable.List()).setSize(100) } get ignoredTopSites () { @@ -120,7 +120,6 @@ class NewTabPage extends React.Component { get gridLayout () { const sizeToCount = {large: 18, medium: 12, small: 6} const count = sizeToCount[this.gridLayoutSize] - return this.topSites.take(count) } From f016455103a435ed1076f9365e5b885775677581 Mon Sep 17 00:00:00 2001 From: Cezar Augusto Date: Thu, 4 Jan 2018 17:56:55 -0200 Subject: [PATCH 5/5] allow unpinning a website at any index --- js/about/newtab.js | 25 +++++++++++++------ .../reducers/aboutNewTabsReducerTest.js | 2 +- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/js/about/newtab.js b/js/about/newtab.js index 9857c75476b..daf056ccae5 100644 --- a/js/about/newtab.js +++ b/js/about/newtab.js @@ -176,15 +176,26 @@ class NewTabPage extends React.Component { } onPinnedTopSite (siteKey) { - const currentSiteIndex = this.topSites.findIndex(site => site.get('key') === siteKey) - const siteProps = this.topSites.find(site => site.get('key') === siteKey) - // ensure pinned sites are pinned in the right order - // do not edit these lines unless you've manually tested site visits - // after pinning and after dnd reordering + let sites = this.topSites let pinnedTopSites = this.pinnedTopSites - .splice(currentSiteIndex, 1, this.isPinned(siteKey) ? null : siteProps) - aboutActions.setNewTabDetail({pinnedTopSites: pinnedTopSites}, true) + const siteProps = sites.find(site => site.get('key') === siteKey) + + const currentSiteIndex = sites.findIndex(site => site.get('key') === siteKey) + const currentPinnedSiteIndex = pinnedTopSites + .findIndex(site => site && site.get('key') === siteKey) + + // ensure pinned sites are pinned in the right order when pinned + // if not pinned, pin and attach it to its position + if (!this.isPinned(siteKey)) { + pinnedTopSites = pinnedTopSites.splice(currentSiteIndex, 1, siteProps) + } else { + pinnedTopSites = pinnedTopSites.splice(currentPinnedSiteIndex, 1, null) + sites = sites.splice(currentPinnedSiteIndex, 1, siteProps) + aboutActions.setNewTabDetail({sites}, true) + } + + aboutActions.setNewTabDetail({pinnedTopSites}, true) } onIgnoredTopSite (siteKey) { diff --git a/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js b/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js index 52d13070420..6a370672637 100644 --- a/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js +++ b/test/unit/app/browser/reducers/aboutNewTabsReducerTest.js @@ -50,7 +50,7 @@ describe('aboutNewTabReducerTest', function () { const initialState = Immutable.fromJS({ settings: { }, about: { - newtab: { } + newtab: { pinnedTopSites: [] } } }) it('gets a value from default settings when nothing is set', () => {