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

VERIFIED FIXED in Firefox 41

Status

()

VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: sebastian)

Tracking

unspecified
Firefox 41
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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)

Updated

4 years ago
Assignee: nobody → s.kaspari
(Assignee)

Comment 1

4 years ago
Created attachment 8613802 [details] [diff] [review]
1158275_searchengines.patch

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)
(Assignee)

Comment 2

4 years ago
Created attachment 8613803 [details]
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.
(Assignee)

Comment 3

4 years ago
Created attachment 8613829 [details] [diff] [review]
1158275_searchengines_v2.patch

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)
(Assignee)

Comment 5

4 years ago
Seems like testAddSearchEngine will need some love now.
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Blocks: 1158280
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+
(Assignee)

Comment 7

4 years ago
Created attachment 8614381 [details] [diff] [review]
1158275_searchengines_v3.patch

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)
(Assignee)

Comment 9

4 years ago
Created attachment 8615555 [details] [diff] [review]
1158275_testAddSearchEngine.patch

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d42a927871b
Attachment #8615555 - 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!
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Duplicate of this bug: 1158280
https://hg.mozilla.org/mozilla-central/rev/8e5abcb55c35
https://hg.mozilla.org/mozilla-central/rev/d7ed7208e7f4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Comment 16

4 years ago
Verified as fixed in build 41.0a1 2015-06-15;
Device: Samsung Galaxy Tab 10.1 P7510 (Android 4.0.4).
Status: RESOLVED → VERIFIED
status-firefox41: fixed → verified
You need to log in before you can comment on or make changes to this bug.