Closed Bug 595235 Opened 9 years ago Closed 6 years ago

Make search suggestions work with new search sidebar

Categories

(SeaMonkey :: Search, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.21

People

(Reporter: kairo, Assigned: philip.chee)

References

Details

Attachments

(2 files, 3 obsolete files)

The new search sidebar after bug 410613 is very primitive, but it should be easy to add search suggestion support.
OS: Linux → All
Hardware: x86 → All
Version: SeaMonkey 2.0 Branch → Trunk
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.
Depends on: 861691
Attached patch Patch v1.0 Do it. (obsolete) — Splinter Review
> +  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).
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
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).
Attachment #752282 - Flags: review?(neil)
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 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?]
Attachment #752282 - Flags: review?(neil) → review-
Attached patch Patch v1.1 fix review issues. (obsolete) — Splinter Review
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.
Attachment #752282 - Attachment is obsolete: true
Attachment #753704 - Flags: review?(neil)
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.
Attachment #753704 - Flags: review?(neil) → review+
Attachment #754139 - Flags: review+
/sigh/ fix typo. s/event/eventParam/
Attachment #753704 - Attachment is obsolete: true
Attachment #754139 - Attachment is obsolete: true
Attachment #754230 - Flags: review+
Pushed http://hg.mozilla.org/comm-central/rev/8919c64f2bb2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.21
You need to log in before you can comment on or make changes to this bug.