Merge branch 'completion-on-custom-search-only' into completion-on-custom-search-only-merge

Conflicts:
	background_scripts/completion.coffee

6 files changed, 91 insertions, 102 deletions

diff --git a/background_scripts/completion.coffee b/background_scripts/completion.coffee
index 02c5478a..bae73b8d 100644
--- a/background_scripts/completion.coffee
+++ b/background_scripts/completion.coffee
@@ -43,23 +43,33 @@ class Suggestion
     # or @relevancyFunction.
     @relevancy ?= @relevancyFunction this
 
-  generateHtml: ->
+  generateHtml: (request) ->
     return @html if @html
     relevancyHtml = if @showRelevancy then "<span class='relevancy'>#{@computeRelevancy()}</span>" else ""
     insertTextClass = if @insertText then "vomnibarInsertText" else "vomnibarNoInsertText"
     insertTextIndicator = "&#8618;" # A right hooked arrow.
+    @title = @insertText if @insertText and request.isCustomSearch
     # NOTE(philc): We're using these vimium-specific class names so we don't collide with the page's CSS.
     @html =
-      """
-      <div class="vimiumReset vomnibarTopHalf">
-         <span class="vimiumReset vomnibarSource #{insertTextClass}">#{insertTextIndicator}</span><span class="vimiumReset vomnibarSource">#{@type}</span>
-         <span class="vimiumReset vomnibarTitle">#{@highlightQueryTerms Utils.escapeHtml @title}</span>
-       </div>
-       <div class="vimiumReset vomnibarBottomHalf">
-        <span class="vimiumReset vomnibarSource vomnibarNoInsertText">#{insertTextIndicator}</span><span class="vimiumReset vomnibarUrl">#{@highlightUrlTerms Utils.escapeHtml @shortenUrl()}</span>
-        #{relevancyHtml}
-      </div>
-      """
+      if request.isCustomSearch
+        """
+        <div class="vimiumReset vomnibarTopHalf">
+           <span class="vimiumReset vomnibarSource #{insertTextClass}">#{insertTextIndicator}</span><span class="vimiumReset vomnibarSource">#{@type}</span>
+           <span class="vimiumReset vomnibarTitle">#{@highlightQueryTerms Utils.escapeHtml @title}</span>
+           #{relevancyHtml}
+         </div>
+        """
+      else
+        """
+        <div class="vimiumReset vomnibarTopHalf">
+           <span class="vimiumReset vomnibarSource #{insertTextClass}">#{insertTextIndicator}</span><span class="vimiumReset vomnibarSource">#{@type}</span>
+           <span class="vimiumReset vomnibarTitle">#{@highlightQueryTerms Utils.escapeHtml @title}</span>
+         </div>
+         <div class="vimiumReset vomnibarBottomHalf">
+          <span class="vimiumReset vomnibarSource vomnibarNoInsertText">#{insertTextIndicator}</span><span class="vimiumReset vomnibarUrl">#{@highlightUrlTerms Utils.escapeHtml @shortenUrl()}</span>
+          #{relevancyHtml}
+        </div>
+        """
 
   # Use neat trick to snatch a domain (http://stackoverflow.com/a/8498668).
   getUrlRoot: (url) ->
@@ -293,7 +303,7 @@ class DomainCompleter
         queryTerms: queryTerms
         type: "domain"
         url: domains[0]?[0] ? "" # This is the URL or an empty string, but not null.
-        relevancy: 1
+        relevancy: 2.0
       ].filter (s) -> 0 < s.url.length
 
   # Returns a list of domains of the form: [ [domain, relevancy], ... ]
@@ -414,7 +424,7 @@ class SearchEngineCompleter
   preprocessRequest: (request) ->
     @searchEngines.use (engines) =>
       { queryTerms, query } = request
-      request.searchEngines = engines
+      extend request, searchEngines: engines, keywords: key for own key of engines
       keyword = queryTerms[0]
       # Note. For a keyword "w", we match "w search terms" and "w ", but not "w" on its own.
       if keyword and engines[keyword] and (1 < queryTerms.length or /\S\s/.test query)
@@ -422,6 +432,7 @@ class SearchEngineCompleter
           queryTerms: queryTerms[1..]
           keyword: keyword
           engine: engines[keyword]
+          isCustomSearch: true
 
   refresh: (port) ->
     @previousSuggestions = {}
@@ -454,92 +465,77 @@ class SearchEngineCompleter
 
   filter: (request, onComplete) ->
     { queryTerms, query, engine } = request
+    return onComplete [] unless engine
 
