Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

For #17377 - Fix for empty awesomeBar on searchFragment open #17435

Conversation

codrut-topliceanu
Copy link
Contributor

The awesomeBar was set to stay hidden for the first consumeFrom(store) of the SearchDialogFragment.kt. However, recent changes send an additional store update when the view is created. To work around it we can take a look at AwesomeBarView.update(), we see that the awesomeBar suggestions get provided only if the query != url. So we can use the same method to keep the awesomeBar hidden until the user changes anything in the url.

Pull Request checklist

  • Tests: This PR includes no tests as it is only a minor change
  • Screenshots: This PR includes screenshots or GIFs
  • Accessibility: The code in this PR follows accessibility best practices.

To download an APK when reviewing a PR:

  1. click on Show All Checks,
  2. click Details next to "Taskcluster (pull_request)" after it appears and then finishes with a green checkmark,
  3. click on the "Fenix - assemble" task, then click "Run Artifacts".
  4. the APK links should be on the left side of the screen, named for each CPU architecture
awesomebarfix.mp4

@codrut-topliceanu codrut-topliceanu force-pushed the 17377_blank_awesomebar_on_first_open branch from 9044707 to a9f0044 Compare January 15, 2021 08:09
@codrut-topliceanu
Copy link
Contributor Author

codrut-topliceanu commented Jan 15, 2021

Pushed a fix for the case where the awesomebar would disappear if ever the user would type in the address bar the same url the tab has.

@codrut-topliceanu
Copy link
Contributor Author

codrut-topliceanu commented Jan 15, 2021

I've found two fixes for this issue.
1st one is in the code for this PR, and works by hiding the Awesomebar when SearchDialogFragment is first opened and it keeps it hidden until the query and url are different. (as mentioned in the commit msg).
Here it is in action:
Fenix_shouldShowAwesomebar

2nd one would be in AC in BrowserAwesomebar.kt at the end of the queryProvidersForSuggestions() function and works by hiding the awesomebar if the suggestion adapter is empty.

      job?.invokeOnCompletion {
           this@BrowserAwesomeBar.visibility =
               if (suggestionsAdapter.itemCount == 0) View.INVISIBLE else View.VISIBLE
       }

And here is a GIF of it in action:
AC_job_invokeOnCompletion

The 1st solution, in Fenix, is a lot more responsive but not as "elegant" as it uses a rather complex if case over there.
The 2nd solution, in AC, is a lot more clear and fixes the root of the problem: no need to show any awesomebar if there isn't anything to show in it. However it is a bit slower as it gets invoked after the job that queries the providers for suggestions gets called. While we do have a time limit on those, it might still seem noticeable to keen eyed users. (If you look carefully at the two GIFs above you can see that the first one displays the awesomebar and then it gets populated with suggestions).

@codrut-topliceanu codrut-topliceanu added the needs:review PRs that need to be reviewed label Jan 15, 2021
@csadilek csadilek self-assigned this Jan 18, 2021
@csadilek csadilek self-requested a review January 18, 2021 23:08
Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine to fix this here as it's more responsive, as you wrote, and would need a bit more thought in A-C e.g. is it always the right default to hide the awesomebar if there are no suggestions or do we need to make this configurable?

So +1 to landing this. It regressed a while ago and is already in beta as well, so we should consider uplifting this.

One nit / potential simplification below. It would also be nice to cover this with a unit test, but in this case it can be a follow-up ticket.

… open

The awesomeBar was set to stay hidden for the first consumeFrom(store) if the SearchDialogFragment.kt. However, recent changes send an additional store update when the view is created. To work around it we can take a look at AwesomeBarView.update(), we see that the awesomeBar suggestions get provided only if the query != url. So we can use the same method to keep the awesomeBar hidden until the user changes anything in the url.
@codrut-topliceanu codrut-topliceanu force-pushed the 17377_blank_awesomebar_on_first_open branch from a9f0044 to 05a08cb Compare January 20, 2021 11:30
@codecov-io
Copy link

Codecov Report

Merging #17435 (05a08cb) into master (00d971e) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #17435      +/-   ##
============================================
+ Coverage     32.27%   32.30%   +0.03%     
  Complexity     1289     1289              
============================================
  Files           456      456              
  Lines         18946    18925      -21     
  Branches       2642     2637       -5     
============================================
  Hits           6114     6114              
+ Misses        12348    12327      -21     
  Partials        484      484              
Impacted Files Coverage Δ Complexity Δ
...a/org/mozilla/fenix/search/SearchDialogFragment.kt 0.00% <ø> (ø) 0.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00d971e...05a08cb. Read the comment docs.

@codrut-topliceanu codrut-topliceanu merged commit 20f2135 into mozilla-mobile:master Jan 20, 2021
@codrut-topliceanu codrut-topliceanu removed the needs:review PRs that need to be reviewed label Jan 20, 2021
pkirakosyan pushed a commit to gexsi/user-agent-android that referenced this pull request Aug 4, 2021
… open (mozilla-mobile#17435)

The awesomeBar was set to stay hidden for the first consumeFrom(store) if the SearchDialogFragment.kt. However, recent changes send an additional store update when the view is created. To work around it we can take a look at AwesomeBarView.update(), we see that the awesomeBar suggestions get provided only if the query != url. So we can use the same method to keep the awesomeBar hidden until the user changes anything in the url.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants