Closed Bug 1158275 Opened 7 years ago Closed 7 years ago

Remove old method of selecting non-primary search engines in BrowserSearch

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox41 verified)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- verified

People

(Reporter: mcomella, Assigned: sebastian)

References

Details

Attachments

(3 files, 2 obsolete files)

Follow-up to bug 1137483.

i.e. the list should only include the primary search engine, its suggestions, and awesomescreen results.

This should also remove the primary search engine from the quick search bar.
Assignee: nobody → s.kaspari
Attached patch 1158275_searchengines.patch (obsolete) — Splinter Review
This patch will only add "getSuggestEngineCount()" search engines to the list (So theoretically there can be more than one). This patch does not yet remove the primary search engine(s) from the new SearchBar. I'll attach a second patch for that.
Attachment #8613802 - Flags: review?(michael.l.comella)
Attached image search-empty-state.png
The initial state of the search view looks kinda empty now (see attached screenshot) - with suggestions not (yet) enabled and empty history.
Attached patch 1158275_searchengines_v2.patch (obsolete) — Splinter Review
Updated patch that also handles the search bar.
Attachment #8613802 - Attachment is obsolete: true
Attachment #8613802 - Flags: review?(michael.l.comella)
Attachment #8613829 - Flags: review?(michael.l.comella)
Seems like testAddSearchEngine will need some love now.
Status: NEW → ASSIGNED
Comment on attachment 8613829 [details] [diff] [review]
1158275_searchengines_v2.patch

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

Nice cleanup of ROW_SEARCH!

WFM!

::: mobile/android/base/home/BrowserSearch.java
@@ +348,5 @@
> +        mSearchEngineBar.setSearchEngines(
> +                mSearchEngines.size() > getSuggestEngineCount()
> +                ? mSearchEngines.subList(getSuggestEngineCount(), mSearchEngines.size())
> +                : new ArrayList<SearchEngine>()
> +        );

nit: I haven't seen the trailing paren on a newline by itself style before in our code, but it wfm.

@@ +587,5 @@
>              if (mAdapter != null) {
>                  mAdapter.notifyDataSetChanged();
>              }
>  
> +            mSearchEngineBar.setSearchEngines(

nit: Perhaps this should be a helper function.

@@ +881,5 @@
>              final int resultCount = super.getCount();
>  
>              // Don't show search engines or suggestions if search field is empty
>              if (TextUtils.isEmpty(mSearchTerm)) {
>                  return resultCount;

Some of the code in this file seems to be legacy - we never show the browser search screen if there is no search term entered. I wonder if we can't greatly simplify this file by removing these other cases (e.g. getSuggestEngineCount() returns a constant because we have that hard-coded atm) - file a followup?
Attachment #8613829 - Flags: review?(michael.l.comella) → review+
This patch fixes a bug where selecting a search result with suggestions off might select the wrong row. In addition to that getSuggestEngineCount() is now gone and all the mSearchEngines size checking is now hidden behind getPrimaryEngineCount(). To avoid further confusion with patches that depend on each other in weird ways, this patch now includes the changes for bug 1158280 as well.
Attachment #8613829 - Attachment is obsolete: true
Attachment #8614381 - Flags: review?(michael.l.comella)
Comment on attachment 8614381 [details] [diff] [review]
1158275_searchengines_v3.patch

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

::: mobile/android/base/home/BrowserSearch.java
@@ +585,5 @@
>              if (mAdapter != null) {
>                  mAdapter.notifyDataSetChanged();
>              }
>  
> +            mSearchEngineBar.setSearchEngines(

nit: maybe this should be a helper function.

(But I see this was done in https://bugzilla.mozilla.org/show_bug.cgi?id=1170824#c1)
Attachment #8614381 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8615555 [details] [diff] [review]
1158275_testAddSearchEngine.patch

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

::: mobile/android/tests/browser/robocop/testAddSearchEngine.java
@@ +145,5 @@
>          mActions.sendKeys(SEARCH_TEXT);
>          boolean correctNumSearchEnginesDisplayed = waitForCondition(new Condition() {
>              @Override
>              public boolean isSatisfied() {
> +                ListView searchResultList = findListViewWithTag(HomePager.LIST_TAG_BROWSER_SEARCH);

Nice rename. :) I like cleanup.
Attachment #8615555 - Flags: review?(michael.l.comella) → review+
Nice job finding a low entropy way to make all the old search engine methodology go away!
Duplicate of this bug: 1158280
Verified as fixed in build 41.0a1 2015-06-15;
Device: Samsung Galaxy Tab 10.1 P7510 (Android 4.0.4).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.