[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-pim
Subject: Re: [Kde-pim] Review Request 108755: akonadi_maildir_resource doesn't work as expected, can anybody
From: "Andras Mantia" <amantia () kde ! org>
Date: 2013-02-03 20:28:27
Message-ID: 20130203202827.26434.28133 () vidsolbach ! de
[Download RAW message or body]
-----------------------------------------------------------
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@kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic