Closed Bug 910189 Opened 10 years ago Closed 10 years ago

Unable to deactivate (not remove) the default search providers/engines

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec+)

Status:
RESOLVED FIXED
Milestone:
Firefox 29
Tracking Flags:
Tracking Status
fennec + ---

People

(Reporter: kats, Assigned: liuche)

References

Details

(Keywords: regression)

Attachments

(9 files, 11 obsolete files)

long-press-and-remove-default-providers.png
694.95 KB, image/png
Details
restore-default-providers.png
1.02 MB, image/png
Details
Screenshot: Tablet UI
80.95 KB, image/png
Details
Screenshot: tablet Android settings
102.45 KB, image/png
Details
Screenshot: pre-ICS restore defaults settings item
40.35 KB, image/png
Details
Part 1: Add "restore defaults" hooks into Search Settings for a dialog v2
10.78 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
Screenshot: [new] pre-ICS restore defaults settings item
43.63 KB, image/png
Details
Part 2: Allow removal of default engines v4
10.35 KB, patch
liuche
: review+
Details | Diff | Splinter Review
Part 3: Restore default search engines (no dialog) v5
12.01 KB, patch
Details | Diff | Splinter Review 
On a Galaxy Q running 2.3.6 with latest nightly:
1. Go into the settings > Customize > Search Settings
2. Wait for the list to populate
3. Long-press on the Twitter search provider (and lift finger, see bug 910186)

Expected: context menu has a "Remove" item that can be used to delete the twitter search

Actual: no way to delete the twitter search (or any of the other built-ins). My custom search engines do have a remove item.

I will never use the twitter search so having the search provider there is just a waste of space and makes it harder to use my custom search providers because I have to scroll to get to them. The default search providers should be removable like all the others.
Yes. I pushed the devs to not worry about removing the built-in engines for the initial restructure of the UI. I wanted it to happen in a followup, which this bug can be if it's not already a dupe.

Next, we can't actually remove the built-in engines. We can only hide them. Since they are built-in, we need a way to make them visible again too.

Desktop Firefox uses a "Restore Defaults" button to make any hidden engines visible again. We'd need the same. This action is in fact the reason why I pushed to ignore hiding built-in engines since there was no design ready to handle it.