-    { custom, searchUrl, description } =
-      if engine
-        { keyword, searchUrl, description } = engine
-        extend request, { searchUrl, customSearchMode: true }
-        custom: true
-        searchUrl: searchUrl
-        description: description
-      else
-        custom: false
-        searchUrl: Settings.get "searchUrl"
-        description: "search"
-
-    return onComplete [] unless custom or 0 < queryTerms.length
+    { keyword, searchUrl, description } = engine
+    extend request, searchUrl, customSearchMode: true
 
-    factor = Math.max 0.0, Math.min 1.0, Settings.get "omniSearchWeight"
-    haveCompletionEngine = (0.0 < factor or custom) and CompletionSearch.haveCompletionEngine searchUrl
+    factor = 0.5
+    haveCompletionEngine = CompletionSearch.haveCompletionEngine searchUrl
 
     # This filter is applied to all of the suggestions from all of the completers, after they have been
     # aggregated by the MultiCompleter.
     filter = (suggestions) ->
-      if custom and haveCompletionEngine
-        # We only accept suggestions:
-        #   - from this completer, or
-        #   - from other completers, but then only if their URL matches this search engine and matches 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..])
-
-      else if not custom
-        # Filter out any suggestion which is just what the user would get if they hit <Enter> anyway.  For
-        # example, don't offer "https://www.google.com/search?q=vimium" if the query is "vimium".
-        defaultUrl = Utils.createSearchUrl queryTerms, searchUrl
-        defaultQuery = queryTerms.join " "
-        suggestions.filter (suggestion) -> Utils.extractQuery(searchUrl, suggestion.url) != defaultQuery
-      else
-        suggestions
+      suggestions.filter (suggestion) ->
+        # We only keep suggestions which either *were* generated by this search engine, or *could have
+        # been* generated by this search engine (and match the current query).
+        suggestion.isSearchSuggestion or suggestion.isCustomSearch or
+          (
+            terms = Utils.extractQuery searchUrl, suggestion.url
+            terms and RankingUtils.matches queryTerms, terms
+          )
 
     # If a previous suggestion still matches the query, then we keep it (even if the completion engine may not
     # return it for the current query).  This allows the user to pick suggestions by typing fragments of their
     # text, without regard to whether the completion engine can complete the actual text of the query.
     previousSuggestions =
-      for url, suggestion of @previousSuggestions
-        continue unless RankingUtils.matches queryTerms, suggestion.title
-        # Reset various fields, they may not be correct wrt. the current query.
-        extend suggestion, relevancy: null, html: null, queryTerms: queryTerms
-        suggestion.relevancy = null
-        suggestion
+      if queryTerms.length == 0
+        []
+      else
+        for url, suggestion of @previousSuggestions
+          continue unless RankingUtils.matches queryTerms, suggestion.title
+          # Reset various fields, they may not be correct wrt. the current query.
+          extend suggestion, relevancy: null, html: null, queryTerms: queryTerms
+          suggestion.relevancy = null
+          suggestion
 
     primarySuggestion = new Suggestion
       queryTerms: queryTerms
       type: description
       url: Utils.createSearchUrl queryTerms, searchUrl
       title: queryTerms.join " "
-      relevancy: 1
-      autoSelect: custom
-      highlightTerms: not haveCompletionEngine
+      relevancy: 2.0
+      autoSelect: true
+      highlightTerms: false
       isSearchSuggestion: true
+      isPrimarySuggestion: true
 
-    mkSuggestion = (suggestion) =>
-      url = Utils.createSearchUrl suggestion, searchUrl
-      @previousSuggestions[url] = new Suggestion
-        queryTerms: queryTerms
-        type: description
-        url: url
-        title: suggestion
-        insertText: suggestion
-        highlightTerms: false
-        highlightTermsExcludeUrl: true
-        isCustomSearch: custom
-        relevancyFunction: @computeRelevancy
-        relevancyData: factor
+    return onComplete [ primarySuggestion ], { filter } if queryTerms.length == 0
+
+    mkSuggestion = do =>
+      count = 0
+      (suggestion) =>
+        url = Utils.createSearchUrl suggestion, searchUrl
+        @previousSuggestions[url] = new Suggestion
+          queryTerms: queryTerms
+          type: description
+          url: url
+          title: suggestion
+          insertText: suggestion
+          highlightTerms: false
+          highlightTermsExcludeUrl: true
+          isCustomSearch: true
+          relevancy: if ++count == 1 then 1.0 else null
+          relevancyFunction: @computeRelevancy
 
     cachedSuggestions =
       if haveCompletionEngine then CompletionSearch.complete searchUrl, queryTerms else null
 
     suggestions = previousSuggestions
