[Kde-pim] [PATCH#2] imap resource: Support multiple IDLE folders.

[Andras Mantia]

Hi,

 after those discussion, maybe we should also look at the patch. ;)


> diff --git a/resources/imap/CMakeLists.txt b/resources/imap/CMakeLists.txt

>  kde4_add_ui_files(akonadi_imap_resource_SRCS serverinfo.ui)
> +kde4_add_ui_files(akonadi_imap_resource_SRCS idlefolderdialogview.ui)

You can add the ui file in the same line.


> @@ -0,0 +1,257 @@
> +/* -*- C++ -*-
> + *
> + *  Copyright (c) 2012 Rüdiger Sonderfeld <ruediger at c-plusplus.de>
> + *
> + *  This library is free software; you can redistribute it and/or modify
> it
> + *  under the terms of the GNU Library General Public License as
> published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> your
> + *  option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful, but
> WITHOUT
> + *  ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + *  FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Library General Public
> + *  License for more details.
> + *
> + *  You should have received a copy of the GNU Library General Public
> License
> + *  along with this library; see the file COPYING.LIB.  If not, write to
> the
> + *  Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> Boston, MA
> + *  02110-1301, USA.
> + */
> +
> +#include "idlefolderdialog.h"
> +
> +#include "settings.h"
> +#include "imapaccount.h"
> +#include "sessionuiproxy.h"
> +
> +#include <QtCore/QCoreApplication>
> +#include <QtGui/QStandardItemModel>
> +#include <QtGui/QBoxLayout>
> +#include <QtGui/QCheckBox>
> +#include <QtGui/QLabel>
> +#include <QtGui/QTreeView>
> +
> +#include <kimap/session.h>
> +#include <kimap/loginjob.h>
> +
> +#include <klocale.h>
> +#include <kdebug.h>
> +#include <kmessagebox.h>
> +
> +#include "ui_idlefolderdialogview.h"
> +
> +IdleFolderDialog::IdleFolderDialog( QWidget *parent )
> +  : KDialog( parent ),
> +    m_session( 0x0 ),
> +    m_ui( new Ui::IdleFolderDialogView ),
> +    m_model( new QStandardItemModel( this ) )
> +{
> +  setModal( true );
> +  setButtons( Ok | Cancel | Reset );
> +
> +  setButtonText( Reset, i18nc( "@action:button", "Reload &List" ) );
> +  enableButton( Reset, false );
> +  connect( this, SIGNAL(user1Clicked()),
> +           this, SLOT(onReloadRequested()) );
> +
> +  m_ui->setupUi( mainWidget() );
> +
> +  m_ui->folderView->setModel( m_model );
> +  m_ui->enableAutoInbox->setChecked( Settings::self()->autoWatchInbox()
> ); +
> +  connect( m_model, SIGNAL(itemChanged(QStandardItem*)),
> +           this, SLOT(onItemChanged(QStandardItem*)) );
> +}
> +
> +IdleFolderDialog::~IdleFolderDialog()
> +{
> +}

The code here that logs onto the server and reads the folders is as I see a 
copy-paste from the SubscriptionDialog. I have two problems with it
- the license header must indicate that this is based on someone else's code 
(basically copy the copyright from there)
- copy paste results in code duplication.

I suggest to fix both by fixing the second problem. Create a new class for 
the model (FolderListingModel or whatever) and reuse that in both your and 
the subscription dialog. You will need to solve the differences in 
onMailBoxesReceived and come up with a generic solution, but for me it 
sounds doable. For both cases the main problem is the same: select a set of 
the existing subfolder based on a criteria. Only the criteria is different.



> +
> +namespace
> +{
> +  QStandardItem *findItem( QStandardItem *parentItem, const QString &text
> ) {
> +    Q_ASSERT( parentItem );
> +    const int rows = parentItem->rowCount();
> +    for( int i = 0; i < rows; ++i ) {
> +      QStandardItem *child = parentItem->child( i );
> +      if( !child ) {
> +        return 0x0;
> +      }
> +      else if( child->text() == text ) {
> +        return child;
> +      }
> +    }
> +    return 0x0;
> +  }
> +}

Hard to argue with someone having a cpluplus.de address, but we usually 
don't use anonymous namespaces for such functions. Either put it as a 
private function, or just without namespace in the cpp file. I even prefer 
such non-member functions at the beginning of the file, but it is personal 
taste.
What is less a personal taste though is 0x0 for null pointers, I think the 
whole KDE codebase uses simply 0. I know there is no difference, it is just 
a style issue to be consistent.


> +void IdleFolderDialog::onItemChanged( QStandardItem *item )
> +{
> +  QFont font = item->font();
> +  font.setBold( item->checkState() != item->data( InitialStateRole
> ).toInt() );
> +  item->setFont( font );
> +}
> +
> +void IdleFolderDialog::slotButtonClicked( int button )
> +{
> +  if ( button == KDialog::Ok ) {
> +    applyChanges();
> +    accept();
> +  } else {
> +    KDialog::slotButtonClicked( button );
> +  }
> +}
> +
> +void IdleFolderDialog::applyChanges()
> +{
> +  const QList<QStandardItem*> items = checkedItems( m_model-
>>invisibleRootItem() );
> +
> +  if( items.size() > warnAfterNIdleFolders ) {
> +    const int answer = KMessageBox::warningContinueCancel( 0x0,

Use "this" as the parent of the message box.

> diff --git a/resources/imap/imapresource.kcfg
> b/resources/imap/imapresource.kcfg
> index f3bfd84..1ef67f0 100644
> --- a/resources/imap/imapresource.kcfg
> +++ b/resources/imap/imapresource.kcfg
> @@ -71,8 +71,12 @@
>      </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>
> +    <entry name="AutoWatchInbox" type="Bool">
> +      <label>Automatically watch /INBOX</label>
> +      <default>true</default>
>      </entry>
>    </group>
>    <group name="siever">

For this change we need an upgrade path. Probably a kconf_update script.


> +  QPointer<IdleFolderDialog> idleFolders = new IdleFolderDialog( this );
> +  idleFolders->setCaption( i18n( "" ) );
> +  idleFolders->setWindowIcon( KIcon( "network-server" ) );
> +  idleFolders->connectAccount( account, m_ui->password->text() );
> +
> +  idleFolders->exec();
> +  delete idleFolders;

In this case just create the dialog on the stack.


The rest of the code (SessionPool and actual IDLE handling) for me looks 
good and fine, but Kevin might comment on that, the IDLE part was his code.
Of course, the TODO parts need to be finished.
The quality of the code is also good, thank you for it. :)

I know I said it is ok to send initially a diff directly to the list, but 
now as the changes are bigger and affect more classes, would be probably 
good if you'd use reviewboard next time.

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