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

List:       kmail-devel
Subject:    Re: [PATCH] subscription
From:       Carsten Burghardt <burghardt () kde ! org>
Date:       2002-12-15 17:16:04
[Download RAW message or body]

On Monday 09 December 2002 23:25, Marc Mutz wrote:
> 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;

OK.

> - You don't need <kio/job.h> in kmacctimap.h
>
> namespace KIO {
> 	class Job;
> };

Correct, but then I need kio/global.h for the UDSEntryList (typedef)

> - The new KMAcctImap signals should have take const references as args,
>   not values.

Hmm, that is a problem. When I declare the QStringList's as const references 
the listing doesn't work anymore. Probably because of the fact that the lists 
need to be cleared after the signal is emitted (in imapaccountbase).

> - Why aren't the new slots and signals in imapaccountbase? Don't they
>   relate to cachedimap, too? Why not?

They do and therefore I moved some stuff to imapaccountbase.
The new listDirectory can now be used from the cachedimap implementation.

> - Please leave the using clauses directly below their .h files:
>
> #include "vacation.h"
> using KMail::Vacation;
> #include "subscription.h"
> using KMail::Subscription;

OK.

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

OK.

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

Not anymore ;-)
I tested it with disconnected imap and it works. Well, the whole offline-code 
doesn't really work here but that is another story.

OK to commit? Anybody should of course test this patch...

-- 
Regards,

Carsten Burghardt

["subscription5.diff.gz" (application/x-gzip)]
_______________________________________________
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]