Search completion; yet another reworking.

I'm having difficulty getting all aspects of the UX right for all
combinations of modes.  This is the latest attempt.  The main goal is to
mimic Chrome to the greatest extent possible.  There will be more to
come, but I think this is an improvement.

2 files changed, 105 insertions, 135 deletions

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index db151bed..d3d54521 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -403,12 +403,14 @@ class SearchEngineCompleter
         handler: "keywords"
         keywords: key for own key of engines
 
-  filter: ({ queryTerms, query, engine, fetchOnlyThePrimarySuggestion }, onComplete) ->
+  filter: (request, onComplete) ->
+    { queryTerms, query, engine } = request
     [ primarySuggestion, removePrimarySuggestion ] = [ null, false ]
 
     { custom, searchUrl, description } =
       if engine
         { keyword, searchUrl, description } = engine
+        extend request, { searchUrl, suppressLeadingKeyword: keyword }
         custom: true
         searchUrl: searchUrl
         description: description
@@ -420,8 +422,7 @@ class SearchEngineCompleter
     return onComplete [] unless custom or 0 < queryTerms.length
 
     factor = Settings.get "omniSearchWeight"
-    haveCompletionEngine = CompletionSearch.haveCompletionEngine searchUrl
-    haveCompletionEngine = false if factor == 0.0 and not custom
+    haveCompletionEngine = (0.0 < factor or custom) and CompletionSearch.haveCompletionEngine searchUrl
 
     # Relevancy:
     #   - Relevancy does not depend upon the actual suggestion (so, it does not depend upon word
@@ -436,47 +437,36 @@ class SearchEngineCompleter
     characterCount = query.length - queryTerms.length + 1
     relevancy = (if custom then 0.9 else factor) * (Math.min(characterCount, 10.0)/10.0)
 
-    # This distinguishes two very different kinds of vomnibar baviours, the newer bahviour (true) and the
-    # legacy behavior (false).  We retain the latter for the default search engine, and for custom search
-    # engines for which we do not have a completion engine.  By "exclusive vomnibar", we mean that
-    # this completer exclusively controls which suggestions may or may not be included, including filtering
-    # out suggestions from other completers.
-    useExclusiveVomnibar = custom and haveCompletionEngine
-
-    filter = null
-    if useExclusiveVomnibar
-      filter = (suggestions) ->
-        # We accept suggestions from this completer; and we also accept suggestions from other completers, but
-        # only if their URL matches this search engine and this query (ie. only if they could have been
-        # generated by this search engine previously).
-        suggestions = suggestions.filter (suggestion) ->
-          suggestion.type == description or
-            # This is a suggestion for the same search engine.
-            (suggestion.url.startsWith(engine.searchUrlPrefix) and
-              # And the URL suffix (which must contain the query part) matches the current query.
-              RankingUtils.matches queryTerms, suggestion.url[engine.searchUrlPrefix.length..])
-
-        if fetchOnlyThePrimarySuggestion
-          suggestions.filter (suggestion) -> suggestion == primarySuggestion
-        else if removePrimarySuggestion
-          suggestions.filter (suggestion) -> suggestion != primarySuggestion
-        else
-          suggestions
+    # This filter is applied to all of the suggestions from all of the completers.
+    filter = (suggestions) ->
+      return suggestions unless custom and haveCompletionEngine
+
+      # The primary suggestion was just a guess.  If we've managed fetch actual completions (asynchronously),
+      # then we now remove it.
+      if removePrimarySuggestion
+        suggestions = suggestions.filter (suggestion) -> suggestion != primarySuggestion
+
+      # We only accept suggestions:
+      #   - from this completer, or
+      #   - from other completers, but then only if their URL matches this search engine and this query
+      #     (that is only if their URL could have been generated by this search engine).
+      suggestions.filter (suggestion) ->
+        suggestion.type == description or
+          # This is a suggestion for the same search engine.
+          (suggestion.url.startsWith(engine.searchUrlPrefix) and
+            # And the URL suffix (which must contain the query part) matches the current query.
+            RankingUtils.matches queryTerms, suggestion.url[engine.searchUrlPrefix.length..])
 
     primarySuggestion = new Suggestion
       queryTerms: queryTerms
       type: description
       url: Utils.createSearchUrl queryTerms, searchUrl
       title: queryTerms.join " "
