Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Remove entries from History/Domain completers when they're removed on chrome. #715

Merged
merged 9 commits into from

2 participants

@smblott-github
Collaborator

Currently, if a history entry is removed in chrome (or the entire history is removed), then the effects do not propagate into vimium until the next restart.

This is a pretty inconsistent UX.

The attached code fixes this inconsistency.


(Edit: Comments regarding bug in binarySearch deleted.)

smblott-github added some commits
@smblott-github smblott-github Remove history entries.
When a chrome history entry is removed, remove that entry from our
history too.  Same when the entire history is removed.
3dc8a03
@smblott-github smblott-github Extend removing entries to the domain completer. 03823f9
@smblott-github
Collaborator

Doh! A short walk on the beach and I see how binarySearch is working. I withdraw (and apologize for) the "bug" comments above.

background_scripts/completion.coffee
@@ -169,7 +169,7 @@ class HistoryCompleter
# The domain completer is designed to match a single-word query which looks like it is a domain. This supports
# the user experience where they quickly type a partial domain, hit tab -> enter, and expect to arrive there.
class DomainCompleter
- domains: null # A map of domain -> history
+ domains: null # A map of domain -> { entry: <historyEntry>, referenceCount: <count> }
@philc Owner
philc added a note

Let's add a comment here to explain what reference count is and why it exists.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit_tests/completion_test.coffee
@@ -41,6 +41,9 @@ context "HistoryCache",
should "return length - 1 if it should be at the end of the list", ->
assert.equal 0, HistoryCache.binarySearch(3, [3, 5, 8], @compare)
+ should "return one passed end of list (so: list.length) if greater than last element in list", ->
@philc Owner
philc added a note

Great, nice test!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit_tests/completion_test.coffee
@@ -75,6 +80,30 @@ context "HistoryCache",
HistoryCache.use (@results) =>
assert.arrayEqual [newSite, @history1], @results
+ should "remove pages from the history, when page is not in history", ->
@philc Owner
philc added a note

The name of this test is a bit confusing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit_tests/completion_test.coffee
@@ -120,6 +155,35 @@ context "domain completer",
should "returns no results when there's more than one query term, because clearly it's not a domain", ->
assert.arrayEqual [], filterCompleter(@completer, ["his", "tory"])
+ should "remove 1 matching domain entry", ->
@philc Owner
philc added a note

These tests should be wrapped in a context to group them and describe which area they're testing, and it looks like filterCompleter(@completer, ["story"]) # Force installation of @onVisitRemovedListener. should be in a setup block

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
tests/unit_tests/completion_test.coffee
@@ -120,6 +155,35 @@ context "domain completer",
should "returns no results when there's more than one query term, because clearly it's not a domain", ->
assert.arrayEqual [], filterCompleter(@completer, ["his", "tory"])
+ should "remove 1 matching domain entry", ->
+ # Force installation of `@onVisitRemovedListener`
+ filterCompleter(@completer, ["story"])
+ @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+ assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+
+ should "remove all entries from one domain", ->
+ filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
+ @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
+ @onVisitRemovedListener { allHistory: false, urls: [ @history3.url ] }
+ assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+
+ should "remove 3 (all) matching domain entries", ->
+ filterCompleter(@completer, ["story"]) # Force installation of `@onVisitRemovedListener`.
+ @onVisitRemovedListener { allHistory: false, urls: [ @history2.url ] }
@philc Owner
philc added a note

No spaces between the array brackets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@philc
Owner

This is a nice case to handle, thanks for tackling it. If you can make the requested changes we'll merge it in.

@smblott-github
Collaborator

Thanks @philc -- I think all of your issues are addressed now.

@philc
Owner

lgtm, thanks!

