Default search engine drop down in preferences isn't updated when something else changes the default search engine

VERIFIED FIXED in Firefox 46

Status

()

P3
normal
Rank:
35
VERIFIED FIXED
4 years ago
2 years ago

People

(Reporter: petruta.rasa, Assigned: jeremyjpf, Mentored)

Tracking

Trunk
Firefox 46
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox46 verified)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment, 3 obsolete attachments)

Having about:preferences#search page opened and then switching the default search engine using Ctrl / Cmd + up/down keyboard shortcuts doesn't show the selected search engine unless refreshing the page.

Reproduces on all branches, all platforms.
This also reproduces when adding a search engine from https://addons.mozilla.org/en-US/firefox/search/?atype=4 and set it as default.

Updated

3 years ago
Duplicate of this bug: 1192777
Priority: -- → P3
Summary: about:preferences#search is not updated if the search engine is modified using Ctrl+up/down → Default search engine drop down in preferences isn't updated when something else changes the default search engine
Whiteboard: [fxsearch]

Updated

3 years ago
See Also: → bug 1207625

Updated

3 years ago
Rank: 35
Mentor: florian
(Assignee)

Comment 3

3 years ago
Hi, I would like to look at this bug. Thanks
(Assignee)

Comment 4

3 years ago
Created attachment 8690576 [details] [diff] [review]
bug1122618.patch

It took a while to figure out how to update the page whenever the search engine changed, but after I found it, it was a simple one line addition.
Attachment #8690576 - Flags: review?(florian)
Comment on attachment 8690576 [details] [diff] [review]
bug1122618.patch

Review of attachment 8690576 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

::: browser/components/preferences/in-content/search.js
@@ +180,5 @@
>          gEngineView.invalidate();
>          break;
>        case "engine-removed":
>        case "engine-current":
> +        gSearchPane.buildDefaultEngineDropDown();

This will execute this line for both the "engine-current" and the "engine-removed" notifications. Is this intended? For the "engine-removed" case, if the user clicked the 'remove' button from the search preferences, buildDefaultEngineDropDown will have already been called from the removeEngine method. If the engine was removed from somewhere else (eg. by an add-on, or by the search preferences opened in another window), calling buildDefaultEngineDropDown isn't enough because the engine hasn't been removed from gEngineView._engineStore.

There are 2 possible ways forward here:
- either you want to go for the smallest patch that fixes this bug as described in comment 0, in which case the patch should be adapted so that buildDefaultEngineDropDown is called only for the engine-current notification.
- or you want to further cleanup/fix the search.js code, and separate the code actually performing changes from the code displaying changes. Currently the two are entangled, but the code would be significantly simpler if changes were only displayed when receiving notifications. If you want to work on this, the search way to test is to open 'about:preferences#search' in 2 different windows, make changes to the preferences in one window and check that both windows keep displaying the same thing at all times.
Attachment #8690576 - Flags: review?(florian) → feedback+
Assignee: nobody → jeremyjpf
(Assignee)

Comment 6

3 years ago
Hi Florian, I will take a closer look at this in the next few days when I have time.
(Assignee)

Comment 7

3 years ago
Created attachment 8696306 [details] [diff] [review]
bug1122618.txt

Hi i had some time today and I changed around the code to get rid of the above problem that you specified where I was also calling buildDefaultEngineDropDown when an engine was removed. I also changed the code responsible for the removal of an engine so that removing the actual engine is separated from displaying the changes. The code for making the change and displaying the change is also separated for changing the current default engine as well as for adding a new engine. I didn't separate the two operations for the cases where engines are moved around (change engine order by drag and drop, or reordering engines when "restore default engines" is clicked) because I cannot think of a good way to update the display once an engine has been  moved. One possible idea that I had for this was to call the Services.search.getVisibleEngines() function and then compare the position of the moved engine in the returned list compared to the displayed list. I didn't implement this because I do not think that it is very efficient and I don't know if there is a better way. Let me know if there is a good way to do this as well as if my current changes make sense. I have attached a diff of my changes so far.
Attachment #8696306 - Attachment is patch: true
Comment on attachment 8696306 [details] [diff] [review]
bug1122618.txt

Review of attachment 8696306 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay, I somehow missed the new patch in my bugmail. In the future, please don't hesitate to set a feedback flag on attachments when you expect some input from me/a reviewer.

