Make search suggestions work with new search sidebar

RESOLVED FIXED in seamonkey2.21

Status

SeaMonkey
Search
RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: Robert Kaiser, Assigned: Philip Chee)

Tracking

Trunk
seamonkey2.21
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

8 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 479812 [details] [diff] [review]
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.

Updated

5 years ago
Depends on: 861691
(Assignee)

Comment 2

5 years ago
Created attachment 752282 [details] [diff] [review]
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).
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
(Assignee)

Comment 3

5 years ago
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 4

5 years ago
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

5 years ago
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-
(Assignee)

Comment 6

5 years ago
Created attachment 753704 [details] [diff] [review]
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.
Attachment #752282 - Attachment is obsolete: true
Attachment #753704 - Flags: review?(neil)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
Created attachment 754139 [details] [diff] [review]
Patch v1.2 patch for check-in carrying forward r=Neil
Attachment #754139 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 754230 [details] [diff] [review]
Patch v1.2a patch for check-in carrying forward r=Neil

/sigh/ fix typo. s/event/eventParam/
Attachment #753704 - Attachment is obsolete: true
Attachment #754139 - Attachment is obsolete: true
Attachment #754230 - Flags: review+
(Assignee)

Comment 10

5 years ago
Pushed http://hg.mozilla.org/comm-central/rev/8919c64f2bb2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.21
You need to log in before you can comment on or make changes to this bug.