[Kde-pim] Review Request 108755: akonadi_maildir_resource doesn't work as expected, can anybody help?
Andras Mantia
amantia at kde.org
Sun Feb 3 20:28:27 GMT 2013
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/108755/#review26592
-----------------------------------------------------------
Wow, very good catch, thank you! I am amazed and shamed I didn't realize how broken the cache is. Of course, now that I looked again at the code and your output (and I could reproduce it), it started to make sense.
Let me explain: the idea behind mMaildirsForCollection was that each time we tried to find a Maildir object for a Collection, the disk was accessed and the content of the dir listed. This is slow and uses power.
What went wrong? When the maildirForCollection is called, most of the time the Collection.id() is not known, only the remoteId(). This tricked me, as usually we have the id() as well, but not here...
That explains the negative number and explain the positive numbers as well from time to time, as depending from which method maildirForCollection() was called, the passed collection object might have the id set or not.
Attached is a patch that changes the hash key from id to the path of the collection inside the maildir structure. Note that this doesn't use the actual HDD to find the path, this is purely how Akonadi thinks/knows the folder layout.
What I'd like to know is why you started to debug this. I wonder if you had any misbehavior due to the broken cache. As I see you should have no side-affects, aside of the cache not working fully, unless Akonadi started to assign the same negative number for different collection (but then I'd expect to see a real mess in the folder listing).
diff --git resources/maildir/maildirresource.cpp resources/maildir/maildirresource.cpp
index df989c1..01a432f 100644
--- resources/maildir/maildirresource.cpp
+++ resources/maildir/maildirresource.cpp
@@ -54,8 +54,9 @@ using namespace Akonadi_Maildir_Resource;
Maildir MaildirResource::maildirForCollection( const Collection& col )
{
- if ( mMaildirsForCollection.contains( col.id() ) ) {
- return mMaildirsForCollection.value( col.id() );
+ const QString path = maildirPathForCollection( col );
+ if ( mMaildirsForCollection.contains( path ) ) {
+ return mMaildirsForCollection.value( path );
}
if ( col.remoteId().isEmpty() ) {
@@ -66,12 +67,12 @@ Maildir MaildirResource::maildirForCollection( const Collection& col )
if ( col.parentCollection() == Collection::root() ) {
kWarning( col.remoteId() != mSettings->path() ) << "RID mismatch, is " << col.remoteId() << " expected " << mSettings->path();
Maildir maildir( col.remoteId(), mSettings->topLevelIsContainer() );
- mMaildirsForCollection.insert( col.id(), maildir );
+ mMaildirsForCollection.insert( path, maildir );
return maildir;
}
Maildir parentMd = maildirForCollection( col.parentCollection() );
Maildir maildir = parentMd.subFolder( col.remoteId() );
- mMaildirsForCollection.insert( col.id(), maildir );
+ mMaildirsForCollection.insert( path, maildir );
return maildir;
}
@@ -663,7 +664,8 @@ void MaildirResource::collectionRemoved( const Akonadi::Collection &collection )
emit error( i18n( "Failed to delete sub-folder '%1'.", collection.remoteId() ) );
}
- mMaildirsForCollection.remove( collection.id() );
+ const QString path = maildirPathForCollection( collection );
+ mMaildirsForCollection.remove( path );
changeProcessed();
}
@@ -819,5 +821,17 @@ void MaildirResource::fsWatchFileModifyResult(KJob* job)
}
}
+QString MaildirResource::maildirPathForCollection(const Collection& collection) const
+{
+ QString path = collection.remoteId();
+ Akonadi::Collection parent = collection.parentCollection();
+ while ( !parent.remoteId().isEmpty() ) {
+ path.prepend( parent.remoteId() + QLatin1Char('/') );
+ parent = parent.parentCollection();
+ }
+
+ return path;
+}
+
#include "maildirresource.moc"
diff --git resources/maildir/maildirresource.h resources/maildir/maildirresource.h
index b0c595a..f792b23 100644
--- resources/maildir/maildirresource.h
+++ resources/maildir/maildirresource.h
@@ -87,10 +87,12 @@ class MaildirResource : public Akonadi::ResourceBase, public Akonadi::AgentBase:
/** Creates a collection object for the given maildir @p md. */
Akonadi::Collection collectionForMaildir( const KPIM::Maildir &md ) const;
- private:
+ QString maildirPathForCollection( const Akonadi::Collection &collection) const;
+
+private:
Akonadi_Maildir_Resource::MaildirSettings *mSettings;
KDirWatch *mFsWatcher;
- QHash<Akonadi::Collection::Id, KPIM::Maildir> mMaildirsForCollection;
+ QHash<QString, KPIM::Maildir> mMaildirsForCollection;
};
- Andras Mantia
On Feb. 3, 2013, 5:37 p.m., Guy Maurel wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/108755/
> -----------------------------------------------------------
>
> (Updated Feb. 3, 2013, 5:37 p.m.)
>
>
> Review request for KDEPIM, Andras Mantia, Kevin Krammer, and Till Adam.
>
>
> Description
> -------
>
> I implemented a little dump to examine what is happening with the QHash mMaildirsForCollection.
>
> It doesn't work as we could expect.
>
> At the start of /usr/local/bin/akonadi_agent_launcher akonadi_maildir_resource akonadi_maildir_resource_0
> we get this:
>
> List of objects of mMaildirsForCollection
> +------+------------+----------------------------------------------------------------------------------+
> | id | name | remoteId |
> +------+------------+----------------------------------------------------------------------------------+
> | -5 | local-mail | /home/guy-kde/.local/share/local-mail |
> | -6 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> +------+------------+----------------------------------------------------------------------------------+
> 2 objects
>
> After starting kmail:
>
> List of objects of mMaildirsForCollection
> +------+------------+----------------------------------------------------------------------------------+
> | id | name | remoteId |
> +------+------------+----------------------------------------------------------------------------------+
> | -5 | local-mail | /home/guy-kde/.local/share/local-mail |
> | -6 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -7 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | -8 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | -9 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | -10 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a3 |
> +------+------------+----------------------------------------------------------------------------------+
> 6 objects
>
> All the id are negativ.
> This comes because the function:
> Collection::List MaildirResource::listRecursive
> (file kdepim-runtime/resources/maildir/maildirresource.cpp)
>
> doesn't set the id itself. The default value is set by entity.
>
> This explains why the if at:
> Maildir MaildirResource::maildirForCollection( const Collection& col )
> ...
> if ( mMaildirsForCollection.contains( col.id() ) ) {
>
> is never TRUE.
>
> After having some click at some Folder, the correct "id", which is stored in the mysql-database, is found.
>
> List of objects of mMaildirsForCollection
> +------+------------+----------------------------------------------------------------------------------+
> | id | name | remoteId |
> +------+------------+----------------------------------------------------------------------------------+
> | 2 | local-mail | /home/guy-kde/.local/share/local-mail |
> | 3 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | -5 | local-mail | /home/guy-kde/.local/share/local-mail |
> | 21 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | 4 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> | -6 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | 22 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | -7 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | 23 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | -8 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | 24 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a3 |
> | -9 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | 8 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -10 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a3 |
> | -11 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | -12 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> +------+------------+----------------------------------------------------------------------------------+
> 16 objects
>
> BUT, making some more folder-moves, I get:
>
> List of objects of mMaildirsForCollection
> +------+------------+----------------------------------------------------------------------------------+
> | id | name | remoteId |
> +------+------------+----------------------------------------------------------------------------------+
> | -223 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | -224 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | 2 | local-mail | /home/guy-kde/.local/share/local-mail |
> | -225 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> | 3 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | -5 | local-mail | /home/guy-kde/.local/share/local-mail |
> | 4 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> | -6 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -7 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | -8 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | -9 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | 8 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -10 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a3 |
> | -11 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | -12 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> | -277 | local-mail | /home/guy-kde/.local/share/local-mail |
> | -278 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -279 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | -280 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | 21 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | -281 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a2 |
> | 22 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | -282 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a3 |
> | 23 | a2 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a2 |
> | -283 | outbox | /home/guy-kde/.local/share/local-mail/outbox |
> | 24 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/a3 |
> | -284 | sent-mail | /home/guy-kde/.local/share/local-mail/sent-mail |
> | -218 | local-mail | /home/guy-kde/.local/share/local-mail |
> | -219 | inbox | /home/guy-kde/.local/share/local-mail/inbox |
> | -220 | A | /home/guy-kde/.local/share/local-mail/.inbox.directory/A |
> | -221 | a1 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a1 |
> | -222 | a3 | /home/guy-kde/.local/share/local-mail/.inbox.directory/.A.directory/a3 |
> +------+------------+----------------------------------------------------------------------------------+
> 32 objects
>
> I have no solution to solve this.
> Can anybody help me?
> Thanks.
>
>
> Diffs
> -----
>
> resources/maildir/maildirresource.h b0c595a
> resources/maildir/maildirresource.cpp df989c1
>
> Diff: http://git.reviewboard.kde.org/r/108755/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Guy Maurel
>
>
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
More information about the kde-pim
mailing list