Let's get a design together for how to restore the built-in engines, then we can add "Remove" to the built-in engines (which would hid them under the covers).
Flags: needinfo?(ibarlow)
tracking-fennec: --- → ?
Blocks: 892113
tracking-fennec: ? → +
(In reply to Mark Finkle (:mfinkle) from comment #1)
> Let's get a design together for how to restore the built-in engines, then we
> can add "Remove" to the built-in engines (which would hid them under the
> covers).

How about doing it like before the All In One Screen? Deactivating in the context menu of the search engines in the search settings page, and re-enabling in the same place. Deactivated search engines get a grey, alpha layer on top.
Summary: Unable to remove the default search providers → Unable to deactivate (not remove) the default search providers/engines
In bug 910186 Arun points out that we could use the "3 dot" menu in the Search settings view to hold the "Restore defaults" action. Seems reasonable to me. Might be better that a button, but I think we'll need to handle the cases where menus are not available. I don't know if Gingerbread has this UI concept. We might need a button there.
(In reply to Mark Finkle (:mfinkle) from comment #4)
> In bug 910186 Arun points out that we could use the "3 dot" menu in the
> Search settings view to hold the "Restore defaults" action. Seems reasonable
> to me. Might be better that a button, but I think we'll need to handle the
> cases where menus are not available. I don't know if Gingerbread has this UI
> concept. We might need a button there.

Do all such devices have the "Menu" button? If so, we could stick it in that menu (Isn't the 3-dot menu the thing that was invented to replace the "Menu" button, anyway? Or do there exist devices that support neither?)
(In reply to Chris Kitching [:ckitching] from comment #5)
> (In reply to Mark Finkle (:mfinkle) from comment #4)
> > In bug 910186 Arun points out that we could use the "3 dot" menu in the
> > Search settings view to hold the "Restore defaults" action. Seems reasonable
> > to me. Might be better that a button, but I think we'll need to handle the
> > cases where menus are not available. I don't know if Gingerbread has this UI
> > concept. We might need a button there.
> 
> Do all such devices have the "Menu" button? If so, we could stick it in that
> menu (Isn't the 3-dot menu the thing that was invented to replace the "Menu"
> button, anyway? Or do there exist devices that support neither?)

Older versions of Android don't support the "3 dot" menu. Those devices should support a hardware menu button. A potential problem in those cases is discoverability. A button row in the settings list is much easier to see. Even better than the "3 dot". This could mean we need to decide if the action is important enough to promote to a button or keep as a menu action.

An example is the standard Wifi Settings screen. In newer Androids, the "3 dot" is the only way (I could find) to "scan" and view "advanced" settings, and "add" was a visible action button. In older Androids, "Add Wi-Fi Network" is a button in the list, but "scan" and "advanced" were still on the hardware menu button.
(In reply to Mark Finkle (:mfinkle) from comment #6)
> (In reply to Chris Kitching [:ckitching] from comment #5)
> > (In reply to Mark Finkle (:mfinkle) from comment #4)
> > > In bug 910186 Arun points out that we could use the "3 dot" menu in the
> > > Search settings view to hold the "Restore defaults" action. Seems reasonable
> > > to me. Might be better that a button, but I think we'll need to handle the
> > > cases where menus are not available. I don't know if Gingerbread has this UI
> > > concept. We might need a button there.
> > 
> > Do all such devices have the "Menu" button? If so, we could stick it in that
> > menu (Isn't the 3-dot menu the thing that was invented to replace the "Menu"
> > button, anyway? Or do there exist devices that support neither?)
> 
> Older versions of Android don't support the "3 dot" menu. Those devices
> should support a hardware menu button. A potential problem in those cases is
> discoverability. A button row in the settings list is much easier to see.
> Even better than the "3 dot". This could mean we need to decide if the
> action is important enough to promote to a button or keep as a menu action.

I can understand the concerns regarding discoverability. I would say since the case of a user deleting their providers and wanting to restore it is fairly small, we could afford to put it under the 3-dot menu (for ICS+) as suggested by you. (design attached)
 
> An example is the standard Wifi Settings screen. In newer Androids, the "3
> dot" is the only way (I could find) to "scan" and view "advanced" settings,
> and "add" was a visible action button. In older Androids, "Add Wi-Fi
> Network" is a button in the list, but "scan" and "advanced" were still on
> the hardware menu button.

Seems like the way to go for GB. Will take a look on a GB device and get back.
I mostly agree with Arun Balachandran Ganesan. Using the overlay menu of each item to provide a "Remove" and/or "Disable" option(s) seems like the way to go.
One disagreement: I think that allowing users to restore defaults while providing a way to select which of their custom settings should be kept is overkill. It would be a better design to make it *less useful or necessary* for users to resort to "Restore defaults", and then keep "Restore defaults" as a dumb action (perhaps with a standard confirm dialog).

If default search engines (SEs) need to "stay" so that they can be restored, there are several ways to handle this:

Option 1 [preferred]:
- Built-in SEs have "Disable" option, can’t be deleted.
- User-installed SEs have "Delete" option.
Then, there is very little need for restoring defaults, except as a shortcut, since deleting your custom SEs and enabling disabled built-in ones do the same thing.

Option 2:
- Built-in SEs have "Disable" option, can’t be deleted.
- User-installed SEs have "Delete" option.
- User-installed SEs have "Disable" option as well.
Same benefits as Option 1.

Option 3:
- Built-in SEs have "Delete" option (they’re just disabled under the hood).
- "Deleted" built-in SEs disappear from the list.
- User-installed SEs have "Delete" option (and get deleted for realz).
- "Restore defaults" deletes user SEs, enables all built-in SEs.

I would vote for Option (1), as (3) is too complex and relies on "Restore defaults" too much, and (2) is a bit more complicated than (1) but is not more forgiving of errors than (1) (if you Delete a custom SE instead of Disabling it, it doesn’t help that there was an option to disable).
Thanks for your thoughts, Florent.
 
> Option 1 [preferred]:
> - Built-in SEs have "Disable" option, can’t be deleted.
> - User-installed SEs have "Delete" option.
> Then, there is very little need for restoring defaults, except as a
> shortcut, since deleting your custom SEs and enabling disabled built-in ones
> do the same thing.

If I understand you correctly:
This bug would enable built-in SEs to be deleted. Restore defaults would thus allow the user to restore those search engines. By allowing built-in SEs to be deleted, the difference between a built-in and user installed SE is decreased (except for search suggestions limited built-in SEs). This should reduce complexity, however little. 

Also, I'm confused about the whole, 'disabling option'. We don't currently support disabling a search engine and I don't see a clear rationale for supporting it.
It was fine as it was in v25. Just let us disable the built-ins and delete the added ones. A "restore defaults" is overkill, or do we have a similar mechanism to restore the default bookmarks?
Major regression. Running latest Firefox for Android beta and I'm completely unable to remove or disable built in search-providers.

Phone running Android 4.4.
Seeing this requested multiple times from users on reddit and on SUMO forums
I think while supporting "Disable" for default search engines probably reflects what we're actually doing better, it might be a little confusing to treat some of the engines differently, because users probably won't distinguish "default search engines" as different (hence the requests to remove them).

That being said, the "restore defaults" panel does feel a littttle heavy, especially since we're already hiding it behind the menu/3-dot, but I think that's fine.

Do we need different mocks for pre-ICS? I don't even remember how menus work in Gingerbread and earlier. :\
(In reply to Chenxia Liu [:liuche] from comment #16)

> Do we need different mocks for pre-ICS? I don't even remember how menus work
> in Gingerbread and earlier. :\

I think we could just fallback to a "Restore Defaults" row for Pre-ICS. The 3-dot menu could be a pain and require some more significant changes. Start with that first.
(In reply to Aaron Train [:aaronmt] from comment #15)
> Seeing this requested multiple times from users on reddit and on SUMO forums

I agree. Let's get some focus on this for Fx29. Thanks.
Chenxia - Assigning to you since I think you have a good handle on the different parts and you can let us know if something is not going to work the way we hope it would.
Assignee: nobody → liuche
Attached image Screenshot: Tablet UI — 
I threw this together (which involved an ugly hack - why am I surprised?), but the tablet UI looks pretty weird, mainly because the ActionBar where the 3-dot menu appears is in the outer context of the settings, instead of in the actual Fragment frame.

Any input on tablet UI would be welcome!
Status: NEW → ASSIGNED
Flags: needinfo?(abc)
Here's a screenshot for Android's own settings on Android 4.0.3. It looks like they also do the menu on the outside - so this could just be fine.
Thanks, Chenxia!

That works, then.
Flags: needinfo?(abc)
Add UI for pre-ICS and ICS+ for restoring default search engines. Dialogs to come.
I added a new category and strings. Other string suggestions welcome.
Flags: needinfo?(abc)
Dammit, qref.
Attachment #8347511 - Attachment is obsolete: true
That was easy!
(In reply to Chenxia Liu [:liuche] from comment #27)
> Created attachment 8347535 [details] [diff] [review]
> Part 2: Allow removal of default engines v1
> 
> That was easy!

I'm glad to see you're killing this.

If you've not added another use of the "immutable" field on that JSONObject (The one that was queried to populate mIsImmutableEngine) then you might also want to kill that field. I added that field to the JSONObject in part 1 of the patches in Bug 892113 solely to make the logic you're removing work, so unless this field is useful elsewhere you probably want to get rid of that, too. (You may want to check out the changes to browser.js in that patch to see the details).

Just a sudden thought.
Comment on attachment 8347535 [details] [diff] [review]
Part 2: Allow removal of default engines v1

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

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

Drive-by: This is the only place we're currently using "SearchEngines:Get". So, we should get rid of the code where we handle "SearchEngines:Get", and perhaps we should rename "SearchEngines:GetVisible" to "SearchEngines:Get", since there will be just one. This could be a follow-up bug, but it would make our SearchEngines JS code simpler!

(Note: this will also involve updating testDistribuion but nobody is hiding any search engines before that test runs, so I don't think we should worry about that.)
Missed removal of a JSONObject "immutable" field.
Attachment #8347535 - Attachment is obsolete: true
This patch restores "deleted" default search engines (for both pre-ICS and ICS+), but doesn't include the fancy dialog.
Added some xml keys.
Attachment #8347517 - Attachment is obsolete: true
Flags: needinfo?(ibarlow)
Flags: needinfo?(abc)
Do we need the fancy dialog for v1? I think restoring all the built-in engines seems like a good start. I don't know that we need individual restoration support.
(In reply to Mark Finkle (:mfinkle) from comment #33)
> Do we need the fancy dialog for v1? I think restoring all the built-in
> engines seems like a good start. I don't know that we need individual
> restoration support.

Works for me
Blocks: 951265
Still waiting for thought from :abc about the strings for pre-ICS versions, so those might change.
Attachment #8348418 - Attachment is obsolete: true
Attachment #8348937 - Flags: review?(margaret.leibovic)
Removed SearchEngines:Get - I kept the other message as SearchEngines:GetVisible for consistency with nsBrowserSearchService, and also because we'll need to call search.getDefaultEngines() for the fancy dialog.
Attachment #8348197 - Attachment is obsolete: true
Attachment #8348938 - Flags: review?(margaret.leibovic)

        Attachment #8348417 -
        Attachment is obsolete: true

        Attachment #8348941 -
        Flags: review?(margaret.leibovic)

    
    

    
  


  
New strings.

        Attachment #8348937 -
        Attachment is obsolete: true

        Attachment #8348937 -
        Flags: review?(margaret.leibovic)

        Attachment #8348997 -
        Flags: review?(margaret.leibovic)

    
    

    
  


  
New screenshot for pre-ICS, after discussion with Arun.

    
    

    
  
Thanks, Chenxia. This looks good.

    
    

    
  
Based on discussions with liuche & ibarlow:

We need consistent wording for usage of "search provider" vs "search engine"

From my understanding (which is more likely to be flawed here than not), 

- Search engine (user friendly term?) refers to Google, Yahoo and the likes that crawl the web.
- Search provider (technically accurate term?) may additionally refer to just search in websites, which we support users to add too.
- Maybe, there's a better term?

We could fix it here or in a follow-up bug if that makes more sense.

    
    

    
  
Comment on attachment 8348997 [details] [diff] [review]
Part 1: Add "restore defaults" hooks into Search Settings for a dialog v2

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

I'm slightly nervous about how this will work on various devices, but it looks like you've tested on some different ones and resolved the issues that popped up. We should ask QA to test on their arsenal of phones after this lands.

        Attachment #8348997 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  
Comment on attachment 8348938 [details] [diff] [review]
Part 2: Allow removal of default engines v3

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

Always nice to see things get deleted!

        Attachment #8348938 -
        Flags: review?(margaret.leibovic) → review+
Hardware: Other → All

    
    

    
  
Comment on attachment 8348941 [details] [diff] [review]
Part 3: Restore default search engines (no dialog) v2

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

r+ with comments considered :)

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +404,5 @@
> +                        public boolean onPreferenceClick(Preference preference) {
> +                            thisPreferences.restoreDefaultSearchEngines();
> +                            return true;
> +                        }
> +                    });

This feels kinda hacky, but I don't immediately know of a better way to do it, so I'm fine with us landing it.

@@ +424,5 @@
> +    protected void restoreDefaultSearchEngines() {
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createBroadcastEvent("SearchEngines:RestoreDefaults", null));
> +
> +        // Reload Search category.
> +        searchCategory.requestEngines();

Instead of holding a reference to searchCategory like this and calling this requestEngines method, could we just have gecko send another "SearchEngines:Data" after it restores the default set?

To do that, we would have to change where we remove the "SearchEngines:Data" listener in SearchPreferenceCategory. Do we have a way to know when that PreferenceCategory is destroyed/detached from the activity?

The approach here is acceptable, but I'm just trying to think if there's a more straightforward solution.

        Attachment #8348941 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  
Word of over-caution: please make sure that search recording in FHR continues to work in the wild after these changes...

    
    

    
  
Richard: I see org.mozilla.searches.counts in my raw about:healthreport data, so I'll verify that doesn't stop updating. Let me know if there's somewhere else I should be looking as well.

    
    

    
  


  
Good call, I like this much better too. Changes in this patch.

        Attachment #8348941 -
        Attachment is obsolete: true

        Attachment #8349535 -
        Flags: review?(margaret.leibovic)

    
    

    
  
Comment on attachment 8349535 [details] [diff] [review]
Part 3: Restore default search engines (no dialog) v3

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

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +77,5 @@
>      private boolean mInitialized = false;
>      private int mPrefsRequestId = 0;
>  
> +    // These match keys in resources/xml*/preferences*.xml
> +    private static String PREFS_SEARCH_CATEGORY = NON_PREF_PREFIX + "search.category";

You can get rid of this line now.

@@ +396,5 @@
> +                    final GeckoPreferences thisPreferences = this;
> +                    pref.setOnPreferenceClickListener(new OnPreferenceClickListener() {
> +                        @Override
> +                        public boolean onPreferenceClick(Preference preference) {
> +                            thisPreferences.restoreDefaultSearchEngines();

I thought there was some more elegant way to do closures in Java... I think you can just use `GeckoPreferences.this`, instead of storing this `thisPreferences` variable.

::: mobile/android/base/preferences/SearchPreferenceCategory.java
@@ +49,5 @@
>      }
>  
>      @Override
> +    protected void onPrepareForRemoval() {
> +        GeckoAppShell.unregisterEventListener("SearchEngines:Data", this);

Nice, I'm glad you can do this :)

        Attachment #8349535 -
        Flags: review?(margaret.leibovic) → review+

    
    

    
  


  
Carrying over the r+ after clearing out the other unnecessary code that I missed, and switching the Java closure style.

        Attachment #8349535 -
        Attachment is obsolete: true

        Attachment #8349735 -
        Flags: review+

    
    

    
  


  
Forgot to update a test using the "SearchEngines:Get" message, which was changed in this patch to "SearchEngines:GetVisible".

Try push: https://tbpl.mozilla.org/?tree=Try&rev=57429aba9306

        Attachment #8348938 -
        Attachment is obsolete: true

        Attachment #8350210 -
        Flags: review+

    
    

    
  
(In reply to Wes Kocher (:KWierso) from comment #55)
> Backed out in https://hg.mozilla.org/integration/fx-team/rev/01420aac16cd
> for breaking android builds:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32249927&tree=Fx-Team and
> https://tbpl.mozilla.org/php/getParsedLog.php?id=32250170&tree=Fx-Team

Hrm, one of the retriggers came back green, and was a clobber build. Maybe this needs a clobber?

    
    

    
  
QA, please test on different devices, since I want to make sure that the Android hack I put in there doesn't cause problems.

Also, please make sure to test with both pre-ICS and ICS+ devices, since these patches have different implementations for each.
Flags: needinfo?(aaron.train)

    
    

    
  
Testing on Nightly (01/05, 29.0) 

* Restore defaults does not restore the default search engine (Set Bing as default and then opt to 'restore default').


Is this desirable, 'defaults' in this case is not working as expected to the end-user is it?

Other than that, this looks good to me.

Off-Topic: We should annotate the 'Show search suggestions' preference above with an "(if available)" text.
Flags: needinfo?(aaron.train)

    
    

    
  
(In reply to Aaron Train [:aaronmt] from comment #60)

> Is this desirable, 'defaults' in this case is not working as expected to the
> end-user is it?

I seem to recall that this is just difficult wording. I think it's currently "restore any default engines I removed", not "reset any changes I made to my search engine preferences". But let's see what Chenxia says on Monday!


> Off-Topic: We should annotate the 'Show search suggestions' preference above
> with an "(if available)" text.

File a follow-up?

    
    

    
  
(In reply to Richard Newman [:rnewman] from comment #61)
> (In reply to Aaron Train [:aaronmt] from comment #60)
> 
> > Is this desirable, 'defaults' in this case is not working as expected to the
> > end-user is it?
> 
> I seem to recall that this is just difficult wording. I think it's currently
> "restore any default engines I removed", not "reset any changes I made to my
> search engine preferences". But let's see what Chenxia says on Monday!
> 

That's correct - changing a user's explicitly set default engine doesn't seem like the right thing to do. As I understand it, the intent behind "Restore defaults" is to provide users a way to bring back the default search engines they may have removed accidentally, because removing is easy but adding engines requires a little more work on the user's part.

> 
> > Off-Topic: We should annotate the 'Show search suggestions' preference above
> > with an "(if available)" text.
> 
> File a follow-up?

Looks like Aaron already filed a follow-up, dropping it here for reference: bug 956861.
Depends on: 1030113

    
  
Depends on: 892094

    
  
Depends on: 1052747
Product: Firefox for Android → Firefox for Android Graveyard

          You need to log in
          before you can comment on or make changes to this bug.