BookmarksRunner: Remove caching of profile database path
ClosedPublic

Authored by alex on Apr 6 2020, 10:55 AM.

Details

Summary

As requested in D28196 this patch removes the caching feature.
This caching causes issues when the user has multiple profiles, because the cached value
is not updated as long as the database exists.

Depends on D28473.

Test Plan

Should compile, no changes to lookup logic made.

Diff Detail

Repository
R120 Plasma Workspace
Branch
bookmarksrunner_firefox_fix_remove_caching (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 24831
Build 24849: arc lint + arc unit
alex created this revision.Apr 6 2020, 10:55 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 6 2020, 10:55 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
alex requested review of this revision.Apr 6 2020, 10:55 AM
bruns added a comment.Apr 6 2020, 11:59 AM

There are two possible ways of changing the caching, and each one will break behavior for one of two different groups of users:

  1. People who rely on the krunner following the default profile.
  2. People who rely on krunner using the dbfile config entry.

I would argue for just removing the grp.writeEntry("dbfile", m_dbFile) line. This keeps the current behavior for the second group, and still solves the problem for the first group when there is no config yet. Only users of the second group with an existing config have to do a one-time change to their config, e.g. resetting/removing the "dbfile" config value.

alex added a comment.EditedApr 6 2020, 12:10 PM

I would argue for just removing the grp.writeEntry("dbfile", m_dbFile) line. This keeps the current behavior for the second group, and still solves the problem for the first group when there is no config yet. Only users of the second group with an existing config have to do a one-time change to their config, e.g. resetting/removing the "dbfile" config value.

But what about renaming the dbfile entry, maybe to alternate_dbfile ? Then the second group of users has to adapt to the changes and
the first group can be sure that the default profile gets looked up correctly (in case the config already exists).

Edit: I initially thought that removing this is the best Idea, because this feature causes issues and is not documented (other that in the source code). And I don't see an actual usecase to load the bookmarks from another profile and launch them in the default one.

alex added a comment.Apr 19 2020, 5:53 PM

Friendly ping :-)

bruns requested changes to this revision.Apr 19 2020, 6:02 PM

Please just remove the writeEntry.

You can then propose "But what about renaming the dbfile entry, maybe to alternate_dbfile?" as another review.

This revision now requires changes to proceed.Apr 19 2020, 6:02 PM
alex updated this revision to Diff 80578.EditedApr 19 2020, 6:10 PM

Implement requested changes

Thanks, I will do that after the bugfix is done :-)

alex updated this revision to Diff 80580.Apr 19 2020, 6:20 PM
bruns accepted this revision.Apr 19 2020, 7:09 PM
This revision is now accepted and ready to land.Apr 19 2020, 7:09 PM
This revision was automatically updated to reflect the committed changes.