Closed
Bug 595235
Opened 14 years ago
Closed 12 years ago
Make search suggestions work with new search sidebar
Categories
(SeaMonkey :: Search, defect)
SeaMonkey
Search
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.21
People
(Reporter: kairo, Assigned: philip.chee)
References
Details
Attachments
(2 files, 3 obsolete files)
2.92 KB,
patch
|
Details | Diff | Splinter Review | |
7.57 KB,
patch
|
philip.chee
:
review+
|
Details | Diff | Splinter Review |
The new search sidebar after bug 410613 is very primitive, but it should be easy to add search suggestion support.
Updated•14 years ago
|
OS: Linux → All
Hardware: x86 → All
Version: SeaMonkey 2.0 Branch → Trunk
Reporter | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•12 years ago
|
||
> + 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•12 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•12 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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
Attachment #754139 -
Flags: review+
Assignee | ||
Comment 9•12 years ago
|
||
/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•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.21
You need to log in
before you can comment on or make changes to this bug.
Description
•