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

[Kevin Ottens]

> On March 4, 2013, 7:54 a.m., Kevin Ottens wrote:
> > resources/imap/addcollectiontask.cpp, line 55
> > <http://git.reviewboard.kde.org/r/109276/diff/1/?file=116820#file116820line55>
> >
> >     Any chance you could keep that const and use the ternary operator?
> 
> Andras Mantia wrote:
>     Technically yes. But this code should be changed in the long term, as we would need a better way to detect the separator. Any suggestion for it?
>     One idea would be keeping it as a result of the folder syncing (that's a place where we get the separators). But separators can be different per folder tree (or namespace) if I understood correctly, so do we store one per folder? I tried it, but complicates the code a lot and we need to pass variables between tasks (through the resource).
>     Anyway factoring out this method to ResourceTask makes sense and will make easier later to change it.

I don't have any good suggestion unfortunately. The roots are the problem there, we don't know the separator for them at that point. One level deeper it's easy as we get it in the listing (hence the current code). Might be that I missed something in the RFC, I doubt we're the only ones dealing with that problem.


- Kevin


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


On March 3, 2013, 10:52 p.m., Andras Mantia wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109276/
> -----------------------------------------------------------
> 
> (Updated March 3, 2013, 10:52 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 at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/