Closed Bug 1119872 Opened 9 years ago Closed 8 years ago
"restore default engines" button should also reset the selected engine to the default
See bug 1113692 comment 1. It's generally confusing to have this button only re-add the default engines, and not also reset your selected engine to the default.
Whiteboard: [good first bug]
Hey, I would love to work on this bug It will take until tomorrow for me to really kick off work on it, and it's my first bug, so I'd really appreciate a little guidance on it.
I'm guessing the file I would want to change would be C:\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dist\bin\browser\chrome\browser\content\browser\search\engineManager.js , correct? Specifically adding code to the restoreDefaultEngines function. What is the actual function that sets an engine as the one being used in the search bar though, and what parameters does it need passed?
Hey Jacob - thanks for looking in to this. And sorry for the slow response. You're right that engineManager.js is the right place to look. Specifically, its "restoreDefaultEngines" method: https://hg.mozilla.org/mozilla-central/annotate/eab4a81e4457/browser/components/search/content/engineManager.js#l340 Looking into this a bit further, though, and it looks like I may have been wrong in marking this as a "good first bug", because changing that function to reset the default engine is actually a little bit involved. Given that you're looking for a first bug, perhaps you'd be interested in taking another bug instead? Perhaps bug 510516? Otherwise, I can walk you through the changes needed here, but multiple files would need changing and we may need a few cycles of feedback before getting a final patch - up to you!
Whiteboard: [good first bug]
Priority: -- → P3
Florian, can you take over mentorship of this bug?
You need to edit the restoreDefaultEngines function in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.js Please ignore the engineManager.js file, it's part of the old search UI that we will remove soon. Conveniently, a resetToOriginalDefaultEngine method has recently been added on the search service (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIBrowserSearchService.idl?rev=d544be8b3dc3#284). You'll want to call it with something like: Services.search.resetToOriginalDefaultEngine();
Whiteboard: [fxsearch][partnerengine] → [partnerengine][fxsearch]
Sorry if it takes me a bit to get back with you on this bug. Possibly a few more days, I'm graduating High School soon so things are a little crazy. Hopefully over the weekend I'll have time to take a closer look at it. Regardless, thanks for the info so far Florian!
Hi, I would like to take a look at this bug. Thanks
I added in the change that you proposed with the resetToOriginalDefaultEngine() function. This changes the search engine back to the default however, it did not show the change in the dropdown menu. This was because in the buildDefaultEngineDropDown function, the dropdown menu defaulted to show what was previously selected and only show the current default if no search engine was selected in the menu. I believe that this is pointless as the current default engine is always updated whenever a new engine is selected in the dropdown menu, so there is never a case where you would want to use the selected engine over the actual current default engine. Even if in theory there was a case where the selected engine should be picked over the currentEngine and the selected engine is not equal to the currentEngine, this would not be good since the selected engine in the dropdown menu would not properly reflect the actual search engine that is being used, so this should never be the case. I therefore removed the portion in buildDefaultEngineDropDown() where it first attempts to set the current engine to the one that is selected in the dropdown menu. This fixes my problem and shouldn't change any of the previous functionality in any way.
Attachment #8685762 - Flags: review?(florian)
https://hg.mozilla.org/integration/fx-team/rev/701fb57ec4c7afea9dae110aee653f6d457e9bb1 Bug 1119872 - Changed "Restore default engines" button action to also reset the selected engine to the default engine, r=florian.
Comment on attachment 8685762 [details] [diff] [review] bug1119872.patch Sorry for the delay in reviewing this, I was off last week. Thanks for the patch, it looks good to me. (In reply to Jeremy Francispillai from comment #9) > in the buildDefaultEngineDropDown function, the dropdown > menu defaulted to show what was previously selected and only show the > current default if no search engine was selected in the menu. I believe that > this is pointless as the current default engine is always updated whenever a > new engine is selected in the dropdown menu, so there is never a case where > you would want to use the selected engine over the actual current default > engine. In case you are curious about the reason why the code was this way: I think it's a leftover from when we had preferences in a separate dialog that had Apply/OK/Cancel buttons. Now that all changes apply immediately, it's indeed pointless and we can remove it. ># HG changeset patch ># User Jeremy Francispillai <email@example.com> ># Parent e2a910c048dc82fc3be53475f18e7f81f03e377b >Changed "Restore default engines" button action to also reset the selected engine to the default engine. In the future, could you please make the commit message follow this format? Bug NNNNNNN - <message>, r=<reviewer>
Attachment #8685762 - Flags: review?(florian) → review+
As comment 10 shows, I pushed the patch to the fx-team repository; assuming everything goes fine, it should be merged to mozilla-central within about a day, and then be in the next Nightly. If you would be interested in working on another bug in the same code area, bug 1122618 is related. If you would like to try touching a different area of the code, I can suggest bug 1170193.
Assignee: nobody → jeremyjpf
Status: NEW → ASSIGNED
Verified fixed on latest Nightly 45.0a1 (buildID: 20151118030232).
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0 Build ID: 20151120030227 If the restored default engine is other than Google the default engine is restored only after restarting Firefox. Used the following scenario: 1. Launch Firefox 2. Go to about:preferences#search and set Amazon as the default engine 3. In the One-click search engines list -> select Amazon -> click the "Remove" button 4. Click on the "Restore Default Search Engines" button Expected Results: Amazon is placed back in the "One Click search engines" list and is set back as the Default engine. Actual Results: Amazon is placed back in the "One Click search engines" list but is not set back as the Default engine (Google remains the default engine). It's set as the default engine only after restarting Firefox. Should I file a new bug to cover this scenario or Re-open this one?
(In reply to Simona B from comment #15) > Expected Results: > Amazon is placed back in the "One Click search engines" list and is set back > as the Default engine. The expected result is that Google should stay the default engine, even after a restart. > Actual Results: > Amazon is placed back in the "One Click search engines" list but is not set > back as the Default engine (Google remains the default engine). This is as expected. > [Amazon i]s set as the default engine only after restarting Firefox. This is unexpected, please file a bug covering this. The actual fix may turn out to the be same as for bug 1165341, but having a possibly duplicate bug won't hurt; forgetting this would. Thanks for catching this! :)
(In reply to Florian Quèze [:florian] [:flo] from comment #16) > (In reply to Simona B from comment #15) > > [Amazon i]s set as the default engine only after restarting Firefox. > > This is unexpected, please file a bug covering this. This was filed as bug 1227045.
Depends on: 1227045
You need to log in before you can comment on or make changes to this bug.