Closed Bug 1200319 Opened 4 years ago Closed 4 years ago

How many, which ones of which type of pills should be displayed on phones or tablets for search suggestions

Categories

(Firefox for Android :: Awesomescreen, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: ally, Assigned: ally)

References

Details

Attachments

(2 files)

I wanted to title this "How many pills should fennec take".

With the introduction of saved searches in the awesomescreen search, we have a couple questions to answer if the user has a lot of saved searches. 

The answers may be different on phones (small screens) vs tablets

We currently limit search engine suggestions to 4. 

How then should we limit the saved searches (these are previous searches done by the user) on small screens?

One of the proposals is 2 from the search engine, 2 from the user's history. If so, which two? The most recent? Maybe 3 saved seaches and 1 from the search engine?

The proposal for tablets is to show all matches. Tablets have lots of screen space.
Depends on: 1189719
Some specs, hope this helps!

Phones:
---

Total: 4 max
Suggestions: 4 max, 2 min
History: 2 max, 0 min

Tablets:
---

Total: 8 max
Suggestions: 4 max, 2 min
History: 4 max, 0 min.
Assignee: nobody → ally
How Many:
Fwiw, I can't really enforce a min. :) If either return no results, I can't really do anything there.

It is best to assume that the counts for history & suggestions be independent of each other. 

I have yet to come across a search where yahoo hasn't sent the max # of suggestions (3+original search term). I think the max for phones should be 6, otherwise, users are never going to see any of the saved suggestions.

I think 2 min for history on phones is a little harsh, but I'm willing to give it a go. 

If users hate typing, shouldn't we give them as many things to tap/long tap on as early as possible to reduce typing? 

Which ones:
The other question is elimination. Currently we show the newest matches that match the start of the search term exactly. Let's call this s-wildcard

Let's say You have searched (in order)
dog treats
dog beds
dog food
best leashes for dogs
dog leashes
dog with a blog
dog names
dog breeds

And you searched for "dog" on a phone currently, you would see:
dog breeds
dog names
dog with a blog
dog leashes

If you searched for "leashes" on a phone currently you would see:
[]  

Today mfinkle proposed that this is maybe not the best search. He thinks that wildcard-*-wildcard would be a better one. Assuming the 2 pill limit from before

And you searched for "dog" on a phone, you would see:
dog breeds
dog names

If you searched for "leashes" on a phone you would see:
dog leashes
best leashes for dogs
Met with Ally to clear this up over IRC.

tl;dr
 - 4 suggestions when no history
 - 2 of each when possible
 - ordering based on "time/date"
 - leashes will display a history item for "dog leashes"
 - filed follow up bug 1201670 for possibly removing first "suggestion"
s/IRC/IRL 

wildcard-searchterm-wildcard for sql search

not counting the first suggestion, which is what is in the urlbar not a new suggestion, toward search engine max
mcomella, feel free to reassign the review. The suggested reviewers tool is suggesting mbrubeck & wesj ;).

That patch introduces different limits for phones and tablets on search history and search engine suggestions. 

There is one special edge case: phone when there are 1 or 0 searches present, where we fill the empty space with extra search engine suggestions. In that case, the number of search engine suggestions is impacted by the number of saved searches and we need to know how many saved searches there are before showing the search engine suggestions, which are first in the list. This is why updateSavedSearchSuggestions has been broken up into two functions: the first that opens the cursor and counts the entries, and the second that shows them and closes the cursor.

Prior to this patch, the limit for both categories is 4. There is a hardcoded 4 in the patch for the pre-feature(no-feature?). 

Although the savedsearch functions are currently never called outside of the nightly flags, I have left Nightly guards inside the functions in case new call sites are added. Defensive programming and all that.
Attachment #8658353 - Flags: review?(michael.l.comella)
Comment on attachment 8658353 [details] [diff] [review]
SavedSearchesPillCounts

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

Seems functional so r+. However, I think it could use some readability improvements and wouldn't mind seeing it again after the suggested revisions.