Your current changes make sense :-). It's fine with me if you just address the 2 following comments and don't further expand the scope to fix the move/restore default cases.

I don't see any solution to the restore default case that wouldn't significantly expand the scope of the patch. The only idea I have right now is to move most of the code of the restoreDefaultEngines method to nsSearchService, so that search.js can listen to a single notification for the whole thing, and rebuild the tree and the drop down. If we really want to pursue this, I think it would be better in a separate bug.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    var index = -1;

Can this be simplified using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex ?

::: toolkit/components/search/nsSearchService.js
@@ +3991,4 @@
>        // Since we removed an engine, we need to update the preferences.
>        this._saveSortedEngineList();
>      }
> +    

nit: please avoid adding trailing whitespace.
Attachment #8696306 - Flags: feedback+
(Assignee)

Comment 9

3 years ago
Created attachment 8703748 [details] [diff] [review]
bug1122618updated.patch

Here is my updated patch. I changed the code to find the index using array.findIndex. I can no longer find the whitespace that you were referring to, but I will pay attention to whitespace in my future patches. Thanks
Attachment #8703748 - Flags: review?(florian)
Comment on attachment 8703748 [details] [diff] [review]
bug1122618updated.patch

Review of attachment 8703748 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. The following comments are mostly cosmetic details :-).

For the commit message, we indicate the reviewer using irc nicks rather than the full name, eg. for me it would be r=florian.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    var index = this._engines.findIndex(function(element){ return element.name == aEngine.name; });

function(element){ return element.name == aEngine.name; }
can be simplified to:
  element => element.name == aEngine.name

Additionally, to avoid calling the 'name' getter repeatedly on the aEngine object, you can store it in a variable before calling findIndex.

nit: in new code we tend to replace 'var' with 'let'. I would do it here.

::: toolkit/components/search/nsSearchService.js
@@ +4019,4 @@
>        // Since we removed an engine, we need to update the preferences.
>        this._saveSortedEngineList();
>      }
> +    

You still have trailing whitespace on this line.
Attachment #8703748 - Flags: review?(florian) → feedback+
Attachment #8696306 - Attachment is obsolete: true
Attachment #8690576 - Attachment is obsolete: true
(Assignee)

Comment 11

3 years ago
Created attachment 8704231 [details] [diff] [review]
bug1122618updated.patch

Added the specified changes.
Attachment #8703748 - Attachment is obsolete: true
Attachment #8704231 - Flags: review?(florian)
Comment on attachment 8704231 [details] [diff] [review]
bug1122618updated.patch

Review of attachment 8704231 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! I'll test this one last time tomorrow before setting the r+ flag.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    let aEngineName = aEngine.name;

The 'a' prefix means 'argument', we don't use it for local variables (and we don't really use that prefix anymore in new code these days).
Attachment #8704231 - Flags: review?(florian) → review+
https://hg.mozilla.org/integration/fx-team/rev/503ebfe13cd2f079829f4a4c8b7e860b4989403f
Bug 1122618 - Added change to rebuild the defaultEngineDropDown whenever the default engine is changed. Separated the code performing the changes and displaying the changes when removing/adding engines. r=florian.

Comment 14

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/503ebfe13cd2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Hi Jeremy. Thanks for fixing this bug. If you would like suggestions for another bug to look at, I can propose bug 1170193 or bug 1214284.
(Assignee)

Comment 16

3 years ago
Hi Florian, thanks for the help. I am currently back in school right now and I won't have a lot of free time for the next little while. I will pick back up where I left off in when I have some time.
(In reply to Jeremy Francispillai from comment #16)
> Hi Florian, thanks for the help.

You are welcome!

> I am currently back in school right now and
> I won't have a lot of free time for the next little while. I will pick back
> up where I left off in when I have some time.

No worries, feel free to pick another bug whenever you have time and feel like it.
Flags: qe-verify+

Updated

3 years ago
Depends on: 1260820
I verified the bug. This is verified fixed on:
- 46.0b7 (20160330161941),
- 47.0a2 (2016-04-01),
- 48.0a1 (2016-03-31),
using Windows 7 Ultimate x64, Ubuntu 14.04 x86 and Mac OS X 10.11 Beta.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified

Updated

2 years ago
Depends on: 1327953

Updated

2 years ago
Depends on: 1332671
You need to log in before you can comment on or make changes to this bug.