Closed
Bug 918474
Opened 11 years ago
Closed 10 years ago
Default Search dropdown doesn't get refreshed after adding new search engine
Categories
(SeaMonkey :: Preferences, defect)
SeaMonkey
Preferences
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.30
People
(Reporter: mnyromyr, Assigned: ewong)
Details
Attachments
(3 files, 3 obsolete files)
3.20 KB,
patch
|
Details | Diff | Splinter Review | |
1.97 KB,
patch
|
ewong
:
review+
|
Details | Diff | Splinter Review |
1.02 KB,
patch
|
iannbugzilla
:
review+
|
Details | Diff | Splinter Review |
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 …
Assignee | ||
Comment 1•10 years ago
|
||
this is a modified patch from Philip Chee.
Assignee | ||
Comment 2•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8339962 -
Attachment is obsolete: true
Attachment #8450033 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8450033 -
Flags: review+
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
(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-
Assignee | ||
Comment 8•10 years ago
|
||
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+
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8459357 -
Attachment is obsolete: true
Attachment #8459377 -
Flags: review+
Assignee | ||
Comment 11•10 years ago
|
||
Pushed to comm-central: https://hg.mozilla.org/comm-central/rev/fa7f8f8f6c7e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
Comment 12•10 years ago
|
||
Minor nit:
> + while (menulist.hasChildNodes())
> + menulist.removeChild(menulist.lastChild);
You can now use lastChild.remove() directly instead of calling <element>.removeChild()
Comment 13•10 years ago
|
||
(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)
Assignee | ||
Comment 14•10 years ago
|
||
(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 → ---
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8459472 -
Flags: review?(iann_bugzilla)
Assignee | ||
Updated•10 years ago
|
Status: REOPENED → ASSIGNED
Attachment #8459472 -
Flags: review?(iann_bugzilla) → review+
Comment 16•10 years ago
|
||
Pushed: comm-central changeset 5b6e2c58ec0e http://hg.mozilla.org/comm-central/rev/5b6e2c58ec0e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•