1207876 - Allow search engine suggestion count to be greater than 3 on tablets

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Awesomescreen, defect, P5)

Description

[Sergej Kravcenko]

Search suggestion bar on tablets can show up to 8 suggestions, but search suggestion limit in BrowserSearch.java is hardcoded to 3.

Comment 1

[Allison Naaktgeboren :ally]

(In reply to Sergej Kravcenko from comment #0)
> Search suggestion bar on tablets can show up to 8 suggestions, but search
> suggestion limit in BrowserSearch.java is hardcoded to 3.

yeah 8 total = 
1 user entered search term
3 from default search engine (this is the limit you are referring to in BrowserSearch.java)
4 from search history

Comment 2

[Allison Naaktgeboren :ally]

How would you feel about expanding the number of suggestions from the default search engines on tablets? How strongly do you feel about it?

Comment 3

[Sergej Kravcenko]

It is actually 4 max, I was wrong. On tablets empty saved suggestion slots are not replaced with each engine suggestions. So 4 max can be displayed from search engine. On phones we have 1+2+2 configuration and last 2 saved suggestions can be replaced with search engine suggestions, so we also have 4 max.

The bug is only about default search engine count, it is now hardcoded to 3, but 4 is required.

Comment 4

[Allison Naaktgeboren :ally]

(In reply to Sergej Kravcenko from comment #3)
> It is actually 4 max, I was wrong. On tablets empty saved suggestion slots
> are not replaced with each engine suggestions. So 4 max can be displayed
> from search engine. On phones we have 1+2+2 configuration and last 2 saved
> suggestions can be replaced with search engine suggestions, so we also have
> 4 max.

Yes, there was an ask from Anthony for that behavior to happen on phones, not tablets. I wrote it here

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/SearchEngineRow.java#290

What you're asking for is a new feature as it were.
It's a reasonable enough feature request that I'm willing to accomodate, as long as Anthony agrees.

> 
> The bug is only about default search engine count, it is now hardcoded to 3,
> but 4 is required.

You are correct in your first comment that the original code in BrowserSearch limits the number of search engine suggestions we receive regardless of form factor to 3
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#628

"required" is not the right word. The SearchEngineRow allows up to 4 on tablets in the code even though we know its already currently throttled at 3, because I like my UI not to break/do insane things if someone changes the downstream behavior.

Comment 5

[Sergej Kravcenko]

It's actually correct only if we assume that max suggestion count will not be changed later. Bug 1205149 made these values configurable. BrowserSearch.java hardcoded value must be removed. Correct max count can calculated from values in integer.xml (max_saved_suggestions and max_search_suggestions)

Comment 6

[Allison Naaktgeboren :ally]

(In reply to Sergej Kravcenko from comment #5)
> It's actually correct only if we assume that max suggestion count will not
> be changed later.

It's correct even if we do. You are missing _why_ these things are separated under two different numbers in two different classes.

If the maximums for either tablets or phones are changed in SearchEngineRow that does not mean that we _must_ fetch more, it means we can handle more suggestions gracefully if we decide to increase the number fetched. 

> Bug 1205149 made these values configurable.
> BrowserSearch.java hardcoded value must be removed. Correct max count can
> calculated from values in integer.xml (max_saved_suggestions and
> max_search_suggestions)

There is no _must_ about it. It's idea, a reasonable one, but it's not a regression from specification.

Comment 7

[Sergej Kravcenko]

You are missing my point. I think constants in BrowserSearch and SearchEngineRow are the same. They are UI limits and the first one cancels the second one. SuggestClient should(must) be created with max value that UI can display, not the constant limit based on other principles. 
For example, if my screen allow to display N items, I want to request N items form google search. But if google search has some limitations, then it can return fewer items. I think these limits should be in corresponding search providers, but not in BrowserSearch.

Comment 8

[Allison Naaktgeboren :ally]

(In reply to Sergej Kravcenko from comment #7)
> You are missing my point. I think constants in BrowserSearch and
> SearchEngineRow are the same. 

They are not. I can understand why you think that, but that's not the case.

> They are UI limits and the first one cancels
> the second one. SuggestClient should(must) be created with max value that UI
> can display, not the constant limit based on other principles.
> For example, if my screen allow to display N items, I want to request N
> items form google search. 

You projecting what you would like to see, not what the currently desired behavior is. As I said in comment 4, I'm open to changing the current behavior, but it's not entirely my decision to make.

Comment 9

[Allison Naaktgeboren :ally]

If it's not clear, Anthony needs to buy into the idea too.

Comment 10

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

Hey Sergej,

Let's take a step back here. I'm having trouble understanding what problem we're trying to solve by maxing out our suggestions. 

I agree in that we could show more search suggestion pills in our Tablet UI because of the added real estate. But why do you feel like we need to show more? I am aware of the UI capabilities but I'm also uncomfortable making changes without additional reasoning.

Can you explain a bit about what user value you think this would have?

Comment 11

[Sergej Kravcenko]

Ok, look:

Before patch https://hg.mozilla.org/mozilla-central/rev/ff72c4a3db5a SearchEngineRow didn't limit search suggestions and they were limited by BrowserSearch "SUGGESTION_MAX = 3". Then in Bug 1200319 you have decided that on phones and tablets max saved suggestion count is 4 and patch have added these limits to SearchEngineRow. 
Now we have situation: we can't show 4 suggestions because they are limited to 3 by SUGGESTION_MAX. What happens if you decide to increase pill count even more?

From the other side if you decide to change TABLET_MAX or PHONE_MAX to lower values in the future, we will request unused suggestions from providers and waste speed and network traffic, because you will still request SUGGESTION_MAX count.

From the developer point of view, it's confusing to have 2 constants to control one value. SUGGESTION_MAX must be removed and SuggestClient limit must be calculated from TABLET_MAX or PHONE_MAX values.

Comment 12

[Allison Naaktgeboren :ally]

(In reply to Sergej Kravcenko from comment #11)
> Ok, look:
> 
> Before patch https://hg.mozilla.org/mozilla-central/rev/ff72c4a3db5a
> SearchEngineRow didn't limit search suggestions and they were limited by
> BrowserSearch "SUGGESTION_MAX = 3". Then in Bug 1200319 you have decided
> that on phones and tablets max saved suggestion count is 4 and patch have

The phone limit is 2-3+1 based on saved search
The tablet was 3+1.
There was a miscommunication about whether or not mUserSearchTerm counted as part of search engine suggestions at one point.  Adjusting the code to reuse the constants is fine with me.

If that's what you want to change now, split off a new bug please. I look forward to your patch.

This one can remain about whether or not to fill extra spots of search history with engine suggestions on tablets.

Comment 13

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

I am confused, so I need to work back from basics:

SUGGESTION_MAX = max number of search engine suggestions we want from the search engine. I assume this is also the maximum number we display. If less are returned, we only display the lower number of search suggestions.

TABLET_MAX/PHONE_MAX = max number of saved search terms we will request from the DB. The number could be different depending on tablet or phone. If less are returned, we display the lower number of returned history items. (I find these constants confusing because they don't mention 'search history' in any way)

I assume we will display a maximum of buttons/pills: USER_TYPED + SUGGESTION_MAX + (TABLET_MAX | PHONE_MAX)

Comment 14

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

(In reply to Allison Naaktgeboren please NEEDINFO? :ally from comment #12)

> This one can remain about whether or not to fill extra spots of search
> history with engine suggestions on tablets.

Is that what is bug is about? I had no idea based on the comments.

Comment 15

[Sergej Kravcenko]

TABLET_MAX/PHONE_MAX were used for both saved and remote suggestions. After Bug 1205149 they separated into different constants inside integer.xml. This bug now is all about SUGGESTION_MAX and new constants.

When we are talking about tablets then max suggestion count is "max_search_suggestions" in values-large-v11. When about phones, it's little trickier, because they can be in range from "max_search_suggestions" to "max_search_suggestions + max_saved_suggestions".

So now final visible count is limited by old SUGGESTION_MAX. And if we want to change visible count we must change both constants, the old one and the new. This is why I say that SUGGESTION_MAX is redundant and must be changed to calculated value based on "max_search_suggestions" and "max_saved_suggestions".

Hope it's clear now.

Comment 16

[Firefox Bug Husbandry Bot]

Re-triaging per https://bugzilla.mozilla.org/show_bug.cgi?id=1473195

Needinfo :susheel if you think this bug should be re-triaged.

Comment 17

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.