-      relevancy: relevancy
-      insertText: if useExclusiveVomnibar then query else null
-      # We suppress the leading keyword for custom search engines; for example, "w query terms" becomes just
-      # "query terms" in the vomnibar.
-      suppressLeadingKeyword: custom
-      # Toggles for the legacy behaviour.
-      autoSelect: not useExclusiveVomnibar
-      forceAutoSelect: not useExclusiveVomnibar
-      highlightTerms: not useExclusiveVomnibar
+      relevancy: 1
+      autoSelect: custom
+      forceAutoSelect: custom
+      highlightTerms: not haveCompletionEngine
+      searchSuggestionType: "primary"
 
     mkSuggestion = (suggestion) ->
       new Suggestion
@@ -487,40 +477,43 @@ class SearchEngineCompleter
         relevancy: relevancy *= 0.9
         insertText: suggestion
         highlightTerms: false
-        searchEngineCompletionSuggestion: true
+        searchSuggestionType: "completion"
 
     deliverCompletions = (onComplete, completions, args...) ->
       # Make the first suggestion float to the top of the vomnibar (except if we would be competing with the
       # domain completer, which also assigns a relevancy of 1).
       if 0 < completions.length
-        completions[0].relevancy = 1 if custom or (1 < queryTerms.length or /\S\s/.test query)
+        if custom or (1 < queryTerms.length or /\S\s/.test query)
+          extend completions[0],
+            relevancy: 1
+            autoSelect: custom
+            forceAutoSelect: custom
+            isPrimarySuggestion: custom
+            insertText: null
       onComplete completions, args...
 
-    # If we have cached suggestions, then we can bundle them immediately (otherwise we'll have to fetch them
-    # asynchronously).
-    cachedSuggestions = null
-    cachedSuggestions = CompletionSearch.complete searchUrl, queryTerms if haveCompletionEngine and not fetchOnlyThePrimarySuggestion
+    cachedSuggestions =
+      if haveCompletionEngine then CompletionSearch.complete searchUrl, queryTerms else null
 
     suggestions =
-      if haveCompletionEngine and cachedSuggestions? and 0 < cachedSuggestions.length and not fetchOnlyThePrimarySuggestion
+      if cachedSuggestions? and 0 < cachedSuggestions.length
         cachedSuggestions.map mkSuggestion
-      else if custom or fetchOnlyThePrimarySuggestion
+      else if custom
         [ primarySuggestion ]
       else
         []
 
-    if queryTerms.length == 0 or cachedSuggestions? or not haveCompletionEngine or fetchOnlyThePrimarySuggestion
-      # There is no prospect of adding further completions, or further completions will not be used (eg.
-      # because the vomnibar is closing and we've been asked for the primary suggestion only).
+    if queryTerms.length == 0 or cachedSuggestions? or not haveCompletionEngine
+      # There is no prospect of adding further completions.
       deliverCompletions onComplete, suggestions, { filter, continuation: null }
     else
-      # Post initial suggestions, then deliver further completions asynchronously, as a continuation.
+      # Post the initial suggestions, then deliver further completions asynchronously, as a continuation.
       deliverCompletions onComplete, suggestions,
         filter: filter
         continuation: (onComplete) =>
           CompletionSearch.complete searchUrl, queryTerms, (suggestions = []) =>
             console.log "fetched suggestions:", suggestions.length, query if SearchEngineCompleter.debug
-            removePrimarySuggestion = primarySuggestion? and 0 < suggestions.length
+            removePrimarySuggestion = 0 < suggestions.length
             deliverCompletions onComplete, suggestions.map mkSuggestion
 
 # A completer which calls filter() on many completers, aggregates the results, ranks them, and returns the top
diff --git a/pages/vomnibar.coffee b/pages/vomnibar.coffee
index c17a14f5..b168abf0 100644
--- a/pages/vomnibar.coffee
+++ b/pages/vomnibar.coffee
@@ -63,7 +63,6 @@ class VomnibarUI
     @postHideCallback = null
 
   reset: ->
-    @fetchOnlyThePrimarySuggestion = false
     @clearUpdateTimer()
     @completionList.style.display = ""
     @input.value = ""
@@ -71,7 +70,6 @@ class VomnibarUI
     @previousAutoSelect = null
     @previousInputValue = null
     @suppressedLeadingKeyword = null
-    @previousLength = 0
     @selection = @initialSelectionValue
     @keywords = []
 
@@ -85,9 +83,14 @@ class VomnibarUI
     else
       @previousAutoSelect = null
 
+    # Notwithstanding all of the above, disable autoSelect if the user is deleting text from the query.
+    if @lastAction == "delete"
+      @selection = -1
+      @previousAutoSelect = null
+
     # For custom search engines, we suppress the leading term (e.g. the "w" of "w query terms") within the
     # vomnibar input.
