1207340 - Pref/Search Engine Suggestions UI can get out of sync

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Search Suggestions + Prompt

We don't seem to refresh UI for search suggestions when we change the pref, so we'll get inconsistent UI.

STR:
1. New fennec where opt-in search suggestions UI hasn't been dismissed. (Search suggestions off by default.)
2. Make some searches.
3. Enable search suggestions through settings UI.
4. Go back to awesomescreen and type searches

Expected: Search suggestions, no opt-in prompt
Actual: Search suggestions, as well as opt-in prompt

Comment 1

[Richard Newman [:rnewman]]

Might be a dupe of Bug 1206639, Bug 1206628. Sync up with mcomella.

Comment 2

[Allison Naaktgeboren :ally]

After a quick read through Bug 1206639, Bug 1206628, I think it might be a dupe. What do you think?

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Like bug 1206639 & bug 1206628, I think is a pre-existing bug uncovered by bug 1186683, which no longer creates BrowserSearch each time it is shown, meaning we never refreshed the opt-in prompt's visibility correctly (it should probably be done in onStart, or one of those methods). Prabhjyot was interested in working on the regressions so I'll assign here too.

Prabhjyot, since you're also assigned to the other regressions, let me know if this is too much and I'll try to find someone else to help out! There is about a month and a half to the next merge though so we have time.

Comment 4

[Allison Naaktgeboren :ally]

Prabhjyot, we know that the Search/Awesomescreen code is unintuitive, so please let me know if you have any questions or want feedback.

I look forward to your patch :)

Comment 5

[Prabhjyot Sodhi [:psd]]

Attachment: 1207340.patch

This works, as in, it removes the prompt-in when I search for something after enabling suggestions. However, I have noticed that sometimes there is a delay in the removal of the ViewStub, this could be a problem in slower devices. Any ideas that could help?

Comment 6

[Prabhjyot Sodhi [:psd]]

The uploaded patch must be applied on top of the patch uploaded on bug 1206628.

Comment 7

[Allison Naaktgeboren :ally]

by the by, you need only set either NI or feedback? or review? to get someone's attention on bugzilla. :)

Comment 8

[Allison Naaktgeboren :ally]

Comment on attachment 8672357 [details] [diff] [review]
1207340.patch

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

My chief concern here is that we might be papering over the problem. What other approaches have you tried?

It's also not clear to me how this patch functionally relies on Bug 1206628 to work. Or did you mean simply that this patch bitrots if not applied on top of that patch?

::: mobile/android/base/home/BrowserSearch.java
@@ +649,5 @@
>              updateSearchEngineBar();
>  
>              // Show suggestions opt-in prompt only if suggestions are not enabled yet,
>              // user hasn't been prompted and we're not on a private browsing tab.
> +            // Else remove the opt-in prompt if it has been inflated in the view

nit: you don't need the "Else" in the comment. The rest of the sentence is sufficient. You might want to explain in a sentence why we're removing the prompt if it has already been inflated though.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8672357 [details] [diff] [review]
1207340.patch

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

This works, but I see the prompt flicker on before it disappears. Can we do better?

Perhaps this check should occur in onResume? It feels strange that we change suggestions opt-in visibility in setSearchEngines – should we move that too?

::: mobile/android/base/home/BrowserSearch.java
@@ +649,5 @@
>              updateSearchEngineBar();
>  
>              // Show suggestions opt-in prompt only if suggestions are not enabled yet,
>              // user hasn't been prompted and we're not on a private browsing tab.
> +            // Else remove the opt-in prompt if it has been inflated in the view

I agree w/ Ally – can you explain why we need to remove the prompt?

