[Kde-pim] [PATCH] imap resource: Support multiple IDLE folders.
Andras Mantia
amantia at kde.org
Tue Oct 16 18:39:46 BST 2012
Hi,
first of all thanks for the patch (and sorry for top posting this time).
I'm not the IMAP resource maintainer, but touched some imap code in the
past, so ....
In general I think the code and the approach is ok. There are some coding
style guide issue (especially const Foo &foo vs. Foo const &foo - we use the
former), noticed some changes in the diff that might indicate whitespace
only changes (maybe space at line ends? anyway, this will be visible in
reviewboard).
I'd put the utility code (url2collection and collection2url) in a separate
file.
What I wonder is the remoteId setting
+ c.setRemoteId( url.scheme() + "://" + url.authority() + '/' );
What is the reason behind it? As far as I see currently the root collection
has the imap://user@server/ remote id, while the sub-collections have
/FOLDERNAME, while the method is not called for the root item.
In any case, you should use rootRemoteId() there (from ResourceState or
Settings).
Of course we need configuration option as well. As I said on IRC, I'd use a
gui similar to how subscriptions are set up. By default idle on INBOX only,
and the user can set the rest.
A way to remove a single collection from idle watch is also needed. I have
no problem making SessionPool more configurable on the fly.
Something I wonder: are there limitations on how many connection could be
opened to a server? If yes, we might need to add some error handling, so if
you set up 10 idling collections, but the server accepts max. 5 connections
for the same user, we should warn about it (and tell the consequences and
which folder will not be watched).
Andras
Rüdiger Sonderfeld wrote:
> Currently the imap resource only uses IDLE for the /INBOX folder.
> However having several inboxes or important folders is a common
> use case and the imap resource should be capable of supporting it.
>
> This patch is an initial attempt at adding support for multiple IDLE
> folders. The current implementation changes the single IdleRidPath to
> IdleBoxes containing URLs to IMAP folders. The helper functions
> url2collection and collection2url are used to convert from
> Akonadi::Collections to URLs and vice versa. The ImapResources class
> then creates an ImapIdleManagers for each URL.
>
> Currently the only way to watch multiple folders (/INBOX is still
> automatically added) is to edit the imap resource's config file and
> restarting akonadi. This of course has to be changed and it should be
> configurable from KMail. However the current SessionPool
> implementation does not allow to change the maxPoolSize at runtime.
>
> This is my first contribution to KDEPIM. So I'm looking forward to any
> comments and criticism.
>
> Signed-off-by: Rüdiger Sonderfeld <ruediger at c-plusplus.de>
> ---
> resources/imap/imapresource.cpp | 81
> +++++++++++++++++++++++++-------------
> resources/imap/imapresource.h | 4 +-
> resources/imap/imapresource.kcfg | 4 +-
> resources/imap/resourcestate.cpp | 34 ++++++++++++----
> 4 files changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/resources/imap/imapresource.cpp
> b/resources/imap/imapresource.cpp index 096f890..5213d10 100644
> --- a/resources/imap/imapresource.cpp
> +++ b/resources/imap/imapresource.cpp
> @@ -91,8 +91,7 @@ using namespace Akonadi;
>
> ImapResource::ImapResource( const QString &id )
> : ResourceBase( id ),
> - m_pool( new SessionPool( 2, this ) ),
> - m_idle( 0 ),
> + m_pool( new SessionPool( Settings::self()->idleBoxes().size()+1, this
> ) ),
> m_fastSync( false )
> {
> if ( name() == identifier() ) {
> @@ -480,8 +479,7 @@ void ImapResource::doSetOnline(bool online)
> Q_FOREACH(ResourceTask* task, m_taskList)
> task->kill();
> m_taskList.clear();
> - delete m_idle;
> - m_idle = 0;
> + clearIdle();
> } else if ( online && !m_pool->isConnected() ) {
> scheduleConnectionAttempt();
> }
> @@ -519,44 +517,73 @@ void ImapResource::reconnect()
>
> //
>
----------------------------------------------------------------------------------
>
> +void ImapResource::clearIdle() {
> + Q_FOREACH( ImapIdleManager *im, m_idle ) {
> + delete im;
> + }
> + m_idle.clear();
> +}
> +
> void ImapResource::startIdleIfNeeded()
> {
> - if ( !m_idle ) {
> + if ( m_idle.isEmpty() ) {
> startIdle();
> }
> }
>
> +namespace
> +{
> + Collection url2collection(QUrl const &url)
> + {
> + if( url.scheme() != "imap" && url.scheme() != "imaps" )
> + return Collection();
> +
> + // Add authority part (e.g., imap://foo@example.org)
> + Collection c, p;
> + p.setParentCollection( Collection::root() );
> + c.setParentCollection( p );
> + c.setRemoteId( url.scheme() + "://" + url.authority() + '/' );
> + p = c;
> +
> + // Add a collection item for each subdir
> + QStringList path = url.path().split('/');
> + if( !path.isEmpty() && path.first().isEmpty() )
> + path.removeFirst();
> +
> + Q_FOREACH( QString const &s, path ) {
> + c.setParentCollection( p );
> + c.setRemoteId( '/' + s );
> + p = c;
> + }
> +
> + return c;
> + }
> +}
> +
> void ImapResource::startIdle()
> {
> - delete m_idle;
> - m_idle = 0;
> + clearIdle();
>
> if ( !m_pool->serverCapabilities().contains( "IDLE" ) )
> return;
>
> - const QStringList ridPath = Settings::self()->idleRidPath();
> - if ( ridPath.size() < 2 )
> + QStringList const boxes = Settings::self()->idleBoxes();
> + if( boxes.isEmpty() )
> return;
> + Q_FOREACH( QString const &box, boxes ) {
> + Collection c = url2collection(QUrl(box));
>
> - Collection c, p;
> - p.setParentCollection( Collection::root() );
> - for ( int i = ridPath.size() - 1; i > 0; --i ) {
> - p.setRemoteId( ridPath.at( i ) );
> - c.setParentCollection( p );
> - p = c;
> - }
> - c.setRemoteId( ridPath.first() );
> -
> - Akonadi::CollectionFetchScope scope;
> - scope.setResource( identifier() );
> - scope.setAncestorRetrieval( Akonadi::CollectionFetchScope::All );
> + Akonadi::CollectionFetchScope scope;
> + scope.setResource( identifier() );
> + scope.setAncestorRetrieval( Akonadi::CollectionFetchScope::All );
>
> - Akonadi::CollectionFetchJob *fetch
> - = new Akonadi::CollectionFetchJob( c,
> Akonadi::CollectionFetchJob::Base, this );
> - fetch->setFetchScope( scope );
> + Akonadi::CollectionFetchJob *fetch
> + = new Akonadi::CollectionFetchJob( c,
> Akonadi::CollectionFetchJob::Base, this );
> + fetch->setFetchScope( scope );
>
> - connect( fetch, SIGNAL(result(KJob*)),
> - this, SLOT(onIdleCollectionFetchDone(KJob*)) );
> + connect( fetch, SIGNAL(result(KJob*)),
> + this, SLOT(onIdleCollectionFetchDone(KJob*)) );
> + }
> }
>
> void ImapResource::onIdleCollectionFetchDone( KJob *job )
> @@ -570,7 +597,7 @@ void ImapResource::onIdleCollectionFetchDone( KJob
> *job )
> return;
>
> ResourceStateInterface::Ptr state = ::ResourceState::createIdleState(
> this, c );
> - m_idle = new ImapIdleManager( state, m_pool, this );
> + m_idle.push_back( new ImapIdleManager( state, m_pool, this ) );
>
> } else {
> kWarning() << "CollectionFetch for idling failed."
> diff --git a/resources/imap/imapresource.h b/resources/imap/imapresource.h
> index da1ccfe..fdbae80 100644
> --- a/resources/imap/imapresource.h
> +++ b/resources/imap/imapresource.h
> @@ -122,11 +122,13 @@ private:
> void queueTask( ResourceTask *task );
> bool needsNetwork() const;
>
> + void clearIdle();
> +
> friend class ImapIdleManager;
>
> SessionPool *m_pool;
> QList<ResourceTask*> m_taskList;
> - ImapIdleManager *m_idle;
> + QList<ImapIdleManager*> m_idle;
> bool m_fastSync;
> };
>
> diff --git a/resources/imap/imapresource.kcfg
> b/resources/imap/imapresource.kcfg
> index f3bfd84..9d74b7d 100644
> --- a/resources/imap/imapresource.kcfg
> +++ b/resources/imap/imapresource.kcfg
> @@ -71,8 +71,8 @@
> </entry>
> </group>
> <group name="idle">
> - <entry name="IdleRidPath" type="StringList">
> - <label>RID path to the mailbox to watch for changes</label>
> + <entry name="IdleBoxes" type="StringList">
> + <label>URL to mailboxes to watch for changes</label>
> </entry>
> </group>
> <group name="siever">
> diff --git a/resources/imap/resourcestate.cpp
> b/resources/imap/resourcestate.cpp
> index 7aa42d8..59b3e82 100644
> --- a/resources/imap/resourcestate.cpp
> +++ b/resources/imap/resourcestate.cpp
> @@ -297,17 +297,37 @@ QString ResourceState::mailBoxForCollection( const
> Akonadi::Collection &collecti
> return mailbox;
> }
>
> +namespace
> +{
> + QString collection2url( Akonadi::Collection const &collection )
> + {
> + QStringList ridPath;
> +
> + Akonadi::Collection curCol = collection;
> + while ( curCol != Akonadi::Collection::root() &&
> !curCol.remoteId().isEmpty() ) {
> + QString remoteId = curCol.remoteId();
> + // Remove one / if the next id starts with a / and the current ends
> with /
> + if( !ridPath.isEmpty() && ridPath.front()[0] == '/' &&
> remoteId.endsWith('/') )
> + remoteId.chop(1);
> + ridPath.push_front( remoteId );
> + curCol = curCol.parentCollection();
> + }
> +
> + return ridPath.join("");
> + }
> +}
> +
> +// TODO change name (addToIdleCollection?)
> void ResourceState::setIdleCollection( const Akonadi::Collection
> &collection
> )
> {
> - QStringList ridPath;
> + QString const url( collection2url( collection ) );
> + if( Settings::self()->idleBoxes().contains( url ) )
> + return;
>
> - Akonadi::Collection curCol = collection;
> - while ( curCol != Akonadi::Collection::root() &&
> !curCol.remoteId().isEmpty() ) {
> - ridPath.append( curCol.remoteId() );
> - curCol = curCol.parentCollection();
> - }
> + QStringList boxes = Settings::self()->idleBoxes();
> + boxes << url;
>
> - Settings::self()->setIdleRidPath( ridPath );
> + Settings::self()->setIdleBoxes( boxes );
> Settings::self()->writeConfig();
> }
>
_______________________________________________
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