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

List:       kmail-devel
Subject:    Re: [PATCH] subscription
From:       Marc Mutz <mutz () kde ! org>
Date:       2002-12-09 22:25:38
[Download RAW message or body]

On Monday 09 December 2002 18:39, Carsten Burghardt wrote:
> So what? Will anybody review the patch, should I commit, should I
> wait?

OK, here we go:

- Why is hasInbox() not in KMAccount?
- hasInbox() is not const-correct
- Make simple getters inline, but all setters virtual, please:

bool hasInbox() const { return mHasInbox; }
virtual void setHasInbox( bool );
// hmm, the account should be able to find this out itself....
bool mHasInbox : 1;

- You don't need <kio/job.h> in kmacctimap.h

namespace KIO {
	class Job;
};

- The new KMAcctImap signals should have take const references as args,
  not values.
- Why aren't the new slots and signals in imapaccountbase? Don't they
  relate to cachedimap, too? Why not?
- Please leave the using clauses directly below their .h files:

#include "vacation.h"
using KMail::Vacation;
#include "subscription.h"
using KMail::Subscription;

- You don't seem to need the KMail::Subscription forward decl in
  kmmainwin.h
- KMail::Subscription is misnamed. If it's a dialog, name it so:
  KMail::SubscriptionDialog
- You don't seem to need the <kdialogbase.h> include in kmmainwin.cpp

I haven't applied your patch here, but what really interests me is the 
complete absence of any kmacctcachedimap.{h,cpp} in your patch.

Marc

-- 
If you read this Mail while moving in traffic, you could hit
lantern stakes or end up knocked over by trucks.
                       -- freely adapted from iX 11/2002 editorial

[Attachment #3 (application/pgp-signature)]
_______________________________________________
KMail Developers mailing list
kmail@mail.kde.org
http://mail.kde.org/mailman/listinfo/kmail

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