::: mobile/android/base/home/SearchEngineRow.java
@@ +252,5 @@
>  
> +        // Remove this default limit value in Bug 1201325
> +        int limit = 4;
> +        if (AppConstants.NIGHTLY_BUILD) {
> +            limit = isTablet ? TABLET_MAX : PHONE_MAX;

You can declare these values in the dimens.xml [1] of various device configurations to avoid having to write out the code. You can retrieve these values like [2]. I'd recommend storing that as a final member variable.

I also don't feel this nightly block works well here. If you want to keep it, we should just return from this whole method early if it's a Nightly build.

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/dimens.xml?rev=65c98a523cf3#213
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=6ddd2771e164#1568

@@ +259,3 @@
>  
> +        mSavedSearches = cr.query(SearchHistory.CONTENT_URI, columns, actualQuery, queryArgs, sortOrderAndLimit);
> +        return mSavedSearches.getCount();

I'd prefer if we didn't use mSavedSearches as a member variable – it becomes difficult to track its open/closed state as it could, in theory, change anywhere in the file. I feel it'd be better to return the Cursor from openAndCountSavedSearches, get the count in updateSuggestions, pass the Cursor into updateFromSavedSearches. The cursor should probably be closed in a try/finally statement in updateSuggestions to ensure it'll always be closed.

@@ +271,5 @@
>          // Set the initial content description.
>          setDescriptionOnSuggestion(mUserEnteredTextView, mUserEnteredTextView.getText().toString());
>  
> +        // Remove this default limit value in Bug 1201325
> +        int limit = 4;

Should this be TABLET_MAX? Why 4?

@@ +274,5 @@
> +        // Remove this default limit value in Bug 1201325
> +        int limit = 4;
> +        if (AppConstants.NIGHTLY_BUILD) {
> +            if (!isTablet) {
> +                limit = PHONE_MAX;

TABLET_MAX isn't used here, so I'm guessing because we show all of results the search provider returns for tablets. That makes me think PHONE_MAX AND TABLET_MAX should be named differently (or there should be two variables for phones like PHONE_MAX_HISTORY_SUGGESTIONS & PHONE_MAX_SEARCH_PROVIDER_SUGGESTIONS). There should probably be a comment explaining the discrepancy of use.

@@ +277,5 @@
> +            if (!isTablet) {
> +                limit = PHONE_MAX;
> +                // If there are less than max saved searches on phones, fill the space with more search engine suggestions
> +                if (savedCount < PHONE_MAX) {
> +                    limit += limit - savedCount;

Why not `limit = PHONE_MAX - savedCount`?

nit: savedCount can be a little more descriptive – maybe historySuggestionCount?

@@ +307,5 @@
>          // This can be called before the opt-in permission prompt is shown or set. Check first.
>          if (suggestionsEnabled) {
>              final int recycledSuggestionCount = mSuggestionView.getChildCount();
> +            boolean isTablet = true;
> +            int savedCount = 0;

Add a comment explaining that these default to values that prevent the UI from changing if it's not a Nightly build so these values don't get updated.

The code pattern is a little unintuitive so I think the comment would be helpful.
Attachment #8658353 - Flags: review?(michael.l.comella) → review+
>You can declare these values in the dimens.xml [1] of various device configurations to avoid having to write out the code. You can retrieve these values like [2]. I'd recommend storing that as a final member variable.

>[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/values/dimens.xml?rev=65c98a523cf3#213

So what configuration is the cut off for tablet? Is it layout-large-v11? 
Looking at mobile/android/base/resources/, I see configurations based on density (drawable-xhdpi) or version (values-v13). It is not at all clear to me which layout corresponds to this cutoff of tablet/not tablet, which is why I did not go this route in this patch. I'm willing to go this route if you wish. Please advise. I'd like to land this no later than tuesday.

 >The cursor should probably be closed in a try/finally statement in updateSuggestions to ensure it'll always be closed.
I added the additional catch like you asked. However, I'm not sure this really buys us much and it definitely makes updateSuggestions harder to read.
Flags: needinfo?(michael.l.comella)
Blocks: 1189719
No longer depends on: 1189719
(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #7)
> So what configuration is the cut off for tablet? Is it layout-large-v11?

Yeah. `*xlarge*` is for ~10" and larger displays (read: tablets) and `*large*` is 7" inch and larger. If an xlarge resource is available, the large resource of the same name will be ignored – if xlarge is not available, large will be used on 10" tablets (and if no large, then the default will be used). Let me know if you have questions about this.

> I'm willing to go this route if
> you wish. Please advise. I'd like to land this no later than tuesday.

If it's too much work, [good next bug] is fine with me.

>  >The cursor should probably be closed in a try/finally statement in
> updateSuggestions to ensure it'll always be closed.
> I added the additional catch like you asked. However, I'm not sure this
> really buys us much and it definitely makes updateSuggestions harder to read.

The Cursor needs to be closed and the best way to do this is with a finally statement. If it's less readable, I'm not sure there's much we can do about that. :\
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/ff72c4a3db5a3088d5edce319a91cb55168811cd
Bug 1200319 - How many, which ones of which type of pills should be displayed on phones or tablets for search suggestions.r=mcomella
Blocks: 1205149
https://hg.mozilla.org/mozilla-central/rev/ff72c4a3db5a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.