Skip to content

Do Not Merge WIP with places autocomplete working in the reference browser #28

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Do Not Merge WIP with places autocomplete working in the reference browser #28

wants to merge 4 commits into from

Conversation

@mhammond
Copy link
Contributor

This PR allows the reference browser to auto-complete from the local places database. All the gory details can be found in README-places.md

While this is a WIP, I'd like some feedback on whether the general shape of this seems reasonable. CC @csadilek @pocmo @linacambridge @thomcc @ncalexan

@mhammond mhammond requested a review from a team as a code owner October 19, 2018 04:42
@st3fan st3fan requested review from csadilek and grigoryk October 23, 2018 23:17
@st3fan
Copy link
Contributor

st3fan commented Oct 23, 2018

I added @csadilek and @grigoryk as reviewers - just note that this is not a final review, more an early feedback review.

st3fan
st3fan reviewed Oct 23, 2018
View reviewed changes
app/src/main/java/org/mozilla/reference/browser/browser/BrowserAutocompleteProvider.kt Outdated
}

// XXX - this should be in the "profile" dir, but I'm not sure how to fetch that.
private val placesConnection = PlacesConnection(context.getExternalFilesDir(null).absolutePath + "/places.sqlite", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

@pocmo do we have a concept of a profile ? Do we need to introduce that?

@mhammond
Copy link
Contributor Author

I've updated mozilla-mobile/android-components#1107 to use the new process which (I think?) we are moving towards.

I'm now getting a strange build error:

AGPBI: {"kind":"error","text":"Program type already present: org.mozilla.places.BuildConfig","sources":[{}],"tool":"D8"}
:app:transformDexArchiveWithExternalLibsDexMergerForGeckoBetaX86Debug FAILED
:app:buildInfoGeneratorGeckoBetaX86Debug
FAILURE: Build failed with an exception.

which might well be something simple - and possibly related to the fact that a-c is now at version 0.29 while this repo still pulls in 0.28. If anyone has clues about this I'd appreciate it but it shouldn't block general review of the shape.

csadilek
csadilek reviewed Oct 24, 2018
View reviewed changes
app/src/main/java/org/mozilla/reference/browser/browser/BrowserAutocompleteProvider.kt
Log.log(tag="BrowserAutocomplete", message=message);
}

class BrowserAutocompleteProvider (
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this type should ultimately live in the components repo. Likely, part of this component:
mozilla-mobile/android-components#1109

Copy link
Contributor

Choose a reason for hiding this comment

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

PlacesConnection could live in the services/sync-places component using the mozilla.components package.

Copy link
Contributor

Choose a reason for hiding this comment

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

PlacesConnection has an unsafe implementation so it should live in a-s. (Although maybe there will be a very similar type in mozilla.components, as with the recent fxa work)

csadilek
csadilek reviewed Oct 24, 2018
View reviewed changes
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.

Just one comment inline.

@grigoryk grigoryk mentioned this pull request Oct 24, 2018
Closed
grigoryk
grigoryk reviewed Oct 24, 2018
View reviewed changes
Copy link
Contributor

@grigoryk grigoryk left a comment •
edited
Loading

Choose a reason for hiding this comment

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

👍 Left a comment on the a-c side of this - mozilla-mobile/android-components#1107 about how we might end up structuring different parts of this.

@mhammond
Copy link
Contributor Author

I've updated this so it works with mozilla-mobile/android-components#1240

@pocmo
Copy link
Contributor

pocmo commented Nov 8, 2018

Closing in favor of #40.

@pocmo pocmo closed this Nov 8, 2018
kpetku pushed a commit to ceno-app/ceno-android that referenced this pull request Jan 13, 2023
@paidforby
Add option to set default search engine in general settings, related to
c8c4f40 
mozilla-mobile#28
kpetku pushed a commit to ceno-app/ceno-android that referenced this pull request Jan 18, 2023
@paidforby
Allow search suggestions to be turned off via settings page, related to
305a3bf 
mozilla-mobile#28 and https://gitlab.com/fdroid/rfp/-/issues/1330, also allows search suggestions to appear on top of homepage
kpetku pushed a commit to ceno-app/ceno-android that referenced this pull request Jan 19, 2023
@paidforby
Remove unwanted search engines from default list on first start, set …
9aa7d54 
…duckduckgo to selected search engine on first start, related to mozilla-mobile#28. Remove unused privacy_settings string for lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@csadilek csadilek csadilek left review comments

+3 more reviewers

@st3fan st3fan st3fan left review comments

@grigoryk grigoryk grigoryk left review comments

@thomcc thomcc thomcc left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mhammond @st3fan @pocmo @grigoryk @csadilek @thomcc