From b361e50686605b2e276cef53ab7456e0f7f35016 Mon Sep 17 00:00:00 2001 From: Aubrey Keus Date: Fri, 14 Oct 2016 20:57:45 -0400 Subject: [PATCH] Change URL suggestions to consider last access timestamp and access count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Re-order results to show history items before bookmark items * Record number of times site is visited * Re-order history items based on number of times visited and last access timestamp * Update and create tests Fixes: #3049 Auditors: @bbondy Test plan: A new attribute, count, is added to history items in the sites section of save-session-1. This attribute records the number of times a site has been accessed. The test plan is broken in two components, one to verify the count attribute is incremented on access, the second to verify that the count and last access time are used when sorting history suggestions. Scenario 1 Part A 1. Clear save-session-1 2. Access a site 3. Close browser 4. Verify that the count attribute for the saved site in save-session-1 has a value of 1 Part B 1. Open browser 2. Navigate to site from Part A 3. Close browser 4. Verify that the count attribute for the saved site in save-session-1 has an incremented value Scenario 2 1. Clear save-session-1 2. Open browser 3. Visit gm.com and gm.ca 4. Close browser 5. Verify that the count attributes of the two new sites are identical (1) 6. Open browser to a new tab 7. Enter ‘gm’ into the url bar 8. Verify that the most recently visited of the two gm sites is the first entry in the history auto-suggest list --- app/renderer/lib/suggestion.js | 84 ++++++++++++++++++++++++++++++ js/components/urlBarSuggestions.js | 28 +++++----- js/constants/appConfig.js | 3 ++ js/state/siteUtil.js | 4 ++ test/unit/lib/urlSuggestionTest.js | 15 ++++++ test/unit/state/siteUtilTest.js | 17 +++++- 6 files changed, 136 insertions(+), 15 deletions(-) create mode 100644 app/renderer/lib/suggestion.js create mode 100644 test/unit/lib/urlSuggestionTest.js diff --git a/app/renderer/lib/suggestion.js b/app/renderer/lib/suggestion.js new file mode 100644 index 00000000000..30f461fd0c6 --- /dev/null +++ b/app/renderer/lib/suggestion.js @@ -0,0 +1,84 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this file, + * You can obtain one at http://mozilla.org/MPL/2.0/. */ + +const appConfig = require('../../../js/constants/appConfig') + +const sigmoid = (t) => { + return 1 / (1 + Math.pow(Math.E, -t)) +} + +const ONE_DAY = 1000 * 60 * 60 * 24 + +/* + * Calculate the sorting priority for a history item based on number of + * accesses and time since last access + * + * @param {number} count - The number of times this site has been accessed + * @param {number} currentTime - Current epoch millisecnds + * @param {boolean} lastAccessedTime - Epoch milliseconds of last access + * + */ +module.exports.sortingPriority = (count, currentTime, lastAccessedTime, ageDecayConstant) => { + // number of days since last access (with fractional component) + const ageInDays = (currentTime - (lastAccessedTime || currentTime)) / ONE_DAY + // decay factor based on age + const ageFactor = 1 - ((sigmoid(ageInDays / ageDecayConstant) - 0.5) * 2) + // sorting priority + // console.log(count, ageInDays, ageFactor, count * ageFactor) + return count * ageFactor +} + +/* + * Sort two history items by priority + * + * @param {ImmutableObject} s1 - first history item + * @param {ImmutableObject} s2 - second history item + * + * Return the relative order of two site entries taking into consideration + * the number of times the site has been accessed and the length of time + * since the last access. + * + * The base sort order is determined by the count attribute of the site + * entry. A modifier is then computed based on the length of time since + * the last access. A sigmoid function is used to weight more recent + * entries higher than entries in the past. This is not a linear function, + * entries in the far past with many counts will still be discounted + * heavily as the sigmoid modifier will cancel most of the count + * base parameter. + * + * Below is a sample comparison of two sites that have been accessed + * recently (but not at the identical time). Each site is accessed + * 9 times. The count is discounted by an aging factor calculated + * using the sigmoid decay function. + * + * http://www.gm.ca/gm/ + * + * ageInDays 0.17171469907407408 + * ageFactor 0.9982828546969802 + * count 9 + * priority 0.9982828546969802 + * + * http://www.gm.com/index.html + * + * ageInDays 0.17148791666666666 + * ageFactor 0.9982851225143763 + * count 9 + * priority 0.9982851225143763 + * + */ +module.exports.sortByAccessCountWithAgeDecay = (s1, s2) => { + const s1Priority = module.exports.sortingPriority( + s1.get('count') || 0, + (new Date()).getTime(), + s1.get('lastAccessedTime') || (new Date()).getTime(), + appConfig.urlSuggestions.ageDecayConstant + ) + const s2Priority = module.exports.sortingPriority( + s2.get('count') || 0, + (new Date()).getTime(), + s2.get('lastAccessedTime') || (new Date()).getTime(), + appConfig.urlSuggestions.ageDecayConstant + ) + return s2Priority - s1Priority +} diff --git a/js/components/urlBarSuggestions.js b/js/components/urlBarSuggestions.js index 0278aa26b75..0ac8e0a1e54 100644 --- a/js/components/urlBarSuggestions.js +++ b/js/components/urlBarSuggestions.js @@ -21,6 +21,7 @@ const eventUtil = require('../lib/eventUtil') const cx = require('../lib/classSet') const locale = require('../l10n') const windowStore = require('../stores/windowStore') +const suggestion = require('../../app/renderer/lib/suggestion') class UrlBarSuggestions extends ImmutableComponent { constructor (props) { @@ -149,8 +150,8 @@ class UrlBarSuggestions extends ImmutableComponent { })) index += suggestions.size } - addToItems(bookmarkSuggestions, 'bookmarksTitle', locale.translation('bookmarksSuggestionTitle'), 'fa-star-o') addToItems(historySuggestions, 'historyTitle', locale.translation('historySuggestionTitle'), 'fa-clock-o') + addToItems(bookmarkSuggestions, 'bookmarksTitle', locale.translation('bookmarksSuggestionTitle'), 'fa-star-o') addToItems(aboutPagesSuggestions, 'aboutPagesTitle', locale.translation('aboutPagesSuggestionTitle'), null) addToItems(tabSuggestions, 'tabsTitle', locale.translation('tabsSuggestionTitle'), 'fa-external-link') addToItems(searchSuggestions, 'searchTitle', locale.translation('searchSuggestionTitle'), 'fa-search') @@ -254,18 +255,19 @@ class UrlBarSuggestions extends ImmutableComponent { if (pos1 - pos2 !== 0) { return pos1 - pos2 } else { - // If there's a tie on the match location, use the shorter URL - return s1.get('location').length - s2.get('location').length + // If there's a tie on the match location, use the age + // decay modified access count + return suggestion.sortByAccessCountWithAgeDecay(s1, s2) } } } - // bookmarks - if (getSetting(settings.BOOKMARK_SUGGESTIONS)) { + // history + if (getSetting(settings.HISTORY_SUGGESTIONS)) { suggestions = suggestions.concat(mapListToElements({ data: props.sites, - maxResults: config.urlBarSuggestions.maxBookmarkSites, - type: suggestionTypes.BOOKMARK, + maxResults: config.urlBarSuggestions.maxHistorySites, + type: suggestionTypes.HISTORY, clickHandler: navigateClickHandler((site) => { return site.get('location') }), @@ -277,17 +279,17 @@ class UrlBarSuggestions extends ImmutableComponent { const location = site.get('location') || '' return (title.toLowerCase().includes(urlLocationLower) || location.toLowerCase().includes(urlLocationLower)) && - site.get('tags') && site.get('tags').includes(siteTags.BOOKMARK) + (!site.get('tags') || site.get('tags').size === 0) } })) } - // history - if (getSetting(settings.HISTORY_SUGGESTIONS)) { + // bookmarks + if (getSetting(settings.BOOKMARK_SUGGESTIONS)) { suggestions = suggestions.concat(mapListToElements({ data: props.sites, - maxResults: config.urlBarSuggestions.maxHistorySites, - type: suggestionTypes.HISTORY, + maxResults: config.urlBarSuggestions.maxBookmarkSites, + type: suggestionTypes.BOOKMARK, clickHandler: navigateClickHandler((site) => { return site.get('location') }), @@ -299,7 +301,7 @@ class UrlBarSuggestions extends ImmutableComponent { const location = site.get('location') || '' return (title.toLowerCase().includes(urlLocationLower) || location.toLowerCase().includes(urlLocationLower)) && - (!site.get('tags') || site.get('tags').size === 0) + site.get('tags') && site.get('tags').includes(siteTags.BOOKMARK) } })) } diff --git a/js/constants/appConfig.js b/js/constants/appConfig.js index 18909c5bb93..b4f1ffa04f7 100644 --- a/js/constants/appConfig.js +++ b/js/constants/appConfig.js @@ -85,6 +85,9 @@ module.exports = { baseUrl: `${updateHost}/1/releases`, winBaseUrl: `${winUpdateHost}/multi-channel/releases/CHANNEL/` }, + urlSuggestions: { + ageDecayConstant: 50 + }, defaultSettings: { 'adblock.customRules': '', 'general.language': null, // null means to use the OS lang diff --git a/js/state/siteUtil.js b/js/state/siteUtil.js index 293a452497a..d4adcfde98e 100644 --- a/js/state/siteUtil.js +++ b/js/state/siteUtil.js @@ -124,6 +124,10 @@ const mergeSiteDetails = (oldSiteDetail, newSiteDetail, tag, folderId) => { if (newSiteDetail.get('themeColor') || oldSiteDetail && oldSiteDetail.get('themeColor')) { site = site.set('themeColor', newSiteDetail.get('themeColor') || oldSiteDetail.get('themeColor')) } + if (site.get('tags').size === 0) { + // Increment the visit count for history items + site = site.set('count', ((oldSiteDetail || site).get('count') || 0) + 1) + } return site } diff --git a/test/unit/lib/urlSuggestionTest.js b/test/unit/lib/urlSuggestionTest.js new file mode 100644 index 00000000000..1e89181f4e5 --- /dev/null +++ b/test/unit/lib/urlSuggestionTest.js @@ -0,0 +1,15 @@ +/* global describe, it */ +const suggestion = require('../../../app/renderer/lib/suggestion') +const assert = require('assert') + +require('../braveUnit') + +const AGE_DECAY = 50 + +describe('suggestion', function () { + it('sorts sites correctly', function () { + assert.ok(suggestion.sortingPriority(10, 100, 50, AGE_DECAY) > suggestion.sortingPriority(10, 100, 40, AGE_DECAY), 'newer sites with equal access counts sort earlier') + assert.ok(suggestion.sortingPriority(10, 100, 50, AGE_DECAY) < suggestion.sortingPriority(11, 100, 40, AGE_DECAY), 'Sites with higher access counts sort earlier (unless time delay overriden)') + assert.ok(suggestion.sortingPriority(10, 10000000000, 10000000000, AGE_DECAY) > suggestion.sortingPriority(11, 10000000000, 1000000000, AGE_DECAY), 'much newer sites without lower counts sort with higher priority') + }) +}) diff --git a/test/unit/state/siteUtilTest.js b/test/unit/state/siteUtilTest.js index 64f736118a7..bf5e4a12f59 100644 --- a/test/unit/state/siteUtilTest.js +++ b/test/unit/state/siteUtilTest.js @@ -27,6 +27,10 @@ describe('siteUtil', function () { parentFolderId: 0, tags: [siteTags.BOOKMARK_FOLDER] }) + const siteMinFields = Immutable.fromJS({ + location: testUrl1, + title: 'sample' + }) describe('getSiteIndex', function () { it('returns -1 if sites is falsey', function () { @@ -178,7 +182,17 @@ describe('siteUtil', function () { const expectedSites = Immutable.fromJS([bookmarkAllFields]) assert.deepEqual(processedSites.getIn([0, 'tags']), expectedSites.getIn([0, 'tags'])) }) - + describe('record count', function () { + var processedSites + it('create history record with count', function () { + processedSites = siteUtil.addSite(emptySites, siteMinFields) + assert.deepEqual(processedSites.getIn([0, 'count']), 1) + }) + it('increments count for history item', function () { + processedSites = siteUtil.addSite(processedSites, siteMinFields) + assert.deepEqual(processedSites.getIn([0, 'count']), 2) + }) + }) describe('for new entries (oldSite is null)', function () { describe('when adding bookmark', function () { it('preserves existing siteDetail fields', function () { @@ -192,7 +206,6 @@ describe('siteUtil', function () { assert.deepEqual(processedSites.getIn([0, 'tags']).toJS(), [siteTags.BOOKMARK]) }) }) - describe('when adding bookmark folder', function () { it('assigns a folderId', function () { const processedSites = siteUtil.addSite(emptySites, folderMinFields)