1200367 - saved searches should have an opt-out pref in settings

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Allison Naaktgeboren :ally]

https://bugzilla.mozilla.org/show_bug.cgi?id=1189719#c10 to c15 deal with the issue of user control of the new saved searches showing up on the Awesome Screen.

The core question is this: Should saved searches have their own pref or be controlled by the same pref used to opt in/out of the search engine suggestions.

Rnewman & I support a separate pref. These are a user's past history, their data, and they may want to hide it, but be ok with suggestions from the search engine. Or vice versa. 

Anthony sees this as all being one unit. NeedInfos were set on kar & darrin. Moving them to this bug.

Comment 1

[Allison Naaktgeboren :ally]

Welcome to the discussion! Saved searches has landed, and are currently controlled as one unit by the search engine suggestions pref. That is to say your saved searches(searches the user has typed into the url bar in the past) are visible if and only if you have opted into suggestions from the default search engine.

Question is, is the this the right way forward for user controlling their search experience. Please see comment 0 here and comments 10-15 on Bug 1189719 for more context and opinions.

Comment 2

[Barbara Bermes [:barbara] - NI please!]

I chatted with antlam and we both agreed that we'll do a separate pref option for the search history.

antlam will follow up with mocks.

Thanks :)

Comment 3

[Barbara Bermes [:barbara] - NI please!]

(In reply to Barbara Bermes [:bbermes] from comment #2)
> I chatted with antlam and we both agreed that we'll do a separate pref
> option for the search history.
> 
> antlam will follow up with mocks.
> 
> Thanks :)

opt-in option

Comment 4

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

So, there's two parts to this:

1) Settings pref (Settings > Customize > Search) under "Show search suggestions"
 - "Show search history" (copy TBD)

2) An "opt-in" UI for users to turn this on.
 - New AND Existing users will see this the first time something they search for has a "search history" hit. Note: we might have to play with this a bit. This won't be their first "search" since they've got no history, but in theory, the context provided by this action should help the experience.

Comment 5

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: prev_awes_suggestionstip2.png

copy tbd, let's see how this feels in practice. Ally, can we test a build?

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Just wanted to put some thoughts in here:

We show the opt-in UI for search suggestions for one main reason: search suggestions send data to the search provider on each key press. By opt-in, we allow the user to never send data to the search provider, if they don't want to.

Showing search history does not send any data to the search provider. Opt-in may not be strictly required here. Having a way to disable it would be nice, but the Settings UI might be enough. More feedback is needed.

If we keep two opt-in, I am wondering what the initial use case would feel like. Show search suggestions opt-in first, then show search history opt-in? Or show them both at the same time? Either way, it might feel a little awkward. UX is hard.

Comment 7

[Barbara Bermes [:barbara] - NI please!]

Ok, so if you feel, from a privacy perceptive, we can do opt-out for history, I'm fine with that but we should definitely separate both as an individual settings preference.

Comment 8

[Allison Naaktgeboren :ally]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Just wanted to put some thoughts in here:
> 
> We show the opt-in UI for search suggestions for one main reason: search
> suggestions send data to the search provider on each key press. By opt-in,
> we allow the user to never send data to the search provider, if they don't
> want to.
> 
> Showing search history does not send any data to the search provider. Opt-in
> may not be strictly required here. Having a way to disable it would be nice,
> but the Settings UI might be enough. More feedback is needed.
> 
> If we keep two opt-in, I am wondering what the initial use case would feel
> like. Show search suggestions opt-in first, then show search history opt-in?
> Or show them both at the same time? Either way, it might feel a little
> awkward. UX is hard.

I would certainly like an opt-out. I as a user kinda get sick of prompts, though as a engineer I want them.

So, sounds like consensus on the opt-out then. 

:antlam, do you think you could get to the text for the setting sometime this week?

Comment 9

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Just wanted to put some thoughts in here:
> 
> We show the opt-in UI for search suggestions for one main reason: search
> suggestions send data to the search provider on each key press. By opt-in,
> we allow the user to never send data to the search provider, if they don't
> want to.
> 
> Showing search history does not send any data to the search provider. Opt-in
> may not be strictly required here. Having a way to disable it would be nice,
> but the Settings UI might be enough. More feedback is needed.

Talked to Darrin about this too, I think we're in agreement about NOT adding a second opt-in tip. Although the user may not make the distinction, we think our original assumption about the value that this brings is still valid.

So, let's just have the pref available for our users but not prompt them to "opt-in" to having search history. I.e. have search history on my default

> If we keep two opt-in, I am wondering what the initial use case would feel
> like. Show search suggestions opt-in first, then show search history opt-in?
> Or show them both at the same time? Either way, it might feel a little
> awkward. UX is hard.

UX IS hard! The idea was that search history opt-in only shows when there is "a match". So, a couple of searches would have to be done before a user will see "a match" and therefore this opt-in tip. 

