Add and expose a getBookmarkURLForKeyword API.
#1345
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomcc
reviewed
Jul 1, 2019
| @@ -889,6 +889,10 @@ impl<'de> Deserialize<'de> for BookmarkTreeNode { | |||
| } | |||
| } | |||
|
|
|||
| pub fn bookmarks_get_url_for_keyword(db: &PlacesDb, keyword: &str) -> Result<Option<Url>> { | |||
| Ok(None) | |||
There was a problem hiding this comment.
@linacambridge I'm assuming you just never got to this?
@tinoism If you're interested in finishing this up, this function will need to be completed. (also check with Lina first!)
There was a problem hiding this comment.
We chatted a bit on Slack! 😄 Thanks for taking this over, @tinoism!
thomcc
changed the title
getBookmarkURLForKeyword API.
tinoism
force-pushed
the
ios-keywords
branch
5 times, most recently
from
d2a4d92
to
04035f8
Compare
linabutler
approved these changes
Jul 24, 2019
There was a problem hiding this comment.
This looks great @tinoism, thanks so much for taking it over the finish line! 🏁 I wouldn't mind seeing your patch again with rustCallForOptString, but feel free to file a follow-up if you prefer!
components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt
Outdated
Show resolved
Hide resolved
| .expect("should work"); | ||
| let place_id = conn.last_insert_rowid(); | ||
|
|
||
| // create a bookmark with keyword 'donut' pointing at it. |
linabutler
reviewed
Jul 24, 2019
components/places/android/src/main/java/mozilla/appservices/places/Bookmarks.kt
Outdated
Show resolved
Hide resolved
Rust Places doesn't support setting keywords for URLs, and it's unclear if we'll ever port keywords as they exist on Desktop today. However, we do sync and round-trip keywords set on Desktop, so we can read them directly from the `moz_bookmarks_synced` table. Closes mozilla#953.
thomcc
reviewed
Jul 25, 2019
components/places/android/src/main/java/mozilla/appservices/places/PlacesConnection.kt
Outdated
Show resolved
Hide resolved
|
Thanks @tinoism! |
🎊
linabutler
added a commit
that referenced
this pull request
Jan 21, 2020
Before this patch, we stored keywords for incoming bookmarks in the `moz_bookmarks_synced` table, and read them back out from that table when we uploaded outgoing records. This was done for a few reasons: * In the beginning, Rust Places didn't support search keywords at all. There are plans to unify keywords and custom search engines, since they solve the same problem, but it's not currently a priority. https://bugzilla.mozilla.org/show_bug.cgi?id=648398 has more context. * However, keyword searches were a very popular feature in Firefox for iOS, so we added an API to read them out of `moz_bookmarks_synced` in #1345. * This was still fine, since the only way to get keywords into Fenix and iOS was via Sync. (Though there were some gotchas; for instance, disconnecting Sync would lose all keywords). * But the Fennec to Fenix migration meant that there are now two ways to get keywords into Fenix, since Fennec supported setting them on bookmarks. To make things more interesting, keywords are handled differently across our products. Desktop exposes keywords as bookmark properties, but, internally, associates them with (URL, POST data) tuples. (This is also why they're closer to custom search engines than bookmarks). But Sync, Fennec, and the old (before a-s) Firefox for iOS associated them with individual bookmarks. This patch takes a middle ground. Keywords are associated with URLs, like Desktop, and we store those associations in a separate `moz_keywords` table. This means keywords imported from Fennec should be migrated, and signing out of Sync in Firefox for iOS no longer loses keywords. Like Fennec and iOS, we don't support POST data, since we don't sync it, and there's no UI for setting it. The keyword syncing code is the same as Desktop's: we remove all synced keywords from all local URLs, and all local keywords from old and new synced URLs, then apply incoming items with new keywords and URLs. We also flag items with the same keyword and different URLs, as well as items with different keywords and the same URL, for reupload, to enforce the "1 keyword per URL" association.
4 tasks
linabutler
added a commit
that referenced
this pull request
Mar 10, 2020
Before this patch, we stored keywords for incoming bookmarks in the `moz_bookmarks_synced` table, and read them back out from that table when we uploaded outgoing records. This was done for a few reasons: * In the beginning, Rust Places didn't support search keywords at all. There are plans to unify keywords and custom search engines, since they solve the same problem, but it's not currently a priority. https://bugzilla.mozilla.org/show_bug.cgi?id=648398 has more context. * However, keyword searches were a very popular feature in Firefox for iOS, so we added an API to read them out of `moz_bookmarks_synced` in #1345. * This was still fine, since the only way to get keywords into Fenix and iOS was via Sync. (Though there were some gotchas; for instance, disconnecting Sync would lose all keywords). * But the Fennec to Fenix migration meant that there are now two ways to get keywords into Fenix, since Fennec supported setting them on bookmarks. To make things more interesting, keywords are handled differently across our products. Desktop exposes keywords as bookmark properties, but, internally, associates them with (URL, POST data) tuples. (This is also why they're closer to custom search engines than bookmarks). But Sync, Fennec, and the old (before a-s) Firefox for iOS associated them with individual bookmarks. This patch takes a middle ground. Keywords are associated with URLs, like Desktop, and we store those associations in a separate `moz_keywords` table. This means keywords imported from Fennec should be migrated, and signing out of Sync in Firefox for iOS no longer loses keywords. Like Fennec and iOS, we don't support POST data, since we don't sync it, and there's no UI for setting it. The keyword syncing code is the same as Desktop's: we remove all synced keywords from all local URLs, and all local keywords from old and new synced URLs, then apply incoming items with new keywords and URLs. We also flag items with the same keyword and different URLs, as well as items with different keywords and the same URL, for reupload, to enforce the "1 keyword per URL" association. Finally, we prepopulate the `moz_keywords` table with existing synced keywords, so that they can be accessed via `bookmarks_get_url_for_keyword` without triggering a full sync.
linabutler
added a commit
that referenced
this pull request
Mar 10, 2020
Before this patch, we stored keywords for incoming bookmarks in the `moz_bookmarks_synced` table, and read them back out from that table when we uploaded outgoing records. This was done for a few reasons: * In the beginning, Rust Places didn't support search keywords at all. There are plans to unify keywords and custom search engines, since they solve the same problem, but it's not currently a priority. https://bugzilla.mozilla.org/show_bug.cgi?id=648398 has more context. * However, keyword searches were a very popular feature in Firefox for iOS, so we added an API to read them out of `moz_bookmarks_synced` in #1345. * This was still fine, since the only way to get keywords into Fenix and iOS was via Sync. (Though there were some gotchas; for instance, disconnecting Sync would lose all keywords). * But the Fennec to Fenix migration meant that there are now two ways to get keywords into Fenix, since Fennec supported setting them on bookmarks. To make things more interesting, keywords are handled differently across our products. Desktop exposes keywords as bookmark properties, but, internally, associates them with (URL, POST data) tuples. (This is also why they're closer to custom search engines than bookmarks). But Sync, Fennec, and the old (before a-s) Firefox for iOS associated them with individual bookmarks. This patch takes a middle ground. Keywords are associated with URLs, like Desktop, and we store those associations in a separate `moz_keywords` table. This means keywords imported from Fennec should be migrated, and signing out of Sync in Firefox for iOS no longer loses keywords. Like Fennec and iOS, we don't support POST data, since we don't sync it, and there's no UI for setting it. The keyword syncing code is the same as Desktop's: we remove all synced keywords from all local URLs, and all local keywords from old and new synced URLs, then apply incoming items with new keywords and URLs. We also flag items with the same keyword and different URLs, as well as items with different keywords and the same URL, for reupload, to enforce the "1 keyword per URL" association. Finally, we prepopulate the `moz_keywords` table with existing synced keywords, so that they can be accessed via `bookmarks_get_url_for_keyword` without triggering a full sync.
linabutler
added a commit
that referenced
this pull request
Mar 11, 2020
Before this patch, we stored keywords for incoming bookmarks in the `moz_bookmarks_synced` table, and read them back out from that table when we uploaded outgoing records. This was done for a few reasons: * In the beginning, Rust Places didn't support search keywords at all. There are plans to unify keywords and custom search engines, since they solve the same problem, but it's not currently a priority. https://bugzilla.mozilla.org/show_bug.cgi?id=648398 has more context. * However, keyword searches were a very popular feature in Firefox for iOS, so we added an API to read them out of `moz_bookmarks_synced` in #1345. * This was still fine, since the only way to get keywords into Fenix and iOS was via Sync. (Though there were some gotchas; for instance, disconnecting Sync would lose all keywords). * But the Fennec to Fenix migration meant that there are now two ways to get keywords into Fenix, since Fennec supported setting them on bookmarks. To make things more interesting, keywords are handled differently across our products. Desktop exposes keywords as bookmark properties, but, internally, associates them with (URL, POST data) tuples. (This is also why they're closer to custom search engines than bookmarks). But Sync, Fennec, and the old (before a-s) Firefox for iOS associated them with individual bookmarks. This patch takes a middle ground. Keywords are associated with URLs, like Desktop, and we store those associations in a separate `moz_keywords` table. This means keywords imported from Fennec should be migrated, and signing out of Sync in Firefox for iOS no longer loses keywords. Like Fennec and iOS, we don't support POST data, since we don't sync it, and there's no UI for setting it. The keyword syncing code is the same as Desktop's: we remove all synced keywords from all local URLs, and all local keywords from old and new synced URLs, then apply incoming items with new keywords and URLs. We also flag items with the same keyword and different URLs, as well as items with different keywords and the same URL, for reupload, to enforce the "1 keyword per URL" association. Finally, we prepopulate the `moz_keywords` table with existing synced keywords, so that they can be accessed via `bookmarks_get_url_for_keyword` without triggering a full sync.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #953.