-    suggestions.push primarySuggestion if custom
-    suggestions.push cachedSuggestions.map(mkSuggestion)... if custom and cachedSuggestions?
+    suggestions.push primarySuggestion
 
     if queryTerms.length == 0 or cachedSuggestions? or not haveCompletionEngine
-      # There is no prospect of adding further completions.
+      # There is no prospect of adding further completions, so we're done.
       suggestions.push cachedSuggestions.map(mkSuggestion)... if cachedSuggestions?
       onComplete suggestions, { filter, continuation: null }
     else
@@ -547,20 +543,7 @@ class SearchEngineCompleter
       # continuation.
       onComplete suggestions,
         filter: filter
-        continuation: (suggestions, onComplete) =>
-
-          # We can skip querying the completion engine if any new suggestions we propose will not score highly
-          # enough to make the list anyway.  We construct a suggestion which perfectly matches the query, and
-          # ask the relevancy function what score it would get.  If that score is less than the score of the
-          # lowest-ranked suggestion from another completer (and there are already 10 suggestions), then
-          # there's no need to query the completion engine.
-          perfectRelevancyScore = @computeRelevancy new Suggestion
-            queryTerms: queryTerms, title: queryTerms.join(" "), relevancyData: factor
-
-          if 10 <= suggestions.length and perfectRelevancyScore < suggestions[suggestions.length-1].relevancy
-            console.log "skip (cannot make the grade):", suggestions.length, query if SearchEngineCompleter.debug
-            return onComplete []
-
+        continuation: (onComplete) =>
           CompletionSearch.complete searchUrl, queryTerms, (suggestions = []) =>
             console.log "fetched suggestions:", suggestions.length, query if SearchEngineCompleter.debug
             onComplete suggestions.map mkSuggestion
@@ -571,7 +554,7 @@ class SearchEngineCompleter
     #   scores here, and those provided by other completers.
     # - Relevancy depends only on the title (which is the search terms), and not on the URL.
     Suggestion.boostRelevancyScore 0.5,
-      relevancyData * RankingUtils.wordRelevancy queryTerms, title, title
+      0.7 * RankingUtils.wordRelevancy queryTerms, title, title
 
   postProcessSuggestions: (request, suggestions) ->
     return unless request.searchEngines
@@ -586,9 +569,6 @@ class SearchEngineCompleter
             # suggestion, then custom search-engine mode should be activated.
             suggestion.customSearchMode = engine.keyword
             suggestion.title ||= suggestion.insertText
-            # NOTE(smblott) The following is disabled: experimentation with UI.
-            # suggestion.highlightTermsExcludeUrl = true
-            # suggestion.type = engine.description ? "custom search history"
             break
 
 # A completer which calls filter() on many completers, aggregates the results, ranks them, and returns the top
@@ -643,7 +623,7 @@ class MultiCompleter
       if shouldRunContinuations
         jobs = new JobRunner continuations.map (continuation) ->
           (callback) ->
-            continuation suggestions, (newSuggestions) ->
+            continuation (newSuggestions) ->
               suggestions.push newSuggestions...
               callback()
 
@@ -676,7 +656,7 @@ class MultiCompleter
     completer.postProcessSuggestions? request, suggestions for completer in @completers
 
     # Generate HTML for the remaining suggestions and return them.
-    suggestion.generateHtml() for suggestion in suggestions
+    suggestion.generateHtml request for suggestion in suggestions
     suggestions
 
 # Utilities which help us compute a relevancy score for a given item.
diff --git a/background_scripts/settings.coffee b/background_scripts/settings.coffee
index 269b4a2c..d23649ee 100644
--- a/background_scripts/settings.coffee
+++ b/background_scripts/settings.coffee
@@ -43,7 +43,6 @@ root.Settings = Settings =
   # or strings
   defaults:
     scrollStepSize: 60
-    omniSearchWeight: 0.4
     smoothScroll: true
     keyMappings: "# Insert your preferred key mappings here."
     linkHintCharacters: "sadfjklewcmpgh"
diff --git a/pages/options.html b/pages/options.html
index 8535823d..0fa5b18d 100644
--- a/pages/options.html
+++ b/pages/options.html
@@ -238,6 +238,8 @@ b: http://b.com/?q=%s description
           </tr>
 
           <!-- Vimium Labs -->
+          <!--
+          Disabled.  But we leave this code here as a template for the next time we need to introduce "Vimium Labs".
           <tr>
             <td colspan="2"><header>Vimium Labs</header></td>
           </tr>
@@ -263,6 +265,7 @@ b: http://b.com/?q=%s description
               <input id="omniSearchWeight" type="number" min="0.0" max="1.0" step="0.05" />(0 to 1)
             </td>
           </tr>