@philc philc merged commit 32e506d into philc:master
@ForSpareParts ForSpareParts referenced this pull request from a commit in ForSpareParts/vimium
@smblott-github smblott-github Changes in responce to philc's recommendations. ceae1cb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
42 background_scripts/completion.coffee
@@ -169,7 +169,11 @@ class HistoryCompleter
# The domain completer is designed to match a single-word query which looks like it is a domain. This supports
# the user experience where they quickly type a partial domain, hit tab -> enter, and expect to arrive there.
class DomainCompleter
- domains: null # A map of domain -> history
+ # A map of domain -> { entry: <historyEntry>, referenceCount: <count> }
+ # - `entry` is the most recently accessed page in the History within this domain.
+ # - `referenceCount` is a count of the number of History entries within this domain.
+ # If `referenceCount` goes to zero, the domain entry can and should be deleted.
+ domains: null
filter: (queryTerms, onComplete) ->
return onComplete([]) if queryTerms.length > 1
@@ -190,7 +194,7 @@ class DomainCompleter
sortDomainsByRelevancy: (queryTerms, domainCandidates) ->
results = []
for domain in domainCandidates
- recencyScore = RankingUtils.recencyScore(@domains[domain].lastVisitTime || 0)
+ recencyScore = RankingUtils.recencyScore(@domains[domain].entry.lastVisitTime || 0)
wordRelevancy = RankingUtils.wordRelevancy(queryTerms, domain, null)
score = (wordRelevancy + Math.max(recencyScore, wordRelevancy)) / 2
results.push([domain, score])
@@ -200,18 +204,27 @@ class DomainCompleter
populateDomains: (onComplete) ->
HistoryCache.use (history) =>
@domains = {}
- history.forEach (entry) =>
- # We want each key in our domains hash to point to the most recent History entry for that domain.
- domain = @parseDomain(entry.url)
- if domain
- previousEntry = @domains[domain]
- @domains[domain] = entry if !previousEntry || (previousEntry.lastVisitTime < entry.lastVisitTime)
+ history.forEach (entry) => @onPageVisited entry
chrome.history.onVisited.addListener(@onPageVisited.bind(this))
+ chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
onComplete()
onPageVisited: (newPage) ->
domain = @parseDomain(newPage.url)
- @domains[domain] = newPage if domain
+ if domain
+ slot = @domains[domain] ||= { entry: newPage, referenceCount: 0 }
+ # We want each entry in our domains hash to point to the most recent History entry for that domain.
+ slot.entry = newPage if slot.entry.lastVisitTime < newPage.lastVisitTime
+ slot.referenceCount += 1
+
+ onVisitRemoved: (toRemove) ->
+ if toRemove.allHistory
+ @domains = {}
+ else
+ toRemove.urls.forEach (url) =>
+ domain = @parseDomain(url)
+ if domain and @domains[domain] and ( @domains[domain].referenceCount -= 1 ) == 0
+ delete @domains[domain]
parseDomain: (url) -> url.split("/")[2] || ""
@@ -365,6 +378,7 @@ HistoryCache =
history.sort @compareHistoryByUrl
@history = history
chrome.history.onVisited.addListener(@onPageVisited.bind(this))
+ chrome.history.onVisitRemoved.addListener(@onVisitRemoved.bind(this))
callback(@history) for callback in @callbacks
@callbacks = null
@@ -383,6 +397,16 @@ HistoryCache =
else
@history.splice(i, 0, newPage)
+ # When a page is removed from the chrome history, remove it from the vimium history too.
+ onVisitRemoved: (toRemove) ->
+ if toRemove.allHistory
+ @history = []
+ else
+ toRemove.urls.forEach (url) =>
+ i = HistoryCache.binarySearch({url:url}, @history, @compareHistoryByUrl)
+ if i < @history.length and @history[i].url == url
+ @history.splice(i, 1)
+
# Returns the matching index or the closest matching index if the element is not found. That means you
# must check the element at the returned index to know whether the element was actually found.
# This method is used for quickly searching through our history cache.
View
80 tests/unit_tests/completion_test.coffee
@@ -41,6 +41,9 @@ context "HistoryCache",
should "return length - 1 if it should be at the end of the list", ->
assert.equal 0, HistoryCache.binarySearch(3, [3, 5, 8], @compare)
+ should "return one passed end of array (so: array.length) if greater than last element in array", ->
+ assert.equal 3, HistoryCache.binarySearch(10, [3, 5, 8], @compare)
+
should "found return the position if it's between two elements", ->
assert.equal 1, HistoryCache.binarySearch(4, [3, 5, 8], @compare)
assert.equal 2, HistoryCache.binarySearch(7, [3, 5, 8], @compare)
@@ -51,9 +54,11 @@ context "HistoryCache",
@history2 = { url: "a.com", lastVisitTime: 10 }
history = [@history1, @history2]
@onVisitedListener = null
+ @onVisitRemovedListener = null
global.chrome.history =
search: (options, callback) -> callback(history)
onVisited: { addListener: (@onVisitedListener) => }
+ onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
HistoryCache.reset()
should "store visits sorted by url ascending", ->
@@ -75,6 +80,30 @@ context "HistoryCache",
HistoryCache.use (@results) =>
assert.arrayEqual [newSite, @history1], @results
+ should "(not) remove page from the history, when page is not in history (it should be a no-op)", ->
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [@history2, @history1], @results
+ toRemove = { urls: [ "x.com" ], allHistory: false }
+ @onVisitRemovedListener(toRemove)
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [@history2, @history1], @results
+
+ should "remove pages from the history", ->
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [@history2, @history1], @results
+ toRemove = { urls: [ "a.com" ], allHistory: false }
+ @onVisitRemovedListener(toRemove)
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [@history1], @results
+
+ should "remove all pages from the history", ->
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [@history2, @history1], @results
+ toRemove = { allHistory: true }
+ @onVisitRemovedListener(toRemove)
+ HistoryCache.use (@results) =>
+ assert.arrayEqual [], @results
+
context "history completer",
setup ->
@history1 = { title: "history1", url: "history1.com", lastVisitTime: hours(1) }
@@ -83,6 +112,7 @@ context "history completer",
global.chrome.history =
search: (options, callback) => callback([@history1, @history2])
onVisited: { addListener: -> }
+ onVisitRemoved: { addListener: -> }
@completer = new HistoryCompleter()
@@ -102,7 +132,9 @@ context "domain completer",
@history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2]))
- global.chrome.history = { onVisited: { addListener: -> } }
+ global.chrome.history =
+ onVisited: { addListener: -> }
+ onVisitRemoved: { addListener: -> }
stub(Date, "now", returns(hours(24)))
@completer = new DomainCompleter()
@@ -112,7 +144,7 @@ context "domain completer",
assert.arrayEqual ["history1.com"], results.map (result) -> result.url
should "pick domains which are more recent", ->
- # This domains are the same except for their last visited time.
+ # These domains are the same except for their last visited time.
assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
@history2.lastVisitTime = hours(3)
assert.equal "history2.com", filterCompleter(@completer, ["story"])[0].url
@@ -120,6 +152,50 @@ context "domain completer",
should "returns no results when there's more than one query term, because clearly it's not a domain", ->
assert.arrayEqual [], filterCompleter(@completer, ["his", "tory"])
+context "domain completer (removing entries)",
+ setup ->
+ @history1 = { title: "history1", url: "http://history1.com", lastVisitTime: hours(2) }
+ @history2 = { title: "history2", url: "http://history2.com", lastVisitTime: hours(1) }
+ @history3 = { title: "history2something", url: "http://history2.com/something", lastVisitTime: hours(0) }
+
+ stub(HistoryCache, "use", (onComplete) => onComplete([@history1, @history2, @history3]))
+ @onVisitedListener = null
+ @onVisitRemovedListener = null
+ global.chrome.history =
+ onVisited: { addListener: (@onVisitedListener) => }
+ onVisitRemoved: { addListener: (@onVisitRemovedListener) => }
+ stub(Date, "now", returns(hours(24)))
+
+ @completer = new DomainCompleter()
+ # Force installation of listeners.
+ filterCompleter(@completer, ["story"])
+
+ should "remove 1 entry for domain with reference count of 1", ->
+ @onVisitRemovedListener { allHistory: false, urls: [@history1.url] }
+ assert.equal "history2.com", filterCompleter(@completer, ["story"])[0].url
+ assert.equal 0, filterCompleter(@completer, ["story1"]).length
+
+ should "remove 2 entries for domain with reference count of 2", ->
+ @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
+ assert.equal "history2.com", filterCompleter(@completer, ["story2"])[0].url
+ @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
+ assert.equal 0, filterCompleter(@completer, ["story2"]).length
+ assert.equal "history1.com", filterCompleter(@completer, ["story"])[0].url
+
+ should "remove 3 (all) matching domain entries", ->
+ @onVisitRemovedListener { allHistory: false, urls: [@history2.url] }
+ @onVisitRemovedListener { allHistory: false, urls: [@history1.url] }
+ @onVisitRemovedListener { allHistory: false, urls: [@history3.url] }
+ assert.equal 0, filterCompleter(@completer, ["story"]).length
+
+ should "remove 3 (all) matching domain entries, and do it all at once", ->
+ @onVisitRemovedListener { allHistory: false, urls: [ @history2.url, @history1.url, @history3.url ] }
+ assert.equal 0, filterCompleter(@completer, ["story"]).length
+
+ should "remove *all* domain entries", ->
+ @onVisitRemovedListener { allHistory: true }
+ assert.equal 0, filterCompleter(@completer, ["story"]).length
+
context "tab completer",
setup ->
@tabs = [
Something went wrong with that request. Please try again.