595235 - Make search suggestions work with new search sidebar

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Robert Kaiser]

The new search sidebar after bug 410613 is very primitive, but it should be easy to add search suggestion support.

Comment 1

[Robert Kaiser]

Attachment: WIP1: make something work there

I won't assign this to me yet but give everyone who wants to try a head start. This is what I have in my tree atm, and mainly a port from some related lines in the FF search bar to our sidebar, and I could get something in terms of search suggest to work, but I think it's not 100% yet, esp. that form history stuff sounds like it isn't there yet.

Comment 2

[Philip Chee]

Attachment: Patch v1.0 Do it.

> +  formHistory = Components.utils.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;
...
> +    formHistory.update(

Asynchronous FormHistory.jsm. I'll port Bug 697377 (Use Asynchronous FormHistory.jsm) in another bug, to avoid any bug/feature creep here.

> +      { op: "bump", fieldname: "searchbar-history", value: textValue },
> +      { handleError: function(aError) {Components.utils.reportError("Saving search to form history failed: " + aError.message);} }
> +    );

Needs better way to wrap the above lines.

> +      <textbox id="sidebar-search-text" flex="1" class="padded"
without "padded" it looks squashed vertically compared to the toolbarbutton next to it.


> +++ b/suite/installer/package-manifest.in
...
> +@BINPATH@/components/FormHistoryStartup.js
I assume mcsmurf is keeping an eye out for Installer changes. If so I'll remove this change from the patch here. In the meantime needed for this to work (I think).

Comment 3

[Philip Chee]

Comment on attachment 752282 [details] [diff] [review]
Patch v1.0 Do it.

> +  formHistory = Components.utils.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;
...
> +    formHistory.update(

Rrom asynchronous FormHistory.jsm. I'll port Bug 697377 (Use Asynchronous FormHistory.jsm) in another bug, to avoid any bug/feature creep here.

> +      { op: "bump", fieldname: "searchbar-history", value: textValue },
> +      { handleError: function(aError) {Components.utils.reportError("Saving search to form history failed: " + aError.message);} }
> +    );

Needs better way to wrap the above lines.

> +      <textbox id="sidebar-search-text" flex="1" class="padded"
without "padded" it looks squashed vertically compared to the toolbarbutton next to it.


> +++ b/suite/installer/package-manifest.in
...
> +@BINPATH@/components/FormHistoryStartup.js
I assume mcsmurf is keeping an eye out for Installer changes. If so I'll remove this change from the patch here. In the meantime needed for this to work (I think).

Comment 4

[[e-mail]]

Comment on attachment 752282 [details] [diff] [review]
Patch v1.0 Do it.

>+  formHistory = Components.utils.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;
I don't see the point of this. Just load FormHistory.jsm at the top level.

> function doSearch() {
>   var textValue = textbox.value;
>+  if (!textValue)
>+    return;
Search bar allows an empty text value (attempts to open the search page).

>+  var isPB = window.top
Nit: don't need top, usePrivateBrowsing should be the same on all frames.

>+    formHistory.update(
>+      { op: "bump", fieldname: "searchbar-history", value: textValue },
[Whoa, this is gross. Took me ages to work out how it worked.]

>+      { handleError: function(aError) {Components.utils.reportError("Saving search to form history failed: " + aError.message);} }
Nit: badly formatted.

>+               autocompletepopup="PopupAutoComplete"
Does this actually work, or are we getting xpfe autocomplete here, which doesn't need it?

>+               autocompletesearchparam="searchbar-history"
Search param depends on whether window is private.

No showCommentColumn? (Might be for search suggestions, I forget.)

Comment 5

[[e-mail]]

Comment on attachment 752282 [details] [diff] [review]
Patch v1.0 Do it.

[None of my trees have FormHistory.jsm yet]

Nit: This patch has DOS line endings.

>+  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIObserver,
>+                                         Components.interfaces.nsISupportsWeakReference]);
SyntaxError: missing } after property list
[Might be easier to put QueryInterface first?]

Comment 6

[Philip Chee]

Attachment: Patch v1.1 fix review issues.

Changes in this patch:
Added CSS to draw a separator line between the form history entries and the search suggestions.

>>+  formHistory = Components.utils.import("resource://gre/modules/FormHistory.jsm", {}).FormHistory;
> I don't see the point of this. Just load FormHistory.jsm at the top level.
Fixed.

>> function doSearch() {
>>   var textValue = textbox.value;
>>+  if (!textValue)
>>+    return;
> Search bar allows an empty text value (attempts to open the search page).
Removed early return.

>>+  var isPB = window.top
> Nit: don't need top, usePrivateBrowsing should be the same on all frames.
Fixed.

>>+    formHistory.update(
>>+      { op: "bump", fieldname: "searchbar-history", value: textValue },
> [Whoa, this is gross. Took me ages to work out how it worked.]
> 
>>+      { handleError: function(aError) {Components.utils.reportError("Saving search to form history failed: " + aError.message);} }
> Nit: badly formatted.
Fixed.

>>+               autocompletepopup="PopupAutoComplete"
> Does this actually work, or are we getting xpfe autocomplete here, which doesn't need it?
Removed as it doesn't seem to do anything.

>>+               autocompletesearchparam="searchbar-history"
> Search param depends on whether window is private.
Fixed.

> No showCommentColumn? (Might be for search suggestions, I forget.)
With showCommentColumn all the drop down entries are cropped.... due to the default width of the sidebar.

> Nit: This patch has DOS line endings.
Fixed.

>>+  QueryInterface: XPCOMUtils.generateQI([Components.interfaces.nsIObserver,
>>+                                         Components.interfaces.nsISupportsWeakReference]);
> SyntaxError: missing } after property list
Fixed.

> [Might be easier to put QueryInterface first?]
Fixed.

Comment 7

[[e-mail]]

Comment on attachment 753704 [details] [diff] [review]
Patch v1.1 fix review issues.

>+  isPB = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
>+               .getInterface(Components.interfaces.nsIWebNavigation)
>+               .QueryInterface(Components.interfaces.nsILoadContext)
>+               .usePrivateBrowsing;
>+  if (isPB)
[It occurs to me that we could just use if (top.gPrivate) instead.]

>                onkeypress="if (event.keyCode == event.DOM_VK_RETURN) doSearch();"
This should use ontextentered instead, so that when you click on a suggestion it automatically searches, like the search bar does. r=me with that fixed.

Comment 8

[Philip Chee]

Attachment: Patch v1.2 patch for check-in carrying forward r=Neil

Comment 9

[Philip Chee]

Attachment: Patch v1.2a patch for check-in carrying forward r=Neil

/sigh/ fix typo. s/event/eventParam/

Comment 10

[Philip Chee]

Pushed http://hg.mozilla.org/comm-central/rev/8919c64f2bb2