'Re: [Kde-pim] Review Request 109276: Fixes in IMAP resource folder handling'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-pim
Subject:    Re: [Kde-pim] Review Request 109276: Fixes in IMAP resource folder handling
From:       "Andras Mantia" <amantia () kde ! org>
Date:       2013-05-24 9:34:28
Message-ID: 20130524093428.6954.76029 () vidsolbach ! de
[Download RAW message or body]

> On May 6, 2013, 6:20 p.m., Volker Krause wrote:
> > resources/imap/addcollectiontask.cpp, line 87
> > <http://git.reviewboard.kde.org/r/109276/diff/1/?file=116820#file116820line87>
> > 
> > This looks rather dangerous, what if there's already content in this folder?

Good question. What is the probability that it can happen? This is called from the resource's \
collectionAdded. The problem is point #3. Any other idea?

- Andras

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109276/#review32148
-----------------------------------------------------------

On March 28, 2013, 9:45 p.m., Andras Mantia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109276/
> -----------------------------------------------------------
> 
> (Updated March 28, 2013, 9:45 p.m.)
> 
> 
> Review request for KDEPIM and Kevin Ottens.
> 
> 
> Description
> -------
> 
> We discovered several issues with the IMAP resource folder handling. This patch addresses \
> them, although in some cases with a temporarly solution: 
> 1) Creation of toplevel folders created folders with weird remote ids. E.g when creating \
> folder "foo", it set the remoteid to "ifoo". This was because AddCollectionTask sets the \
> first char of the remoteId for new collections based on the first character of the remoteId \
> of the parent collection. For toplevel collections this is "i" from "imap://"... This breaks \
> everything created below this folder as well. Now it is hardcoded to "/", although this will \
> break for IMAP servers using other separators. A nicer solution is needed (finding the right \
> separator is not trivial though). This bug seems to be present also in other tasks as well, \
> but I didn't change as I didn't tested those codepaths. 
> 2) Creating toplevel folders requires a resync of the resource for them to appear. This is \
> actually some bug in the ETM that I don't feel like debugging, so a solution used also in \
> other parts of the code is reused: request a resync of the collection list. 
> 3) If a folder is created, but it fails (e.g lack of rights on the server), the collection \
> for the folder remains in the akonadi database without a remoteId. This blocks creating of \
> new folders with the same name. I consider that failing to create a folder on the resource's \
> server side is something the user must know and the akonadi cache should reflect the status \
>                 on the server, I did the following:
> - remove the collection from the cache
> - propagate the error through the status message. This is less then optimal, but afaik there \
> is no other mechanism so far to catch the errors in applications (KMail). Maybe the tracer \
> can be used though? 
> 4) Given two folders starting with the same characters (e.g foo and foobar) and deleting \
> "foo" deleted both "foo" and "foobar" because of the wrong startsWith check. Now the folder \
>                 is deleted, if:
> - it matches the name exactly
> - the found folder is a subolder of the folder that will be deleted (startwith name + \
> separator) 
> 5) Deleting of folder was...broken completely. If we had INBOX, foo, bar and tried to deleted \
> e.g bar,  the code started to list the directories, but aborted on the first time when \
> onMailBoxesReceived was called, but the list of mailbox didn't include what we wanted to \
> delete. E.g if the list emitted INBOX, then we aborted without waiting that the slot is \
> called with the rest of the folders. Now we keep track if we found the right folder and abort \
> only if none of the listed folders is what we were expecting. 
> 6) Dummyresourcestate unconditionally removed the first char from the mailbox name, so for a \
> collection "foo" it returned "oo" messing the tests up. Now it is slightly more intelligent. 
> 7) adapt the test (now the order of deletion calls is different) and fix its synchronization \
> by not using QTest::qWait(), but an event loop to detect when the task finished. This catches \
> (with a deadlock) cases when the task is finished without calling changeProcessed() or \
> cancelTask(). 
> 
> Diffs
> -----
> 
> resources/imap/tests/testremovecollectiontask.cpp ba1780b 
> resources/imap/tests/dummyresourcestate.cpp 518bf8a 
> resources/imap/removecollectionrecursivetask.cpp 50de4d6 
> resources/imap/removecollectionrecursivetask.h 361cb0d 
> resources/imap/imapresource.cpp 8d2332e 
> resources/imap/imapresource.h 130a337 
> resources/imap/changecollectiontask.cpp c5ae674 
> resources/imap/addcollectiontask.cpp d44b98b 
> 
> Diff: http://git.reviewboard.kde.org/r/109276/diff/
> 
> 
> Testing
> -------
> 
> Testing on GMail and the KDAB imap server, running the above unit test (most of the unit \
> tests fail, but I was looking only at this one now). 
> 
> Thanks,
> 
> Andras Mantia
> 
> 

_______________________________________________
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] 