Closed Bug 867108 Opened 6 years ago Closed 6 years ago

hiding and then restoring the default search engine leads to the wrong engine being returned from searchService.defaultEngine until a restart

Categories

(Firefox :: Search, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: u421692, Assigned: Gavin)

References

Details

Attachments

(1 file)

Environment:
Build: Firefox for Android 23.0a1 (2013-04-29)
Device: Samsung Galaxy Tab 10.1(GT-P7510)
OS: Android 4.0.4

Steps to reproduce:
1. Go to Add-ons screen.
2. Disable the default search engine.

Expected result:
The second search engine in list is made default search engine and should be moved to top of the list

Actual result:
The second search engine in list is made default search engine but it is not moved to top of the list
Why does ordering matter?
At the moment when the user chooses to set a search engine as default using the "Set as Default" option, the search engine is moved to the top of the list. When the default engine is changed as a result of disabling the current default search engine, the list is not changed. If the default engine is always the first in the list the user will always know which search engine is set as default since there is no clear indication in the add-ons manager of this.
It sounds like the real issue here is that you want a visual indication of which search engine is the default.

I'm opposed to reordering the search engines when one is disabled, because it will either stay at the bottom if it's re-enabled, or we'd have to do something complicated to remember where it should go back to when re-enabled. This seems potentially buggy, and hard to figure out exactly what the expected behavior would be.

Also, although the default engine isn't at the top in that case, it is still the topmost enabled search engine, so the information being presented to the user isn't actually wrong.
(In reply to :Margaret Leibovic from comment #3)

> Also, although the default engine isn't at the top in that case, it is still
> the topmost enabled search engine, so the information being presented to the
> user isn't actually wrong.

Yes, but when the disabled search engine is reenabled it will be the topmost enabled search engine(preserves his position), but it will not be the default search engine(this might confuse the user).
(In reply to Pop Mihai from comment #4)
> (In reply to :Margaret Leibovic from comment #3)
> 
> > Also, although the default engine isn't at the top in that case, it is still
> > the topmost enabled search engine, so the information being presented to the
> > user isn't actually wrong.
> 
> Yes, but when the disabled search engine is reenabled it will be the topmost
> enabled search engine(preserves his position), but it will not be the
> default search engine(this might confuse the user).

Hm, this is a weird situation that I didn't think about. I guess I assumed re-enabling a search engine that used to be the default would make it the default again, but that's not the case. Now that you mentioned this, maybe we should consider moving an engine to the bottom of the list when it's disabled. I'm hesitant about this change because we don't give users a way to reorder engines themselves, but I guess we've already gone there, since we move an engine to the top of the list if it's set as the default.

Also, it does make the awesomescreen look confusing if the default engine is second in the list, so for that alone I think we should make sure the default engine is at the top of the list.

Basically, this is confusing because the search service treats engine order and default engine as different concepts, but our UI conflates those issues.
Let's not go to extremes trying to make this work. If we re-order disabled search engines, we need to re-order all the things: add-ons and themes too.

I'd rather go simple and just not allow the default search engine to be disabled.

Themes are similar, but we never show the "default" theme, so users can't end up in this situation. If we did show the default theme, we would probably not allow it to be disabled - which is what desktop does.
After talking to Gavin on IRC, it seems like this is actually a search service issue that should be fixed. The default engine pref doesn't change when the engine is hidden/unhidden, so we're currently returning the wrong default engine when the default is hidden/unhidden.
Assignee: nobody → gavin.sharp
Component: General → Search
Product: Firefox for Android → Firefox
Version: Firefox 23 → Trunk
Summary: Default search engine should always be first in list → hiding and then restoring the default search engine leads to the wrong engine being returned from searchService.defaultEngine until a restart
Attached patch patchSplinter Review
Includes a test (and cleans up the existing test a little).
Attachment #746106 - Flags: review?(mdeboer)
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 746106 [details] [diff] [review]
patch

looks good, but I have one question: for example, when I select Google, all Firefox searches use that engine. When I hide it, it correctly selects the next engine in order; searches go through Yahoo for example. When I now unhide the Google engine, Yahoo is still the current engine, but the default engine is set back to Google. Should we update the pref too when the default engine is hidden?
Attachment #746106 - Flags: review?(mdeboer) → feedback+
(In reply to Mike de Boer [:mikedeboer] from comment #9)
> looks good, but I have one question: for example, when I select Google, all
> Firefox searches use that engine. When I hide it, it correctly selects the
> next engine in order; searches go through Yahoo for example. When I now
> unhide the Google engine, Yahoo is still the current engine, but the default
> engine is set back to Google. Should we update the pref too when the default
> engine is hidden?

I think your patch in bug 860560 should address this particular case?

I agree in general that the "magical getter" that sometimes doesn't return the engine associated with the value of the defaultenginename pref is kind of broken, but I think it may be best to address that in a followup bug, where we fix things per the parenthetical in bug 738818 comment 85.
Comment on attachment 746106 [details] [diff] [review]
patch

Ah, I understand what you mean now. So reviewing this patch made me aware that I need to add a case to bug 860560, since this will land sooner.
Attachment #746106 - Flags: feedback+ → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/51c22367953f
Flags: in-testsuite+
Target Milestone: --- → Firefox 24
https://hg.mozilla.org/mozilla-central/rev/51c22367953f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.