925427 - Order of search engines is different in awesomescreen and settings page

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

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.

Comment 1

[Chenxia Liu [: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.

Comment 2

[:Margaret Leibovic]

Attachment: patch

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.

Comment 3

[:Margaret Leibovic]

Attachment: (Part 2) Clean up SearchPreferenceCategory.handleMessage

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.

Comment 5

[Chenxia Liu [:liuche]]

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?

Comment 6

[:Margaret Leibovic]

(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 7

[Chenxia Liu [:liuche]]

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.

Comment 8

[:Margaret Leibovic]

(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

Comment 9

[:Margaret Leibovic]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bdd94ab5687d

Comment 11

[:Margaret Leibovic]

This should track 26, since we want to make sure this works with the new search engines in settings UI.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1cfa9e04efb7