Closed Bug 1210534 Opened 4 years ago Closed 4 years ago

Search suggestions are not refreshed after single character entered into search field

Categories

(Firefox for Android :: Awesomescreen, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox44 --- verified
firefox45 --- verified
b2g-v2.5 --- fixed
fennec 44+ ---

People

(Reporter: sergej, Assigned: sergej)

References

Details

Attachments

(1 file)

Steps to reproduce.

1. Press address bar on home screen
2. Enter any single letter and observe search results based on that letter
3. Hit backspace and delete that letter
4. Enter any other single letter and observe same search suggestions from step 2

Expected: Search suggestions refresh.
Thanks for reporting. Sergej, do you want to look into writing a patch for this?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(sergej)
I'll take a look
Flags: needinfo?(sergej)
Assignee: nobody → sergej
I expect this is another bug 1186683 regression.
Blocks: 1186683
tracking-fennec: --- → 44+
Bug 1210534 - Search suggestions are not refreshed after single character entered into search field
Attachment #8670339 - Flags: review?(michael.l.comella)
Sergej, sorry for the delay – I'll get to this tomorrow.
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review19489

I investigated and found the issue to be `BrowserSearch.filter` is called as expected but `isVisible()` [1] unexpectedly returns false (presumably because the `showBrowserSearch` call hasn't had time to propagate yet.

Your solution seems overly complex given the analysis above – is it possible to do something simpler? Maybe we can recall `BrowserSearch.filter` with the previously saved `mSearchTerm` in `onHiddenChanged` (assuming `add` and the other requirements for `isVisible` have taken affect). Or we can add a different condition to replace `isVisible`.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java?rev=aa701b608df5#795
Attachment #8670339 - Flags: review?(michael.l.comella)
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Bug 1210534 - Search suggestions are not refreshed after single character entered into search field
Attachment #8670339 - Flags: review?(michael.l.comella)
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review19769

::: mobile/android/base/home/BrowserSearch.java:195
(Diff revision 2)
> +        mCursorLoaderCallbacks = new CursorLoaderCallbacks();

Nice cleanup!

::: mobile/android/base/home/BrowserSearch.java:798
(Diff revision 2)
> -        if (isVisible()) {
> +        if (mAdapter != null) {

I find just this change fixes my issues – what problems do the other changes solve?
Attachment #8670339 - Flags: review?(michael.l.comella)
It solves initial load when fragment is created. Now it's working because on first load, fragment requests search engines from gecko and resend query. But Bug 927692 will fix this, and there is possibility that filter method will be called before adapter is available.
I see. In order to prevent confusion, we generally try to scope different fixes/features into new bugs (e.g. bug 927692) with the exception to small cleanups (e.g. whitespace & readability). Unfortunately, someone else is assigned to the preload search engines bug so we should probably let them work through it. It's worth noting that your solution looks quite elegant, however. :)

Do you think you can trim down this patch to just refresh the search engines after a single character is typed?
Flags: needinfo?(sergej)
Attachment #8670339 - Flags: review?(michael.l.comella)
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Bug 1210534 - Search suggestions are not refreshed after single character entered into search field
Here you go. I think we must leave comment in bug 927692, to warn that adapter can be null, when fragment is created and filter method runs first before onActivityCreated.
Flags: needinfo?(sergej)
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

https://reviewboard.mozilla.org/r/21361/#review21107

Sorry for the delay on this Sergej – this looks good to me.

Merge got moved to tomorrow so do you mind flagging this for uplift to aurora?
Attachment #8670339 - Flags: review?(michael.l.comella) → review+
(In reply to Sergej Kravcenko from comment #12)
> Here you go. I think we must leave comment in bug 927692, to warn that
> adapter can be null, when fragment is created and filter method runs first
> before onActivityCreated.

Would you like to go ahead and do this? It might be good to add a comment somewhere for this too.
Didn't realize the push in comment 15 didn't have an `r=` – I reviewed it.
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

Approval Request Comment
[Feature/regressing bug #]: bug 1186683.
[User impact if declined]:
  Users who open the toolbar search, hide the search screen, and enter a single character to show it again will get old results.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: Low. Before we updated the search screen only when it was visible – now we do it as long as the memory we need to access is not null.

[String/UUID change made/needed]: None
Attachment #8670339 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/0930afdb8a61
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment on attachment 8670339 [details]
MozReview Request: Bug 1210534 - Search suggestions are not refreshed after single character entered into search field

This seems like a safe fix, let's uplift to Aurora44.
Attachment #8670339 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
After you enter a single letter, delete it and enter another single letter, search suggestions are refreshed.

Verified as fixed using:
Device: Xiaomi Mi i4 (Android 5.0.2)
Build: Firefox for Android 45.0a1 (2015-11-11)
After you enter a single letter, delete it and enter another single letter, search suggestions are refreshed.

Verified as fixed using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 44.0a2 (2015-11-23)
You need to log in before you can comment on or make changes to this bug.