From 5a256f2526d54c8bdf808c9338a357215e8ac26d Mon Sep 17 00:00:00 2001 From: "Brian R. Bondy" Date: Tue, 16 May 2017 16:18:22 -0400 Subject: [PATCH] Tokenize with urlParse and for non urls --- app/common/lib/siteSuggestions.js | 42 ++++++++-- app/common/lib/suggestion.js | 25 ++++-- app/renderer/components/navigation/urlBar.js | 9 ++- .../urlBarSuggestionsTest.js | 15 ++-- test/navbar-components/urlBarTest.js | 8 +- .../app/common/lib/siteSuggestionsTest.js | 81 ++++++++++++++++--- test/unit/app/common/lib/suggestionTest.js | 6 +- 7 files changed, 140 insertions(+), 46 deletions(-) diff --git a/app/common/lib/siteSuggestions.js b/app/common/lib/siteSuggestions.js index 5546f466d83..4c56b7bb52b 100644 --- a/app/common/lib/siteSuggestions.js +++ b/app/common/lib/siteSuggestions.js @@ -3,7 +3,9 @@ * You can obtain one at http://mozilla.org/MPL/2.0/. */ const Bloodhound = require('bloodhound-js') +const {isUrl} = require('../../../js/lib/appUrlUtil') const siteTags = require('../../../js/constants/siteTags') +const urlParse = require('../urlParse') let initialized = false let engine @@ -37,15 +39,24 @@ const init = (sites) => { return promise } +const getPartsFromNonUrlInput = (input) => + input.toLowerCase().split(/[,-.\s\\/?&]/) + const getTagToken = (tag) => '|' + tag + '|' + const tokenizeInput = (data) => { let url = data || '' let parts = [] if (typeof data === 'object' && data !== null) { + // When lastAccessTime is 1 it is a default built-in entry which we don't want + // to appear in suggestions. + if (data.lastAccessedTime === 1) { + return [] + } url = data.location if (data.title) { - parts = data.title.toLowerCase().split(/\s/) + parts = getPartsFromNonUrlInput(data.title) } if (data.tags) { parts = parts.concat(data.tags.map(getTagToken)) @@ -56,12 +67,27 @@ const tokenizeInput = (data) => { } } - if (url) { - url = url.toLowerCase().replace(/^https?:\/\//i, '') - parts = parts.concat(url.split(/[.\s\\/?&]/)) + if (url && isUrl(url)) { + const parsedUrl = urlParse(url.toLowerCase()) + if (parsedUrl.hash) { + parts.push(parsedUrl.hash.slice(1)) + } + if (parsedUrl.host) { + parts = parts.concat(parsedUrl.host.split('.')) + } + if (parsedUrl.pathname) { + parts = parts.concat(parsedUrl.pathname.split(/[.\s\\/]/)) + } + if (parsedUrl.query) { + parts = parts.concat(parsedUrl.query.split(/[&=]/)) + } + if (parsedUrl.protocol) { + parts = parts.concat(parsedUrl.protocol) + } + } else if (url) { + parts = parts.concat(getPartsFromNonUrlInput(url)) } - - return parts + return parts.filter(x => !!x) } const add = (data) => { @@ -81,8 +107,8 @@ const query = (input, options = {}) => { } return new Promise((resolve, reject) => { - const {getSortForSuggestions, normalizeLocation} = require('./suggestion') - input = normalizeLocation((input || '').toLowerCase()) + const {getSortForSuggestions} = require('./suggestion') + input = (input || '').toLowerCase() lastQueryOptions = Object.assign({}, options, { input, internalSort: getSortForSuggestions(input) diff --git a/app/common/lib/suggestion.js b/app/common/lib/suggestion.js index 0638d42610b..be8c83f3bfb 100644 --- a/app/common/lib/suggestion.js +++ b/app/common/lib/suggestion.js @@ -99,18 +99,18 @@ const sortByAccessCountWithAgeDecay = (s1, s2) => { } /* - * Return a 1 if the url is 'simple' as in without query, search or - * hash components. Return 0 otherwise. + * Return true1 the url is 'simple' as in without query, search or + * hash components. Return false otherwise. * - * @param {ImmutableObject} site - object represent history entry + * @param {object} An already normalized simple URL * */ -const simpleDomainNameValue = (site) => { +const isSimpleDomainNameValue = (site) => { const parsed = urlParse(getURL(site)) if (parsed.hash === null && parsed.search === null && parsed.query === null && parsed.pathname === '/') { - return 1 + return true } else { - return 0 + return false } } @@ -231,7 +231,16 @@ const getSortForSuggestions = (userInputLower) => (s1, s2) => { if (pos1 !== pos2) { return pos1 - pos2 } - return url1.length - url2.length + const url1IsSimple = isSimpleDomainNameValue(s1) + const url2IsSimple = isSimpleDomainNameValue(s2) + // If either both false or both true then rely on count w/ decay sort + if (url1IsSimple === url2IsSimple) { + return sortByAccessCountWithAgeDecay(s1, s2) + } + if (url1IsSimple) { + return -1 + } + return 1 } if (pos1 !== -1 && pos2 === -1) { return -1 @@ -434,7 +443,7 @@ module.exports = { sortingPriority, sortByAccessCountWithAgeDecay, getSortForSuggestions, - simpleDomainNameValue, + isSimpleDomainNameValue, normalizeLocation, shouldNormalizeLocation, createVirtualHistoryItems, diff --git a/app/renderer/components/navigation/urlBar.js b/app/renderer/components/navigation/urlBar.js index ee31de99489..a6ab07add08 100644 --- a/app/renderer/components/navigation/urlBar.js +++ b/app/renderer/components/navigation/urlBar.js @@ -234,11 +234,14 @@ class UrlBar extends React.Component { updateAutocomplete (newValue) { let suggestion = '' + let suggestionNormalized = '' if (this.props.suggestionList && this.props.suggestionList.size > 0) { - suggestion = normalizeLocation(this.props.suggestionList.getIn([this.activeIndex || 0, 'location'])) || '' + suggestion = this.props.suggestionList.getIn([this.activeIndex || 0, 'location']) || '' + suggestionNormalized = normalizeLocation(suggestion) } - if (suggestion.startsWith(newValue)) { - const newSuffix = suggestion.substring(newValue.length) + const newValueNormalized = normalizeLocation(newValue) + if (suggestionNormalized.startsWith(newValueNormalized) && suggestionNormalized.length > 0) { + const newSuffix = suggestionNormalized.substring(newValueNormalized.length) this.setValue(newValue, newSuffix) this.urlInput.setSelectionRange(newValue.length, newValue.length + newSuffix.length + 1) return true diff --git a/test/navbar-components/urlBarSuggestionsTest.js b/test/navbar-components/urlBarSuggestionsTest.js index 9f673a607e8..c1ce2cfb8c6 100644 --- a/test/navbar-components/urlBarSuggestionsTest.js +++ b/test/navbar-components/urlBarSuggestionsTest.js @@ -46,7 +46,7 @@ describe('urlBarSuggestions', function () { it('show suggestion when single letter is typed in', function * () { yield this.app.client.ipcSend('shortcut-focus-url') .waitForElementFocus(urlInput) - .setInputText(urlInput, 'a') + .setInputText(urlInput, 'about:about') .waitForExist(urlBarSuggestions) }) @@ -103,13 +103,13 @@ describe('urlBarSuggestions', function () { .keys(Brave.keys.DOWN) .waitForExist(urlBarSuggestions + ' li.suggestionItem[data-index="1"].selected') .keys(Brave.keys.ENTER) - .tabByIndex(1).getUrl().should.become(this.page1Url) + .tabByIndex(1).getUrl().should.become(this.page2Url) }) it('selects a location auto complete result but not for titles', function * () { yield this.app.client .setValue(urlInput, 'http://') - .waitForInputText(urlInput, Brave.server.urlOrigin()) + .waitForInputText(urlInput, this.page1Url) .waitForExist(urlBarSuggestions + ' li.selected') .setValue(urlInput, 'Page') .waitForElementCount(urlBarSuggestions + ' li.selected', 0) @@ -124,9 +124,8 @@ describe('urlBarSuggestions', function () { // so that finally, if the rest of the 1st option is entered via keyboard, it overwrites the suggestion from the mouse yield this.app.client .keys(pagePartialUrl) - .waitForInputText(urlInput, page2Url) // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end) + .waitForInputText(urlInput, page1Url) // after entering partial URL matching two options, 1st is tentatively filled in (_without_ moving cursor to end) .waitForExist(urlBarSuggestions + ' li.suggestionItem') - .moveToObject(urlBarSuggestions + ' li.suggestionItem:not(.selected)') .waitForInputText(urlInput, page1Url) // mousing over 2nd option tentatively completes URL with 2nd option (_without_ moving cursor to end) .keys('2.html') .waitForInputText(urlInput, page2Url) // without moving mouse, typing rest of 1st option URL overwrites the autocomplete from mouseover @@ -134,7 +133,7 @@ describe('urlBarSuggestions', function () { it('selection is not reset when pressing non-input key', function * () { const pagePartialUrl = Brave.server.url('page') - const page1Url = Brave.server.url('page1.html') + const page2Url = Brave.server.url('page2.html') aboutHistoryState.setHistory(Immutable.fromJS({ about: { history: { @@ -148,10 +147,10 @@ describe('urlBarSuggestions', function () { .setValue(urlInput, pagePartialUrl) .waitForVisible(urlBarSuggestions) .keys(Brave.keys.DOWN) - .waitForInputText(urlInput, page1Url) + .waitForInputText(urlInput, page2Url) .keys(Brave.keys.CONTROL) .keys(Brave.keys.CONTROL) - .waitForSelectedText('1.html') + .waitForSelectedText('2.html') }) it('non-prefixed active suggestion loads the suggestion when enter is pressed', function * () { diff --git a/test/navbar-components/urlBarTest.js b/test/navbar-components/urlBarTest.js index 7e14665d1e2..3eb7055651f 100644 --- a/test/navbar-components/urlBarTest.js +++ b/test/navbar-components/urlBarTest.js @@ -271,7 +271,7 @@ describe('urlBar tests', function () { it('does not show suggestions', function * () { yield this.app.client .keys('brave') - .waitForVisible(urlBarSuggestions, 1) + .waitForVisible(urlBarSuggestions) .ipcSend('shortcut-focus-url') .waitForElementFocus(urlInput) .waitForElementCount(urlBarSuggestions, 0) @@ -295,12 +295,12 @@ describe('urlBar tests', function () { .waitForVisible(urlBarSuggestions) // highlight for autocomplete brianbondy.com .moveToObject(urlBarSuggestions, 0, 100) - yield selectsText(this.app.client, 'rianbondy.com') - .keys('ra') + yield selectsText(this.app.client, 'rave.com/test3') + .keys('rian') .execute(function (urlBarSuggestions) { document.querySelector(urlBarSuggestions).scrollTop = 200 }, urlBarSuggestions) - yield selectsText(this.app.client, 've.com') + yield selectsText(this.app.client, 'bondy.com/test4') }) }) diff --git a/test/unit/app/common/lib/siteSuggestionsTest.js b/test/unit/app/common/lib/siteSuggestionsTest.js index c03d4223a54..6f7bd71ebd6 100644 --- a/test/unit/app/common/lib/siteSuggestionsTest.js +++ b/test/unit/app/common/lib/siteSuggestionsTest.js @@ -50,11 +50,14 @@ describe('siteSuggestions lib', function () { it('lowercases tokens', function () { assert.deepEqual(tokenizeInput('BRaD HaTES PRIMES'), ['brad', 'hates', 'primes']) }) - it('does not include http', function () { - assert.deepEqual(tokenizeInput('http://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html']) + it('includes protocol', function () { + assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'https:']) }) - it('does not include https as a token', function () { - assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html']) + it('includes query', function () { + assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html?test=abc&test2=abcd'), ['bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'test', 'abc', 'test2', 'abcd', 'https:']) + }) + it('does not include hash', function () { + assert.deepEqual(tokenizeInput('https://bradrichter.co/I/hate/primes.html?test=abc#testing'), ['testing', 'bradrichter', 'co', 'i', 'hate', 'primes', 'html', 'test', 'abc', 'https:']) }) it('spaces get tokenized', function () { assert.deepEqual(tokenizeInput('brad\thates primes'), ['brad', 'hates', 'primes']) @@ -68,14 +71,11 @@ describe('siteSuggestions lib', function () { it('\\ gets tokenized', function () { assert.deepEqual(tokenizeInput('brad\\hates\\primes'), ['brad', 'hates', 'primes']) }) - it('? gets tokenized', function () { - assert.deepEqual(tokenizeInput('brad?hates?primes'), ['brad', 'hates', 'primes']) - }) - it('& gets tokenized', function () { - assert.deepEqual(tokenizeInput('brad&hates&primes'), ['brad', 'hates', 'primes']) - }) it('can tokenize site objects', function () { - assert.deepEqual(tokenizeInput(site1), ['do', 'not', 'use', '3', 'for', 'items', 'because', 'it', 'is', 'prime', 'www', 'bradrichter', 'co', 'bad_numbers', '3']) + assert.deepEqual(tokenizeInput(site1), ['do', 'not', 'use', '3', 'for', 'items', 'because', 'it', 'is', 'prime', 'www', 'bradrichter', 'co', 'bad_numbers', '3', 'https:']) + }) + it('non URLs get tokenized', function () { + assert.deepEqual(tokenizeInput('hello world Greatest...Boss...Ever'), ['hello', 'world', 'greatest', 'boss', 'ever']) }) }) @@ -127,7 +127,7 @@ describe('siteSuggestions lib', function () { checkResult('brave brad', [], cb) }) }) - describe('query sorts results', function () { + describe('query sorts results by location', function () { before(function (cb) { const sites = Immutable.fromJS([{ location: 'https://brave.com/twi' @@ -173,6 +173,63 @@ describe('siteSuggestions lib', function () { }) }) }) + describe('query sorts results by count', function () { + before(function (cb) { + this.page2 = { + location: 'https://brave.com/page2', + count: 20 + } + const sites = Immutable.fromJS([{ + location: 'https://brave.com/page1', + count: 5 + }, this.page2, { + location: 'https://brave.com/page3', + count: 2 + }]) + init(sites).then(cb.bind(null, null)) + }) + it('highest count first', function (cb) { + query('https://brave.com/page').then((results) => { + assert.deepEqual(results[0], this.page2) + cb() + }) + }) + }) + describe('query respects lastAccessTime', function () { + before(function (cb) { + this.site = { + location: 'https://bravebrowser.com/page2', + lastAccessedTime: 1494958046427, // most recent + count: 1 + } + const sites = Immutable.fromJS([{ + location: 'https://bravez.com/page1', + lastAccessedTime: 1, + count: 1 + }, { + location: 'https://bravebrowser.com/page1', + lastAccessedTime: 1494957046426, + count: 1 + }, this.site, { + location: 'https://bravebrowser.com/page3', + lastAccessedTime: 1494957046437, + count: 1 + }]) + init(sites).then(cb.bind(null, null)) + }) + it('items with lastAccessTime of 1 get ignored (signifies preloaded default)', function (cb) { + query('https://bravez.com/page').then((results) => { + assert.equal(results.length, 0) + cb() + }) + }) + it('most recently accessed get sorted first', function (cb) { + query('bravebrowser').then((results) => { + assert.deepEqual(results[0], this.site) + cb() + }) + }) + }) describe('add sites after init', function () { before(function (cb) { const sites = [site1, site2, site3, site4] diff --git a/test/unit/app/common/lib/suggestionTest.js b/test/unit/app/common/lib/suggestionTest.js index 4e8c4f11c73..8946b67908b 100644 --- a/test/unit/app/common/lib/suggestionTest.js +++ b/test/unit/app/common/lib/suggestionTest.js @@ -68,12 +68,12 @@ describe('suggestion unit tests', function () { }) }) - describe('simpleDomainNameValue', function () { + describe('isSimpleDomainNameValue', function () { it('sorts simple sites higher than complex sites', function () { const siteSimple = Immutable.Map({ location: 'http://www.site.com' }) const siteComplex = Immutable.Map({ location: 'http://www.site.com/?foo=bar#a' }) - assert.ok(suggestion.simpleDomainNameValue(siteSimple) === 1, 'simple site returns 1') - assert.ok(suggestion.simpleDomainNameValue(siteComplex) === 0, 'complex site returns 0') + assert.ok(suggestion.isSimpleDomainNameValue(siteSimple) === true, 'simple site returns 1') + assert.ok(suggestion.isSimpleDomainNameValue(siteComplex) === false, 'complex site returns 0') }) })