Order of search engines is different in awesomescreen and settings page

VERIFIED FIXED in Firefox 26

Status

()

defect
VERIFIED FIXED
6 years ago
3 years ago

People

(Reporter: Margaret, Assigned: Margaret)

Tracking

(Blocks 1 bug)

Trunk
Firefox 27
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox25 unaffected, firefox26 fixed, firefox27 verified, fennec26+)

Details

Attachments

(2 attachments)

This needs some more debugging, but it looks like the order of the search engines in the settings page always shows the non-default engines in alphabetical order, instead of the order in which they appear on the awesomescreen.

Chenxia, was this intentional? I think we should be showing the engines in the same order they're listed in the awesomesreen.

This bug would be a small step in the direction of bug 924461.
Flags: needinfo?(liuche)
Nope, I don't think this was intentional - it looks like search settings code just adds the engines in the order that are given, so we'll have to find some way to get the correct ordering.
Flags: needinfo?(liuche)
Posted patch patchSplinter Review
So, there were two issues here. The first is that we were doing  setOrderingAsAdded(false) when we really meant to call setOrderingAsAdded(true). After I made this change, I found that the first two engines would both end up with order 0, and this would make the default engine actually appear second in the list. It turns out that this problem was caused by setting the preference order in setIsDefaultEngine before actually adding it to the preference category, but moving around the order of things here actually fixed this.
Assignee: nobody → margaret.leibovic
Attachment #817459 - Flags: review?(liuche)
Here's a patch to clean up some things that were bothering me in here. First of all, we're assuming the first engine in the list is the default engine, so we can just do that inside the for loop, rather than getting the engine out of the array and comparing it later. Secondly, we should just unregister the listener as soon as we get this message, in the unlikely event that we bail early somewhere in the body of the method.
Attachment #817465 - Flags: review?(liuche)
Duplicate of this bug: 927117
Comment on attachment 817459 [details] [diff] [review]
patch

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

Looks good, let's fold this with the next patch for actual landing.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +45,4 @@
>  
>          // Request list of search engines from Gecko.
>          GeckoAppShell.registerEventListener("SearchEngines:Data", this);
>          GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));

While you're here, do you want to change this to SearchEngines:GetVisible?
Attachment #817459 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #5)

> Looks good, let's fold this with the next patch for actual landing.

I was thinking we should keep the fix patch as simple as possible, since we should uplift this for 26. But the follow-up clean-up is really quite low-risk, so maybe I shouldn't worry about that.

> ::: mobile/android/base/preferences/SearchPreferenceCategory.java
> @@ +45,4 @@
> >  
> >          // Request list of search engines from Gecko.
> >          GeckoAppShell.registerEventListener("SearchEngines:Data", this);
> >          GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:Get", null));
> 
> While you're here, do you want to change this to SearchEngines:GetVisible?

Sure!
Comment on attachment 817465 [details] [diff] [review]
(Part 2) Clean up SearchPreferenceCategory.handleMessage

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

lgtm, just fold this with the first patch.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +84,5 @@
>  
>                      addPreference(enginePreference);
>  
> +                    // The first element in the array is the default engine.
> +                    if (i == 0) {

Nice, that works.

I think what AaronMT is asking about in bug 927117 is storing a previous default so we can return to it if we delete the current default (although I might be mistaken). That bug might also be a WONTFIX, though, because we don't want to store a whole series of search defaults.
Attachment #817465 - Flags: review?(liuche) → review+
(In reply to Chenxia Liu [:liuche] from comment #7)

> I think what AaronMT is asking about in bug 927117 is storing a previous
> default so we can return to it if we delete the current default (although I
> might be mistaken). That bug might also be a WONTFIX, though, because we
> don't want to store a whole series of search defaults.

Luckily, because we always move the previous default to the second position, removing the current default will automatically restore it, so I don't think we need to worry about this.

Pushed:
https://hg.mozilla.org/integration/fx-team/rev/bdd94ab5687d
Status: NEW → ASSIGNED
Blocks: 924461
Blocks: 892094
Comment on attachment 817459 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 892094
User impact if declined: order of search engines in settings page is wrong
Testing completed (on m-c, etc.): landed on fx-team
Risk to taking this patch (and alternatives if risky): low-risk, only affects search engine list in search setting page
String or IDL/UUID changes made by this patch: none
Attachment #817459 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bdd94ab5687d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Status: RESOLVED → VERIFIED
tracking-fennec: ? → ---
This should track 26, since we want to make sure this works with the new search engines in settings UI.
tracking-fennec: --- → 26+
Attachment #817459 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 962047
You need to log in before you can comment on or make changes to this bug.