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

List:       kmail-devel
Subject:    Re: Implemented a new feature for KMail, please review
From:       Cornelius Schumacher <schumacher () kde ! org>
Date:       2003-07-17 6:25:54
[Download RAW message or body]

On Thursday 17 July 2003 04:30, arnaud.burlet@epfl.ch wrote:
>
> I implemented in KMail a way to take multiple email addresses from
> multiple messages and add them to the address book with only few
> mouse clicks...

That's a nice feature.

> For an illustrated description please see
> http://www.realness.ch/~sts/kmail/

The titles of the e-mail and name columns are swapped.

> I attach an archive with a diff against the kdepim repository, and my
> 2 source files which should go in kmail sources directory.

Some comments on the code:

In KMTakeAddressCommand::execute() it might be better to put all the 
header address field names into a QStringList and iterate over them. So 
you don't need five identical for loops.

In takeAddressForm::addAddress() you create Addressee objects on the 
heap by using "new Addressee". It's better to create them on the stack 
by simply using "Addressee a", so you don't have to care about deletion 
of the objects. The Addressee class is implicitly shared. It is no 
overhead to pass it around by value.

takeAddressForm is an unusual name for a class. Usually class names 
start with a capital letter.

It shouldn't be necessary to call AddressBook::cleanUp().

I don't know what others think, but in my opinion it's clearer to 
inherit from a Designer-generated class and add a .h and a .cpp file 
for the derived class. Using the ui.h file which e.g. intermangles 
member variable declarations with the XML code of the ui file tends to 
be confusing.

Member variables should be prefixed with an "m".

-- 
Cornelius Schumacher <schumacher@kde.org>
_______________________________________________
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]