Closed
Bug 1210534
Opened 9 years ago
Closed 9 years ago
Search suggestions are not refreshed after single character entered into search field
Categories
(Firefox for Android Graveyard :: Awesomescreen, defect)
Firefox for Android Graveyard
Awesomescreen
Tracking
(firefox44 verified, firefox45 verified, b2g-v2.5 fixed, fennec44+)
RESOLVED
FIXED
Firefox 45
People
(Reporter: sergej, Assigned: sergej)
References
Details
Attachments
(1 file)
40 bytes,
text/x-review-board-request
|
mcomella
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
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.
Comment 1•9 years ago
|
||
Thanks for reporting. Sergej, do you want to look into writing a patch for this?
Updated•9 years ago
|
Assignee: nobody → sergej
I expect this is another bug 1186683 regression.
Blocks: 1186683
tracking-fennec: --- → 44+
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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)
Assignee | ||
Comment 9•9 years ago
|
||
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)
Assignee | ||
Updated•9 years ago
|
Attachment #8670339 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 11•9 years ago
|
||
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
Assignee | ||
Comment 12•9 years ago
|
||
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.
https://hg.mozilla.org/integration/fx-team/rev/0930afdb8a61cd130ffe7d6276d2cbd0c1c2ca4d
Bug 1210534 - Search suggestions are not refreshed after single character entered into search field
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?
Comment 18•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Comment 19•9 years ago
|
||
bugherder uplift |
status-b2g-v2.5:
--- → fixed
status-firefox44:
--- → affected
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+
Comment 21•9 years ago
|
||
bugherder uplift |
Comment 22•9 years ago
|
||
bugherder uplift |
Comment 23•9 years ago
|
||
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)
Comment 24•9 years ago
|
||
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)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•