Closed Bug 918474 Opened 6 years ago Closed 6 years ago

Default Search dropdown doesn't get refreshed after adding new search engine

Categories

(SeaMonkey :: Preferences, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: mnyromyr, Assigned: ewong)

Details

Attachments

(3 files, 3 obsolete files)

At least SM 2.19 up to fairly recent trunk (2013-08-28).

- Open preferences and go to "Browser" → "Internet Search".
- Hit the "Manage Search Engines" button. 
- Follow the "Get more search engines" link and add a new search engine, e.g. from <http://mycroftproject.com/search-engines.html?name=duckduckgo>.
- Close the "Manage Search Engine List" dialog by OK.
- Notice that the "Search using" dropdown does not include the new search engine. Only after closing and reopening preferences it will …
Attached patch proposed patch (v1) (obsolete) — Splinter Review
this is a modified patch from Philip Chee.
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #8339962 - Flags: review?(iann_bugzilla)
This was my attempt at this bug.  (For comparison at what I got and what I got with the help from RattyAway).
Comment on attachment 8339962 [details] [diff] [review]
proposed patch (v1)

> function Startup() {
>+  MakeList();
>+  Observer.init();
I would prefer it to be called something like SearchObserver.

>+  window.addEventListener("unload", Observer.destroy, false);
>+}
>+
>+var Observer = {
>+  init: function searchEngineListObserver_init() {
>+    Services.obs.addObserver(this, "browser-search-engine-modified", false);
>+  },
>+
>+  destroy: funciton searchEngineListObserver_destroy() {
typo here, should have been spotted if you had tested it. funciton -> function

>+      pref.updateElements();
>+  },
Comma isn't needed here.
>+}
Should have a semi-colon here.

r=me with those fixed.
Attachment #8339962 - Flags: review?(iann_bugzilla) → review+
Attached patch proposed patch (v2) (obsolete) — Splinter Review
Attachment #8339962 - Attachment is obsolete: true
Attachment #8450033 - Flags: review+
Attachment #8450033 - Flags: review+
Comment on attachment 8450033 [details] [diff] [review]
proposed patch (v2)

I'm getting this with patch #2:

Timestamp: 7/3/2014 3:55:20 PM
Error: NS_ERROR_XPC_BAD_CONVERT_JS: Could not convert JavaScript argument arg 0 [nsIObserverService.removeObserver]
Source File: chrome://communicator/content/pref/pref-search.js
Line: 19

this corresponds to this line:
 Services.obs.removeObserver(this, "browser-search-engine-modified");

What am I missing?
Attachment #8450033 - Flags: review?(iann_bugzilla)
(In reply to Edmund Wong from comment #5)
> What am I missing?

You're trying to add a member function as an event listener. The easiest solution is to add the object itself as the event listener, in which case the handleEvent method will automatically be invoked on it, at which point you can then remove it from the observer service.
Attachment #8450033 - Flags: review?(iann_bugzilla) → review-
Attached patch proposed patch (v3) (obsolete) — Splinter Review
Attachment #8450033 - Attachment is obsolete: true
Attachment #8459357 - Flags: review?(iann_bugzilla)
Comment on attachment 8459357 [details] [diff] [review]
proposed patch (v3)

>+  handleEvent: function searchEngineListObserver_handleEvent(aEvent) {
The function name could be searchEngineListEvent instead but either way r=me
Attachment #8459357 - Flags: review?(iann_bugzilla) → review+
Attachment #8459357 - Attachment is obsolete: true
Attachment #8459377 - Flags: review+
Pushed to comm-central:
https://hg.mozilla.org/comm-central/rev/fa7f8f8f6c7e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
Minor nit:
> +  while (menulist.hasChildNodes())
> +    menulist.removeChild(menulist.lastChild);
You can now use lastChild.remove() directly instead of calling <element>.removeChild()
(In reply to Philip Chee from comment #12)
> Minor nit:
> > +  while (menulist.hasChildNodes())
> > +    menulist.removeChild(menulist.lastChild);
> You can now use lastChild.remove() directly instead of calling
> <element>.removeChild()

I knew that rung a bell, can you do that as a follow up?
Flags: needinfo?(ewong)
(In reply to Ian Neal from comment #13)
> (In reply to Philip Chee from comment #12)
> > Minor nit:
> > > +  while (menulist.hasChildNodes())
> > > +    menulist.removeChild(menulist.lastChild);
> > You can now use lastChild.remove() directly instead of calling
> > <element>.removeChild()
> 
> I knew that rung a bell, can you do that as a follow up?

Sure thing!  I'll re-open this and do a followup patch.
Status: RESOLVED → REOPENED
Flags: needinfo?(ewong)
Resolution: FIXED → ---
Attachment #8459472 - Flags: review?(iann_bugzilla)
Status: REOPENED → ASSIGNED
Attachment #8459472 - Flags: review?(iann_bugzilla) → review+
Pushed: comm-central changeset 5b6e2c58ec0e
http://hg.mozilla.org/comm-central/rev/5b6e2c58ec0e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.