[resources/maildir] Don't save "file:" schema to the config
ClosedPublic

Authored by poboiko on Feb 28 2020, 1:54 PM.

Details

Summary

ConfigWidget uses KConfig underneath, and utilizes KUrlRequester custom
widget. The USER property of this widget (which is used by KConfig) is of
type QUrl, and thus when dialog is accepted, the path config property
gets overriden with QUrl::toString() value, which prepends file: schema
(this is basically because KCoreConfigSkeleton::ItemPath is inherited from
ItemString, and when someone calls ItemString::setProperty, it gets
casted as QVariant::toString).

Inside the ConfigWidget::save the code calls setPath method on
url.toLocalFile, which drops the scheme. Because of that, the pathItem
and path property of mSettings have different values, first has schema
and the second hasn't. Eventually, the value stored by pathItem wins, and
mSettings->path() returns URL with schema. However, Maildir doesn't expect
it and misinterprets it as the relative path to current WORKDIR (which is home
directory), thus creating /home/user/file:/home/user/... file structure.

The proposed solution is to simply call mSettings->save(), which overrides
pathItem value and drops schema from it.

It also fixes the AkoNotes resource, which uses the same ConfigWidget.
Funny enough, Contacts resource, which is somewhat similar, is not affected
as it has the same Settings->save() call.

Alternative approaches include:

  1. Teach Maildir to drop the schema (if it's there).
  2. Teach KCoreConfigSkeleton::ItemPath to work with QUrl and don't append

schema (it makes sense, because ItemPath corresponds to local file. Although
it's not documented that it shouldn't have schema, it seems from the tests that
it was the original intent). This could save the headache of having such issue
in the future, but it could mess up with other programs in funny ways (as
currently file: sometimes gets prepended, and some code might implicitly rely
on it)

Additional note:
There are ui.kcfg_Path->url().isLocalFile() checks around, which doesn't make
sense to me, as KUrlRequester is used for local files and it seems like it
always returns QUrl pointing to local file (i.e. have the file: schema).

BUG: 408354
BUG: 411269
BUG: 413588

Test Plan
  1. Open akonadiconsole -> Local Folders properties, change the folder, save
  2. cat ~/.config/akonadi_maildir_resource_0rc. file: schema gets prepended

2.1) akonadictl restart. file: folder gets created inside homedir

  1. Apply patch, repeat (1)-(2.1). file: schema is dropped.

Diff Detail

Repository
R44 KDE PIM Runtime
Branch
scheme-bug (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 23061
Build 23079: arc lint + arc unit
poboiko created this revision.Feb 28 2020, 1:54 PM
Restricted Application added a project: KDE PIM. · View Herald TranscriptFeb 28 2020, 1:54 PM
poboiko requested review of this revision.Feb 28 2020, 1:54 PM

Nicely done!

Is there a way to auto-fix this for existing users?

Nicely done!

Is there a way to auto-fix this for existing users?

We can add temporary code that strips the schema, for example, inside ensureSaneConfguration(). Something like:

QUrl url(mSettings->path());
mSettings->setPath(url.isLocalFile() ? url.toLocalFile() : url.path());
mSettings->save();

will do the trick (I've just tried it, it seems to be working). I can add it to the patch.

However, there is also potential issue regarding user data that could have ended up inside $HOME/file:/$HOME/path/to/maildir.
I guess it would probably be nice to merge it somehow with the "correct" maildir inside $HOME/path/to/maildir.
But that would require some sort of migrator, not really sure what is the best way to do it.

dvratil accepted this revision.Mar 1 2020, 12:19 PM

Hmm, good point that this would require migrating/merging the "wrong" maildir into the correct one that might have contain some historical data (from before we broke it), which is probably not doable in an automatic manner.

Let's merge this this then. Thanks!

This revision is now accepted and ready to land.Mar 1 2020, 12:19 PM
poboiko closed this revision.Mar 1 2020, 6:45 PM