+          -->
         </tbody>
       </table>
     </div>
diff --git a/pages/vomnibar.coffee b/pages/vomnibar.coffee
index 423ffa59..71f22900 100644
--- a/pages/vomnibar.coffee
+++ b/pages/vomnibar.coffee
@@ -76,7 +76,7 @@ class VomnibarUI
   updateSelection: ->
     # For custom search engines, we suppress the leading term (e.g. the "w" of "w query terms") within the
     # vomnibar input.
-    if @lastReponse.customSearchMode and not @customSearchMode?
+    if @lastReponse.isCustomSearch and not @customSearchMode?
       queryTerms = @input.value.trim().split /\s+/
       @customSearchMode = queryTerms[0]
       @input.value = queryTerms[1..].join " "
@@ -141,15 +141,23 @@ class VomnibarUI
       @selection = @completions.length - 1 if @selection < @initialSelectionValue
       @updateSelection()
     else if (action == "enter")
-      if @selection == -1
+      isCustomSearchPrimarySuggestion = @completions[@selection]?.isPrimarySuggestion and @lastReponse.engine?.searchUrl?
+      if @selection == -1 or isCustomSearchPrimarySuggestion
         query = @input.value.trim()
         # <Enter> on an empty query is a no-op.
         return unless 0 < query.length
+        # First case (@selection == -1).
         # 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?
+        #
+        # Second case (isCustomSearchPrimarySuggestion).
+        # Alternatively, the selected completion could be the primary selection for a custom search engine.
+        # Because the the suggestions are updated asynchronously in omni mode, the user may have typed more
+        # text than that which is included in the URL associated with the primary suggestion.  Therefore, to
+        # avoid a race condition, we construct the query from the actual contents of the input (query).
+        query = Utils.createSearchUrl query, @lastReponse.engine.searchUrl if isCustomSearchPrimarySuggestion
         @hide ->
           chrome.runtime.sendMessage
             handler: if openInNewTab then "openUrlInNewTab" else "openUrlInCurrentTab"
diff --git a/pages/vomnibar.css b/pages/vomnibar.css
index b1ed0252..1b19daad 100644
--- a/pages/vomnibar.css
+++ b/pages/vomnibar.css
@@ -126,7 +126,6 @@
 #vomnibar li em { font-style: italic; }
 #vomnibar li em .vomnibarMatch, #vomnibar li .vomnibarTitle .vomnibarMatch {
   color: #333;
-  text-decoration: underline;
 }
 
 #vomnibar li.vomnibarSelected {
diff --git a/tests/unit_tests/completion_test.coffee b/tests/unit_tests/completion_test.coffee
index 88df0a43..4a0cf746 100644
--- a/tests/unit_tests/completion_test.coffee
+++ b/tests/unit_tests/completion_test.coffee
@@ -244,7 +244,7 @@ context "suggestions",
       url: "url"
       title: "title <span>"
       relevancyFunction: returns 1
-    assert.isTrue suggestion.generateHtml().indexOf("title &lt;span&gt;") >= 0
+    assert.isTrue suggestion.generateHtml({}).indexOf("title &lt;span&gt;") >= 0
 
   should "highlight query words", ->
     suggestion = new Suggestion
@@ -254,7 +254,7 @@ context "suggestions",
       title: "ninjawords"
       relevancyFunction: returns 1
     expected = "<span class='vomnibarMatch'>ninj</span>a<span class='vomnibarMatch'>words</span>"
-    assert.isTrue suggestion.generateHtml().indexOf(expected) >= 0
+    assert.isTrue suggestion.generateHtml({}).indexOf(expected) >= 0
 
   should "highlight query words correctly when whey they overlap", ->
     suggestion = new Suggestion
@@ -264,7 +264,7 @@ context "suggestions",
       title: "ninjawords"
       relevancyFunction: returns 1
     expected = "<span class='vomnibarMatch'>ninjaword</span>s"
-    assert.isTrue suggestion.generateHtml().indexOf(expected) >= 0
+    assert.isTrue suggestion.generateHtml({}).indexOf(expected) >= 0
 
   should "shorten urls", ->
     suggestion = new Suggestion
@@ -273,7 +273,7 @@ context "suggestions",
       url: "http://ninjawords.com"
       title: "ninjawords"
       relevancyFunction: returns 1
-    assert.equal -1, suggestion.generateHtml().indexOf("http://ninjawords.com")
+    assert.equal -1, suggestion.generateHtml({}).indexOf("http://ninjawords.com")
 
 context "RankingUtils.wordRelevancy",
   should "score higher in shorter URLs", ->