-    if @completions[0]?.suppressLeadingKeyword and not @suppressedLeadingKeyword?
+    if @lastReponse.suppressLeadingKeyword and not @suppressedLeadingKeyword?
       queryTerms = @input.value.trim().split /\s+/
       @suppressedLeadingKeyword = queryTerms[0]
       @input.value = queryTerms[1..].join " "
@@ -117,26 +120,21 @@ class VomnibarUI
   # This adds prompted text to the vomnibar input.  The prompted text is a continuation of the text the user
   # has already typed, taken from one of the search suggestions.  It is highlight (using the selection) and
   # will be included with the query should the user type <Enter>.
-  addPromptedText: (response) ->
+  addPromptedText: ->
     # Bail if we don't yet have the background completer's final word on the current query.
-    return unless response.mayCacheResults
-
-    value = @getInputWithoutPromptedText()
-    @previousLength ?= value.length
-    previousLength = @previousLength
-    currentLength = value.length
-    @previousLength = currentLength
+    return unless @lastReponse.mayCacheResults
 
-    return unless previousLength < currentLength
-    return if /^\s/.test(value) or /\s\s/.test value
+    # Bail if the last action was "delete"; or we may be putting back what the user just deleted.
+    return if @lastAction == "delete"
 
-    # Bail if there's an update pending (because then @input and the completion state are out of sync).
+    # Bail if there's an update pending, because @input and the completion state are out of sync.
     return if @updateTimer?
 
-    completions = @completions.filter (completion) -> completion.searchEngineCompletionSuggestion
+    completions = @completions.filter (completion) ->
+      completion. searchSuggestionType in [ "primary", "completion" ]
     return unless 0 < completions.length
 
-    query = value.ltrim().split(/\s+/).join(" ").toLowerCase()
+    query = @getInputWithoutPromptedText().ltrim().split(/\s+/).join(" ").toLowerCase()
     suggestion = completions[0].title
 
     index = suggestion.toLowerCase().indexOf query
@@ -175,7 +173,7 @@ class VomnibarUI
     null
 
   onKeydown: (event) =>
-    action = @actionFromKeyEvent(event)
+    @lastAction = action = @actionFromKeyEvent event
     return true unless action # pass through
 
     openInNewTab = @forceNewTab ||
@@ -183,6 +181,12 @@ class VomnibarUI
     if (action == "dismiss")
       @hide()
     else if action in [ "tab", "down" ]
+      # if action == "tab"
+      #   if @inputContainsASelectionRange()
+      #     window.getSelection().collapseToEnd()
+      #   else
+      #     action = "down"
+      # if action == "down"
       @selection += 1
       @selection = @initialSelectionValue if @selection == @completions.length
       @updateSelection()
@@ -192,26 +196,17 @@ class VomnibarUI
       @updateSelection()
     else if (action == "enter")
       if @selection == -1
-        switch @completer.name
-          when "omni"
-            return unless 0 < @getInputWithoutPromptedText().trim().length
-            # We ask the SearchEngineCompleter for its primary suggestion and launch it.  In some cases, this
-            # adds an extra (and not strictly necessary) round trip to the background completer.  However,
-            # this approach allows all of the various search-engine modes to be handled in a uniform way.
-            @fetchOnlyThePrimarySuggestion = true
-            @update true, =>
-              completion = @completions[0]
-              @hide -> completion?.performAction openInNewTab
-          else
-            # We're in "bookmark" or "tab" mode.
-            # If the user types something and hits enter without selecting a completion from the list, then try
-            # to open their query as a URL directly. If it doesn't look like a URL, then use the default search
-            # engine.
-            query = @getInputValueAsQuery()
-            @hide ->
-              chrome.runtime.sendMessage
-                handler: if openInNewTab then "openUrlInNewTab" else "openUrlInCurrentTab"
-                url: query
+        query = @input.value.trim()
+        return unless 0 < query.length
+        # If the user types something and hits enter without selecting a completion from the list, then:
+        #   - If a search URL has been provided, then use it.  This is custom search engine request.
+        #   - Otherwise, send the query to the background page, which will open it as a URL or create a
+        #     default search, as appropriate.
+        query = Utils.createSearchUrl query, @lastReponse.searchUrl if @lastReponse.searchUrl?
+        @hide ->
+          chrome.runtime.sendMessage
+            handler: if openInNewTab then "openUrlInNewTab" else "openUrlInCurrentTab"
+            url: query
       else
         completion = @completions[@selection]
         @hide -> completion.performAction openInNewTab
@@ -226,7 +221,6 @@ class VomnibarUI
         return true # Do not suppress event.
     else if action in [ "left", "right" ]
       [ start, end ] = [ @input.selectionStart, @input.selectionEnd ]
