[Kde-pim] Re: Review Request: kmail: now it is allowed to create a new folder on the top of an imap folder

[Volker Krause]

> On July 24, 2011, 8:35 a.m., Volker Krause wrote:
> > resources/imap/addcollectiontask.cpp, line 53
> > <http://git.reviewboard.kde.org/r/102031/diff/1/?file=27780#file27780line53>
> >
> >     KJob::exec() is a no-go in resources due to the reentrancy problems it introduces. But why do you need a folder separator for top-level folders at all?
> 
> Antonis Tsiapaliokas wrote:
>     ok, then we will use signals and slots. Because if the separator is different that '/', then if you try to check the mails, the local folders are deleted and the kmail is crashing.

I still don't understand the need for the separator for creating top-level folders, they don't have a separator in their path, no? And for any non-toplevel folders you get it from the remote id of the parent.


> On July 24, 2011, 8:35 a.m., Volker Krause wrote:
> > resources/imap/retrievecollectionstask.cpp, line 49
> > <http://git.reviewboard.kde.org/r/102031/diff/1/?file=27782#file27782line49>
> >
> >     This is wrong nevertheless, the root folder cannot contain mails, so it should not claim it does. Besides, isStructural() should only return true for empty content mime type lists, which is not the case here, it contains Collection::mimeType().
> 
> Antonis Tsiapaliokas wrote:
>     Yes, but without that the "Add folder" actions is disable in kmail... So how does it sound to you, if we add a new function in kmmainwidget.cpp, which will change that only when we are selecting an imap folder and when we are not then the contentMimeType will become again empty...

The code that decides whether this action should be enabled is in kdepimlibs/akonadi/standardactionmanager.cpp, canCreateCollection() I think. It enables the action iff the following two requirements are met: The current folder can contain folders (content mime type contains Collection::mimeType()) and you are allowed to do that (Collection::rights() & CanCreateCollection). That's IMHO the correct behavior and should not require any of the changes you proposed here to enable the action. Now, that of course doesn't solve the problem, but I hope it explains why I don't like this change :) Instead we should track down why this doesn't behave the way it should in the first place, might e.g. be KMail interfering with Akonadi::StandardActionManager in some way.


> On July 24, 2011, 8:35 a.m., Volker Krause wrote:
> > resources/imap/retrievecollectionstask.cpp, line 50
> > <http://git.reviewboard.kde.org/r/102031/diff/1/?file=27782#file27782line50>
> >
> >     I think this should be the only really needed change for this. However, I'm unsure what side-effects this has on IMAP servers that have only INBOX at top-level and everything else below that (e.g. Cyrus).
> 
> Antonis Tsiapaliokas wrote:
>     Why INBOX will be the top-level folder? I thought that kmail was creating a virtual top-level folder for each imap account....

Sorry, I wasn't clear here. The Akonadi top-level collection will be the same virtual one for all servers. But on Cyrus you only have a limited set of folders underneath that (most notably INBOX, but also stuff like "shared.user" etc). All your own sub-folders are below INBOX.


> On July 24, 2011, 8:35 a.m., Volker Krause wrote:
> > resources/imap/retrievecollectionstask.cpp, line 175
> > <http://git.reviewboard.kde.org/r/102031/diff/1/?file=27782#file27782line175>
> >
> >     Why this change? \NoSelect is set on structural folders that don't physically exist, you can't really create sub-folders of those in general.
> 
> Antonis Tsiapaliokas wrote:
>     So you mean that this changes doesn't belong to the imap folder, but it belongs to a sub-folder of the imap?

Yes, this does not affect the top-level one but sub-folders marked as \NoSelect.


- Volker


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


On July 21, 2011, 4:46 p.m., Antonis Tsiapaliokas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/102031/
> -----------------------------------------------------------
> 
> (Updated July 21, 2011, 4:46 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Summary
> -------
> 
> Hello
> 
> On kmail 2, if you try to add a folder under your top imap folder, you will see that the button "Add folder" is disable. This patch is fixing this issue.
> 
> 
> This addresses bug 251704.
>     http://bugs.kde.org/show_bug.cgi?id=251704
> 
> 
> Diffs
> -----
> 
>   resources/imap/addcollectiontask.cpp 5ce6aec 
>   resources/imap/retrievecollectionmetadatatask.cpp 3c9ead4 
>   resources/imap/retrievecollectionstask.cpp 48f0e57 
> 
> Diff: http://git.reviewboard.kde.org/r/102031/diff
> 
> 
> Testing
> -------
> 
> Kdepim and kdepim-runtime compiles and runs without any issue.
> 
> 
> Thanks,
> 
> Antonis
> 
>

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