[Bookmarks Runner] Remove duplicate results for bookmarks
ClosedPublic

Authored by bruns on Sep 8 2018, 7:55 PM.

Details

Summary

An entry from the moz_places db may have several referring entries
in the moz_bookmarks db, i.e. where moz_places.id = moz_bookmarks.fk.
One of these entries is the "main" entry, while the other ones are used
for tags. Only the main entry has a title, while the others have not.
The tag entries have the same type as the main entry, even the parents
have the same type (folder).

Another source for duplicate URLs are manually created bookmarks, e.g.
in different folders. These may have the same or different titles.

To remove these duplicates, merge all entries with the same URL. If a
URL has multiple entries, keep all with distinct titles, otherwise keep
at least one - a bookmark may have an empty title.

See also T9626

Test Plan
  • Create a bookmark
  • Add one or more tags
  • Open FFs bookmarks sidebar
  • Copy and paste the new entry
  • Copy and paste the new entry again, change its title

Search for the new bookmark. It should appear exactly twice, once
with the original title, once with the modified one.
Without patch, it appears 3 times, plus once more for each tag.

Caveat: The bookmarks db has to be checkpointed to make the new
entries visible in the main DB file. To force checkpointing, execute:
$> sqlite3 -column -header ~/.mozilla/firefox/*.default/places.sqlite "PRAGMA wal_checkpoint"

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bruns created this revision.Sep 8 2018, 7:55 PM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 8 2018, 7:55 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bruns requested review of this revision.Sep 8 2018, 7:55 PM
bruns updated this revision to Diff 41227.Sep 8 2018, 9:04 PM

rebase

bruns added a subscriber: ngraham.Sep 9 2018, 12:50 PM
zzag added a subscriber: zzag.Sep 9 2018, 1:01 PM
zzag added inline comments.
runners/bookmarks/browsers/firefox.cpp
136

Why postfix increment?

bruns added inline comments.Sep 9 2018, 1:12 PM
runners/bookmarks/browsers/firefox.cpp
136

Because modern compilers are clever enough do avoid any temporaries

zzag added inline comments.Sep 9 2018, 1:32 PM
runners/bookmarks/browsers/firefox.cpp
136

Does the C++ standard guarantee that?

bruns added inline comments.Sep 9 2018, 1:34 PM
runners/bookmarks/browsers/firefox.cpp
136

It neither guarantees prefix is more efficient than postfix.

zzag added inline comments.Sep 9 2018, 1:35 PM
runners/bookmarks/browsers/firefox.cpp
136

Please use prefix increment.

bruns added inline comments.Sep 9 2018, 1:37 PM
runners/bookmarks/browsers/firefox.cpp
136

If there are more required changes, yes, else - no.

davidedmundson added a subscriber: davidedmundson.EditedSep 9 2018, 2:53 PM

RunnerContext::addMatches has its own duplicate check based on match.id

We're not explicitly setting that currently.
I think we can just do match.setId(url+title); and have everything handled auto-magically.

If that does work it will be less code with the same results, faster and even handle dupes in both firefox and chrome bookmarks. Win win.

bruns added a comment.EditedSep 9 2018, 3:46 PM

RunnerContext::addMatches has its own duplicate check based on match.id

We're not explicitly setting that currently.
I think we can just do match.setId(url+title); and have everything handled auto-magically.

If that does work it will be less code with the same results, faster and even handle dupes in both firefox and chrome bookmarks. Win win.

hm, no:

  • It will not filter out the entries with empty titles
  • The BookmarkMatch constructor fetches the favicon for each match
  • Currently, the runner only fetches results from the default browser, no deduplicating between browsers is irrelevant

Is there anything missing?

bruns updated this revision to Diff 42615.Sun, Sep 30, 5:19 PM

add inline comments to code

bruns added a comment.Tue, Oct 2, 12:07 AM

qdbus-qt5 org.kde.krunner /App org.kde.krunner.App.querySingleRunner bookmarks kde

Current state:

Deduplicated:

bruns added a comment.Tue, Oct 2, 12:15 AM

I would really appreciate if we could get this into the imminent Plasma release.

bruns marked 6 inline comments as done.Tue, Oct 2, 12:15 AM
broulik accepted this revision.Tue, Oct 2, 7:26 AM
broulik added a subscriber: broulik.

Somehow the usage of equal_range and keyValueBegin looks quite convoluted but if it works... ship it

This revision is now accepted and ready to land.Tue, Oct 2, 7:26 AM
This revision was automatically updated to reflect the committed changes.