Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds custom search engines #6551

Merged
merged 25 commits into from Nov 20, 2019

Conversation

@boek
Copy link
Member

boek commented Nov 11, 2019

image

image

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Screenshots: This PR includes screenshots or GIFs of the changes made or an explanation of why it does not
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues finished by this pull request are added to the milestone of the version currently in development.

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
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Nov 11, 2019

Codecov Report

Merging #6551 into master will decrease coverage by 0.17%.
The diff coverage is 12.33%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master   #6551      +/-   ##
===========================================
- Coverage     17.18%     17%   -0.18%     
- Complexity      365     376      +11     
===========================================
  Files           272     279       +7     
  Lines         10779   11208     +429     
  Branches       1516    1573      +57     
===========================================
+ Hits           1852    1906      +54     
- Misses         8771    9143     +372     
- Partials        156     159       +3
Impacted Files Coverage Δ Complexity Δ
...x/search/awesomebar/ShortcutsSuggestionProvider.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
.../mozilla/fenix/search/awesomebar/AwesomeBarView.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
.../mozilla/fenix/settings/search/SearchEngineMenu.kt 0% <0%> (ø) 0 <0> (?)
...lla/fenix/settings/search/SearchStringValidator.kt 0% <0%> (ø) 0 <0> (?)
...illa/fenix/settings/search/SearchEngineFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
.../settings/search/EditCustomSearchEngineFragment.kt 0% <0%> (ø) 0 <0> (?)
...a/fenix/settings/search/AddSearchEngineFragment.kt 0% <0%> (ø) 0 <0> (?)
...c/main/java/org/mozilla/fenix/home/HomeFragment.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
...enix/components/searchengine/SearchEngineWriter.kt 0% <0%> (ø) 0 <0> (?)
...enix/settings/search/SearchEngineListPreference.kt 0% <0%> (ø) 0 <0> (ø) ⬇️
... and 12 more

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 8abf580...989c3b4. Read the comment docs.

@boek boek force-pushed the boek:i5577 branch 6 times, most recently from 7534b64 to 0f14f76 Nov 13, 2019
@boek boek marked this pull request as ready for review Nov 15, 2019
@boek boek force-pushed the boek:i5577 branch 2 times, most recently from 4478318 to e9e6105 Nov 18, 2019
@ekager ekager self-requested a review Nov 18, 2019
Copy link
Member

ekager left a comment

Just some nits

@boek boek force-pushed the boek:i5577 branch from 94cb009 to e778948 Nov 19, 2019
app/build.gradle Show resolved Hide resolved
@boek boek requested review from NotWoods and ekager Nov 19, 2019
Copy link
Member

NotWoods left a comment

I resolved most of my earlier comments but I'd still like to see this using concept-fetch. Ideally also use a recycler view because the view inflation code gets hard to follow for the list.

@jonalmeida

This comment has been minimized.

Copy link
Member

jonalmeida commented Nov 19, 2019

I resolved most of my earlier comments but I'd still like to see this using concept-fetch.

This is a good catch. I would assert that we need to use concept-fetch. See #5599.

@boek boek requested a review from NotWoods Nov 19, 2019
Copy link
Member

NotWoods left a comment

Looks good, can you fix these nits and add (Dispatchers.Main) to the launch calls?

@boek boek force-pushed the boek:i5577 branch from 79f2ca7 to cd0eabf Nov 19, 2019
@boek boek force-pushed the boek:i5577 branch from cd0eabf to 378ace0 Nov 19, 2019
Copy link
Member

NotWoods left a comment

Oops, just caught something else

@boek boek requested a review from NotWoods Nov 19, 2019
@boek boek merged commit 607c3d4 into mozilla-mobile:master Nov 20, 2019
7 of 9 checks passed
7 of 9 checks passed
pr-complete FirefoxCI (pull_request)
Details
test-ui FirefoxCI (pull_request)
Details
Decision Task FirefoxCI (pull_request)
Details
build-debug FirefoxCI (pull_request)
Details
lint-compare-locales FirefoxCI (pull_request)
Details
lint-detekt FirefoxCI (pull_request)
Details
lint-ktlint FirefoxCI (pull_request)
Details
lint-lint FirefoxCI (pull_request)
Details
test-debug FirefoxCI (pull_request)
Details
@boek boek deleted the boek:i5577 branch Nov 20, 2019
@pocmo

This comment has been minimized.

Copy link
Contributor

pocmo commented Nov 21, 2019

@boek This seems to duplicate a bunch of code that already exists in AC (CustomSearchEngineProvider, CustomSearchEngineStore, ..). Did you need to do any modifications to them or what was the reason for duplicating them? Can you please open issue if there are missing features in the AC implementations. Duplicating this functionality is not viable.

@pocmo

This comment has been minimized.

Copy link
Contributor

pocmo commented Nov 21, 2019

Sorry, I was completely in the wrong place. This has not landed in AC (I thought we had ported that over from Focus). Please continue. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.