But I don't think this applies anymore :)

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #8)
> :antlam, do you think you could get to the text for the setting sometime
> this week?

Sure, let's keep it simple for now and place it under the "Show search suggestions" pref in Customize > Search.

"Show search history"

We can revisit this after the settings re-org too.

Comment 10

[Allison Naaktgeboren :ally]

sounds like enough to get a patch up for review. :)

Comment 11

[Barbara Bermes [:barbara] - NI please!]

Hey Ally, can you let us know when we can get a patch for this :) Thanks

Comment 12

[Allison Naaktgeboren :ally]

It's approaching the top of my priority queue, so provided nothing more important comes in, sometime next week.

Comment 13

[Barbara Bermes [:barbara] - NI please!]

thanks ally

Comment 14

[Allison Naaktgeboren :ally]

Attachment: SavedSearchOptoutOption wip

most of the way there:
- code path isn't receiving the state of the checkbox or the default pref value set in mobile.js. Thinks saved searches are always disabled.

- robocop test failures? browser/ robocop directory running locally right now so that's unknown.

Comment 15

[Allison Naaktgeboren :ally]

try looks pretty good

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b6d762c49d7

Comment 16

[Allison Naaktgeboren :ally]

Attachment: SavedSearchOptoutOption

Default value comes in through mobile.js (note changes to this file require a rebuild & package), pref ui is removed in the case of a nonNightly build in GeckoPreferences.java, and we use PrefsHelper to register an observer.


New expected behavior
case 0 : new profile & search engine suggestion prompt not yet set
- should see user entered search term (first entry)
- should see saved search suggestions if there are any
- in settings, the "Show search history" checkbox should be checked
- in settings, the "Show search suggestions" checkbox should be unchecked

case 1: search suggestions from engines and history enabled
- should see entries without clocks and entries with clocks
- in settings, the "Show search history" checkbox should be checked
- in settings, the "Show search suggestions" checkbox should be checked

case 2: search engine suggestions disabled, saved search enabled
- see first term w/o clock (user entered text), rest with clocks if there are prior searches
- in settings, the "Show search history" checkbox should be checked
- in settings, the "Show search suggestions" checkbox should be unchecked

case 3: search engine suggestions enabled, history disabled in settings
- see several entries, all w/o clocks
- in settings, the "Show search history" checkbox should be unchecked
- in settings, the "Show search suggestions" checkbox should be checked

case 4: user clicked no to search engine suggestion prompt, has disabled saved searches in settings
- should see only the 1st search term (user entered text) without a clock icon
- in settings, the "Show search history" checkbox should be unchecked
- in settings, the "Show search suggestions" checkbox should be unchecked

Comment 17

[Allison Naaktgeboren :ally]

note the import for PrefsHelper is missing from this patch but not locally. Intellij keeps trying to replace it |import org.mozilla.gecko.*|  whenever I hit save for some reason.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8663847 [details] [diff] [review]
SavedSearchOptoutOption

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

::: mobile/android/base/home/SearchEngineRow.java
@@ +133,5 @@
>          mUserEnteredView.setOnClickListener(mClickListener);
>  
>          mUserEnteredTextView = (TextView) findViewById(R.id.suggestion_text);
> +
> +        PrefsHelper.getPref("browser.history.search.suggest.enabled", new PrefsHelper.PrefHandlerBase() {

Why use a Gecko pref instead of a regular SharedPreferences pref? It looks like this pref is only used in Java, so we could avoid passing messages over the bridge if we just use SharedPreferences.

Comment 19

[Chenxia Liu [:liuche]]

Comment on attachment 8663847 [details] [diff] [review]
SavedSearchOptoutOption

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

Mainly just switch to using an Android pref, and some code clean-up would be nice, but not strictly necessary.

::: mobile/android/app/mobile.js
@@ +281,5 @@
>  
>  // enable tracking protection for private browsing
>  pref("privacy.trackingprotection.pbmode.enabled", true);
>  
> +// disable search suggestions by default, enable saved searches by default

If you use a SharedPreference pref, you don't need to set this in the Gecko prefs.

::: mobile/android/base/home/SearchEngineRow.java
@@ +69,5 @@
>      // Maximums for suggestions based on form factor
>      private static final int TABLET_MAX = 4;
>      private static final int PHONE_MAX = 2;
>  
> +    // Whether the user has enabled saved search history suggestions

Nit: I wouldn't say "user enabled" because this is opt-out and on by default. Just whether the state is enabled or not.

@@ +133,5 @@
>          mUserEnteredView.setOnClickListener(mClickListener);
>  
>          mUserEnteredTextView = (TextView) findViewById(R.id.suggestion_text);
> +
> +        PrefsHelper.getPref("browser.history.search.suggest.enabled", new PrefsHelper.PrefHandlerBase() {

This should be an Android pref, not a Gecko pref. I think the problem you're running into is that the preference is not initialized until the xml is loaded, which won't happen until the user opens the Settings page. If the default value is true, then you should have the prefs.getBoolean call return true by default (if the pref doesn't exist). But definitely verify that when you toggle the pref in Settings, it still returns the correct thing.

See how the SUGGESTED_SITES pref works:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/db/SuggestedSites.java#423

Either that, or we can initialize all of our prefs in app creation:
http://developer.android.com/reference/android/preference/PreferenceManager.html#setDefaultValues%28android.content.Context,%20int,%20boolean%29

@@ +302,5 @@
>  
>          return suggestionCounter;
>      }
>  
> +    // This can be called before the opt-in permission prompt is shown or set.

Nit: If this comment is going to go outside the method, I'd make it a proper comment with /** **/. Otherwise, I'd put it inside the method if you want to use //.

@@ -293,2 @@
>          mSearchEngine = searchEngine;
> -        // Set the search engine icon (e.g., Google) for the row.

I would keep these two comments in, actually.

@@ +312,2 @@
>  
> +        if (!AppConstants.NIGHTLY_BUILD && searchSuggestionsEnabled) {

These two logic branches could be combined into a nested if statement, and you can just check for searchSuggestionsEnabled within the (outer) if statement.

@@ +316,5 @@
> +        } else if (!AppConstants.NIGHTLY_BUILD) {
> +            return;
> +        }
> +
> +        final int recycledSuggestionCount = mSuggestionView.getChildCount();

Why recycledSuggestionCount? I don't really understand what this field means (e.g., why is it equal to the number of children of the suggestion view?). Maybe this can just be named "viewCount" or "suggestionCount".

(In general, I think "suggestion" is overloaded in this file.)

@@ +317,5 @@
> +            return;
> +        }
> +
> +        final int recycledSuggestionCount = mSuggestionView.getChildCount();
> +        final boolean isTablet = HardwareUtils.isTablet();

Can 'getSavedSearches' handle the tablet check instead of the caller passing in the boolean? isTablet isn't an optional parameter, and HardwareUtils is static anyways.

@@ +323,5 @@
> +            final Cursor c = getSavedSearches(searchTerm, isTablet);
> +            try {
> +                final int savedSearchCount = (c != null) ? c.getCount() : 0;
> +                final int suggestionViewCount = updateFromSearchEngine(animate, recycledSuggestionCount, isTablet, savedSearchCount);
> +                updateFromSavedSearches(c, animate, suggestionViewCount, recycledSuggestionCount);

I find these methods to be confusingly named because they're passing in lots of ints and the argument names aren't very clear about what they will be used for. :/ Please add some method comments for the updateFrom* methods with @param documentation.

@@ +338,5 @@
> +                if (c != null) {
> +                    c.close();
> +                }
> +            }
> +        } else if (searchSuggestionsEnabled) {

I'd like to see (either in this patch or in a follow-up) a single method to handle updating the Suggestions view with both saved searches and search engine suggestions (pass in how many saved search suggestions there are, and how many total suggestion slots there are). As it is, there is a lot of repeated code, and it's kind of awkward to read (and seems awkward to maintain, because the same two methods are being used multiple times in the same place).

::: mobile/android/base/resources/xml-v11/preferences_search.xml
@@ +12,5 @@
>                          android:title="@string/pref_search_suggestions"
>                          android:defaultValue="false"
>                          android:persistent="false" />
>  
> +    <CheckBoxPreference android:key="browser.history.search.suggest.enabled"

I agree that this could be a SharedPreference, rather than a Gecko pref.

@@ +14,5 @@
>                          android:persistent="false" />
>  
> +    <CheckBoxPreference android:key="browser.history.search.suggest.enabled"
> +                        android:title="@string/pref_history_search_suggestions"
> +                        android:defaultValue="false"

Also, this default value should be true, if it's turned on by default.

@@ +15,5 @@
>  
> +    <CheckBoxPreference android:key="browser.history.search.suggest.enabled"
> +                        android:title="@string/pref_history_search_suggestions"
> +                        android:defaultValue="false"
> +                        android:persistent="false" />

Have you tried making android:persistent="true"?

http://developer.android.com/reference/android/preference/Preference.html#attr_android:persistent

Comment 20

[Allison Naaktgeboren :ally]

Why recycledSuggestionCount? I don't really understand what this field means (e.g., why is it equal to the number of children of the suggestion view?). Maybe this can just be named "viewCount" or "suggestionCount".

The number of recycled/re-used suggestions, as the user types we may be able to re use some of the pills. The original name of this variable was 'suggestionCount' but that was deemed unclear in a previous review.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SearchEngineRow.java#189 is the most important use.

Exactly why this code is structured in this particular way to reuse suggestions is unclear to me, but that's what I inherited. 

(In general, I think "suggestion" is overloaded in this file.)

Probably.

I'd like to see (either in this patch or in a follow-up) a single method to handle updating the Suggestions view with both saved searches and search engine suggestions (pass in how many saved search suggestions there are, and how many total suggestion slots there are). As it is, there is a lot of repeated code, and it's kind of awkward to read (and seems awkward to maintain, because the same two methods are being used multiple times in the same place).

I give thee Bug 1207022. I would like to know more about the asks for the Fancy Suggestions(tm) before refactoring too much.

Comment 21

[Allison Naaktgeboren :ally]

Attachment: SavedSearchOptoutOption

see above comment for follow up bug and history of recycledSuggestionCount.

Comment 22

[Chenxia Liu [:liuche]]

Comment on attachment 8664045 [details] [diff] [review]
SavedSearchOptoutOption

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

Thanks! Even the @param comments are much, much, much more helpful. Just make them real comments now!

Side note: I found savedSearch to be a confusing term - probably searchHistory would be better, because the user isn't saving searches.

I need to build this, but sending this back because I'm having some mach artifact problems, and might just need to clobber. Dark ages.

::: mobile/android/base/home/SearchEngineRow.java
@@ +221,5 @@
>          }
>      }
>  
> +    /**
> +     *

Sorry I wasn't explicit - these should real method comments, where you explain what the method does, not just the params. It's not clear to me what the effects of this method are (what UI is updated, etc).

@@ +264,5 @@
>  
>          String[] columns = new String[] { SearchHistory.QUERY };
>          String actualQuery = SearchHistory.QUERY + " LIKE ?";
>          String[] queryArgs = new String[] { '%' + searchTerm + '%' };
> +        final boolean isTablet = HardwareUtils.isTablet();

Just combine these two lines - HardwareUtils.isTablet() ? TABLET_MAX : PHONE_MAX

@@ +274,5 @@
>  
> +    /**
> +     *
> +     * @param animate whether or not to animate suggestions for visual polish
> +     * @param recycledSuggestionCount How many suggestion pills we could recycle from previous calls

What is a suggestion pill? Is there a more general term that could be used for this?

@@ +283,5 @@
>  
>          // Remove this default limit value in Bug 1201325
>          int limit = TABLET_MAX;
>          if (AppConstants.NIGHTLY_BUILD) {
> +            final boolean isTablet = HardwareUtils.isTablet();

Same here.

@@ +311,5 @@
>  
>          return suggestionCounter;
>      }
>  
> +    /**

Can you also add a method comment here explaining at a high level what this is updating (updates Suggestions UI to include X and Y based on Z, or something along those lines).

::: mobile/android/base/preferences/GeckoPreferences.java
@@ +136,5 @@
>      private static final String PREFS_TRACKING_PROTECTION_PRIVATE_BROWSING = "privacy.trackingprotection.pbmode.enabled";
>      private static final String PREFS_TRACKING_PROTECTION_LEARN_MORE = NON_PREF_PREFIX + "trackingprotection.learn_more";
>      private static final String PREFS_CATEGORY_PRIVATE_DATA = NON_PREF_PREFIX + "category_private_data";
>      public static final String PREFS_HOMEPAGE = NON_PREF_PREFIX + "homepage";
> +    public static final String PREFS_HISTORY_SAVED_SEARCH = "browser.history.search.suggest.enabled";

This pref should be an android.not_a_preference.* pref throughout. And I don't think this pref quite makes sense - can you do NON_PREF_PREFIX + "search.search_history.enabled" instead? This isn't history, and we want to differentiate it from plain search suggestions.

Comment 23

[Allison Naaktgeboren :ally]

@@ +274,5 @@
>  
> +    /**
> +     *
> +     * @param animate whether or not to animate suggestions for visual polish
> +     * @param recycledSuggestionCount How many suggestion pills we could recycle from previous calls

What is a suggestion pill? Is there a more general term that could be used for this?

They were called 'suggestion' before. 'Chiclet' as a term has also been tossed around, as well as 'button'. The text of a suggestion & its surrounding ui. We don't have a good term to separate that out from anything else that could be named 'suggestion'

Comment 24

[Allison Naaktgeboren :ally]

Attachment: SavedSearchOptoutOption

Comment 25

[Chenxia Liu [:liuche]]

Comment on attachment 8664414 [details] [diff] [review]
SavedSearchOptoutOption

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

Nice :)

Comment 26

[Allison Naaktgeboren :ally]

https://hg.mozilla.org/integration/fx-team/rev/faacd1e73967ff77138806ca5e9d833990b4238f
Bug 1200367 - saved searches should have an opt-out pref in settings.r=liuche

Comment 27

[Carsten Book [:Tomcat]]

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