@@ +654,4 @@
>              if (!mSuggestionsEnabled && !suggestionsPrompted && mSuggestClient != null) {
>                  showSuggestionsOptIn();
>              }
> +            else {

nit: } else {

Comment 10

[Prabhjyot Sodhi [:psd]]

Hey mcomella!
When I tried to check if we want to remove the prompt in onHiddenChanged, I realised that the browser fragment knows if the search has been enabled only when it receives and parses the JSON from gecko([1]) and hence we have to show prompt only after setting the search engines.

[1]: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/home/BrowserSearch.java#574

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

re comment 10, we spoke on irc:

<mcomella> I think if we call for the update [of the search engines] when we change the preference, it would be good and the preload [bug 927692] will take care of the case for first run – does that make sense?

Comment 12

[Allison Naaktgeboren :ally]

hey psd, how's this going?

Comment 13

[Prabhjyot Sodhi [:psd]]

Attachment: 1207340-4.patch

Hey!
This is what I had in mind and tried to do: add a new listener to the checkboxpreference (which changes suggestions enabled) in GeckoPreferences.java which then calls, asks for an update from Gecko, i.e: SearchEngines:GetVisible, which updates and data and removes the prompt if necessary.

I am stuck at a runtime error (Not able to addPreferences before super.onCreate), while trying to add the listener in GeckoPreferences.java, but more than that, I have some doubts about this algorithm, to solve this.

Comment 14

[Prabhjyot Sodhi [:psd]]

So,
After discussion on IRC, we have concluded that removing the flicker by adding in listeners or trying some other solutions seem overly complicated and not worth it.
I'll upload a patch (which flickers :( ) but fixes this and bug 1216826.

Comment 15

[Prabhjyot Sodhi [:psd]]

Attachment: 1207340-5.patch

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I'll get to this tomorrow, Prabhjyot.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8684333 [details] [diff] [review]
1207340-5.patch

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

Looks good to me! Thanks Prabhjyot!

Make sure you update the commit message from the try text.

Comment 18

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c08c62230baa

Comment 19

[Prabhjyot Sodhi [:psd]]

ah! Realized my mistake! working on correcting it!

Comment 20

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=05048c16b909

Comment 21

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

patching file mobile/android/base/home/BrowserSearch.java
Hunk #2 FAILED at 666
1 out of 3 hunks FAILED -- saving rejects to file mobile/android/base/home/BrowserSearch.java.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh 1207340-5.patch

Comment 22

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa7648d89950

Comment 23

[Prabhjyot Sodhi [:psd]]

Attachment: 1207340-6.patch

rebased!

Comment 24

[Prabhjyot Sodhi [:psd]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5567a7f76ca

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d70d6466d7eb

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Prabhjyot, can you flag this for uplift to Aurora?

Comment 27

[Prabhjyot Sodhi [:psd]]

Comment on attachment 8687123 [details] [diff] [review]
1207340-6.patch

Approval Request Comment
[Feature/regressing bug #]:bug 1186683
[User impact if declined]: User shown prompt even after switching on suggestions
[Describe test coverage new/current, TreeHerder]: Tested manually, visual testing required
[Risks and why]: Low risk - it is a simple change, removing the suggestions prompt when it should not be present
[String/UUID change made/needed]:None

Comment 28

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d70d6466d7eb

Comment 29

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Chenxia, could you please verify this issue is fixed as expected on the latest Nightly build? Thanks.

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8687123 [details] [diff] [review]
1207340-6.patch

This has been on Nightly for a few days now, seems safe to uplift to Aurora44.

Comment 31

[Chenxia Liu [:liuche]]

Ritu, this works for me on the most recent Nightly. I also see the flash that mcomella mentioned in comment #9, but it sounds like we are okay with that.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3ac88e18ff69

Comment 33

[Teodora Vermesan (:TeoVermesan)]

With search suggestions disabled, make a search. Search Suggestion opt-in prompt is displayed: "Would you like to turn on search suggestions?". Enable search suggestions and make a new search.  The Search Suggestion opt-in prompt is not displayed anymore:
Verified as fixes using:
Device: Moto X (Android 4.4)
Build: Firefox for Android 45.0a1 (2015-11-22)

Comment 34

[Teodora Vermesan (:TeoVermesan)]

With search suggestions disabled, make a search. Search Suggestion opt-in prompt is displayed: "Would you like to turn on search suggestions?". Enable search suggestions from settings and make a new search.  The Search Suggestion opt-in prompt is not displayed anymore:
Verified as fixes using:
Device: Nexus 6 (Android 5.1.1)
Build: Firefox for Android 44.0a2 (2015-11-25)