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

List:       kde-devel
Subject:    Re: KMail
From:       Zack Rusin <zack () kde ! org>
Date:       2003-08-07 16:55:06
[Download RAW message or body]

On Thursday 07 August 2003 15:05, Alexey Arzamasov wrote:
> yes, i think so. Here the patch against KMail I just made.
> People, I'm totally new to this list and to KDE development too :)
> So, I just get this issue as example to learn KDE and QT development.
> Writing the patch, I read the manuals on every QT class and methods I
> use :) So, the main thing I worried about writing the patch is too
> learn and check if I on the right way. Please, comment. Comment
> everything that you want - code style, efficitives of QT classes uses
> etc.

Hi, first of all, thank you for your patch. I'm CC'ing the KMail 
development list because most of us don't read the kde-devel list. 
I completely rewrote the addressee selector widget so your patch is not 
necessary in the head. KMail in CVS can already sort entries both on 
the name and email the address. 
1) Most importantly please generate patches using "diff -u3 -p -b -B". 
The best idea is to put it in your .cvsrc file. As we prefer simply 
diffs against one of the cvs branches. The diff you've sent is simply 
hard to read.
2) As far as the patch itself goes, just for the future - don't pass 
objects by value. Even most of them are implicitly shared we don't do 
that. And make sure you think a few times about complexity of every 
algorithm you want to introduce in KMail, for example :
>for (unsigned long i = 0; i < list.count(); i++) {
>               emap[clearEMail( list[ i ] )] = list[i];
>}
is a very bad idea. People have addressbook's with thousands of 
contacts. the operator[](int) on a QStringList is O(n) and you call it 
n times. Simply use iterators. It would be a lot faster.
3) Finally, watch closely the indention style the developers are using 
in the file you're changing. You used a tab indention which we don't 
use in KMail.
We're always happy to see new developers so don't get discouraged and 
keep coding :)

Zack

-- 
"I get to go to lots of overseas places, like Canada." - Britney
Spears 
 
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]