-      @previousLength = end
       if event.ctrlKey and not (event.altKey or event.metaKey)
         return true unless @inputContainsASelectionRange() and end == @input.value.length
         # "Control-Right" advances the start of the selection by a word.
@@ -280,10 +274,8 @@ class VomnibarUI
   updateCompletions: (callback = null) ->
     @completer.filter
       query: @getInputValueAsQuery()
-      fetchOnlyThePrimarySuggestion: @fetchOnlyThePrimarySuggestion
-      mayUseVomnibarCache: not @fetchOnlyThePrimarySuggestion
-      callback: (response) =>
-        { results, mayCacheResults } = response
+      callback: (@lastReponse) =>
+        { results } = @lastReponse
         @completions = results
         # Update completion list with the new suggestions.
         @completionList.innerHTML = @completions.map((completion) -> "<li>#{completion.html}</li>").join("")
@@ -291,7 +283,7 @@ class VomnibarUI
         @selection = Math.min @completions.length - 1, Math.max @initialSelectionValue, @selection
         @previousAutoSelect = null if @completions[0]?.autoSelect and @completions[0]?.forceAutoSelect
         @updateSelection()
-        @addPromptedText response
+        @addPromptedText()
         callback?()
 
   updateOnInput: =>
@@ -350,8 +342,6 @@ class VomnibarUI
 # Sends requests to a Vomnibox completer on the background page.
 #
 class BackgroundCompleter
-  debug: false
-
   # The "name" is the background-page completer to connect to: "omni", "tabs", or "bookmarks".
   constructor: (@name) ->
     @port = chrome.runtime.connect name: "completions"
@@ -364,49 +354,36 @@ class BackgroundCompleter
           @keywords = msg.keywords
           @lastUI.setKeywords @keywords
         when "completions"
-          # The result objects coming from the background page will be of the form:
-          #   { html: "", type: "", url: "", ... }
-          # Type will be one of [tab, bookmark, history, domain, search], or a custom search engine description.
-          for result in msg.results
-            extend result,
-              performAction:
-                if result.type == "tab"
-                  @completionActions.switchToTab result.tabId
-                else
-                  @completionActions.navigateToUrl result.url
-
-          # Cache the results, but only if we have been told it's ok to do so (it could be that more results
-          # will be posted shortly).  We cache the results even if they arrive late.
-          if msg.mayCacheResults
-            console.log "cache set:", "-#{msg.cacheKey}-" if @debug
-            @cache[msg.cacheKey] = msg
-          else
-            console.log "not setting cache:", "-#{msg.cacheKey}-" if @debug
-
-          # Handle the message, but only if it hasn't arrived too late.
-          @mostRecentCallback msg if msg.id == @messageId
+          if msg.id == @messageId
+            # The result objects coming from the background page will be of the form:
+            #   { html: "", type: "", url: "", ... }
+            # Type will be one of [tab, bookmark, history, domain, search], or a custom search engine description.
+            for result in msg.results
+              extend result,
+                performAction:
+                  if result.type == "tab"
+                    @completionActions.switchToTab result.tabId
+                  else
+                    @completionActions.navigateToUrl result.url
+
+            # Handle the message, but only if it hasn't arrived too late.
+            @mostRecentCallback msg
 
   filter: (request) ->
-    [ query, mayUseVomnibarCache, @mostRecentCallback ] = [ request.query, request.mayUseVomnibarCache,  request.callback ]
-    cacheKey = query.ltrim().split(/\s+/).join " "
-
-    if cacheKey of @cache and request.mayUseVomnibarCache
-      console.log "cache hit:", "-#{cacheKey}-" if @debug
-      @mostRecentCallback @cache[cacheKey]
-    else
-      console.log "cache miss:", "-#{cacheKey}-" if @debug
-      @port.postMessage extend request,
-        handler: "filter"
-        name: @name
-        id: @messageId = Utils.createUniqueId()
-        queryTerms: query.trim().split(/\s+/).filter (s) -> 0 < s.length
-        cacheKey: cacheKey
-        # We don't send these keys.
-        callback: null
-        mayUseVomnibarCache: null
+    { query, callback } = request
+    @mostRecentCallback = callback
+
+    @port.postMessage extend request,
+      handler: "filter"
+      name: @name
+      id: @messageId = Utils.createUniqueId()
+      queryTerms: query.trim().split(/\s+/).filter (s) -> 0 < s.length
+      # We don't send these keys.
+      callback: null
+      mayUseVomnibarCache: null
 
   reset: ->
-    [ @keywords, @cache ] = [ [], {} ]
+    @keywords = []
 
   refresh: (@lastUI) ->
     @reset()