From kmail-devel Thu Jul 17 06:25:54 2003 From: Cornelius Schumacher Date: Thu, 17 Jul 2003 06:25:54 +0000 To: kmail-devel Subject: Re: Implemented a new feature for KMail, please review X-MARC-Message: https://marc.info/?l=kmail-devel&m=105842310927431 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 _______________________________________________ KMail Developers mailing list kmail@mail.kde.org http://mail.kde.org/mailman/listinfo/kmail