Add a Places API to recursively count the number of descendants in a … [firefox-android: mhammond/count-bookmarks-in-trees] #5853
Conversation
mhammond
force-pushed
the
count_in_tree
branch
2 times, most recently
from
9e05753 to
e88f6a9
Compare
mhammond
changed the title
mhammond
force-pushed
the
count_in_tree
branch
from
e88f6a9 to
38c6ba2
Compare
| * @param guids The guids of folders to query. | ||
| * @return Count of all bookmark items (ie, not folders or separators) in all specified folders recursively. | ||
| * Empty folders, non-existing GUIDs and non-existing items will return zero. | ||
| * The result is implementation dependant if the trees overlap. |
There was a problem hiding this comment.
Thanks for calling this out! I think that's why I was leaning toward having this API take a single folder, instead of a list—but it's a corner case for sure.
There was a problem hiding this comment.
Yeah - my first version took just one arg, but then looking at the code in a-c it was clear that it would always need to sum the values from Menu, Toolbar and Unfiled, then use a separate count for Mobile - so I figured I'd optimize for the actual use-case. FWIW, https://github.com/mhammond/firefox-android/tree/count-bookmarks-in-trees is that branch.
| WITH RECURSIVE bookmark_tree(id, parent, type) | ||
| AS ( | ||
| SELECT id, parent, type FROM moz_bookmarks | ||
| WHERE parent IN (SELECT id from moz_bookmarks WHERE guid IN ({vars})) |
There was a problem hiding this comment.
Could we use sql_support::repeat_sql_vars here? I thought for sure we had a helper like repeat_vars already... 🤔
| BookmarkType::Bookmark as u8 | ||
| ); | ||
| let params = rusqlite::params_from_iter(item_guids); | ||
| Ok(db.query_row_and_then(&sql, params, |row| row.get(0))?) |
There was a problem hiding this comment.
If the types work out, could we use our db.query_one extension from sql_support here?
There was a problem hiding this comment.
It's unfortunate that db.query_one doesn't take params :( try_query_one does, so there's odd asymmetry there - I moved to try_query_one anyway even though I don't think we could ever return 0 rows.
mhammond
force-pushed
the
count_in_tree
branch
from
38c6ba2 to
d1d7d36
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #5853 +/- ##
==========================================
- Coverage 36.73% 36.72% -0.02%
==========================================
Files 348 348
Lines 33522 33532 +10
==========================================
Hits 12313 12313
- Misses 21209 21219 +10
☔ View full report in Codecov by Sentry. |
…set of folders
Fixes #5789 - that issue implies the API should just count in a single folder, but the link android-components code ends up wanting to do multiple folders, so having this API take a list of folders makes life easier for them - ie, a-c does something like:
(I'm shaving many many yaks trying to get a-c to build with this, which are unrelated to the patch, so thought I'd get this up while I fight that)
@linabutler, wdyt?