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

List:       kmail-devel
Subject:    Address completion (Re: Duplicates)
From:       David Faure <dfaure () klaralvdalens-datakonsult ! se>
Date:       2006-09-13 22:00:29
Message-ID: 200609140000.31403.dfaure () klaralvdalens-datakonsult ! se
[Download RAW message or body]

On Tuesday 12 September 2006 23:55, Allen Winter wrote:
> On Monday 11 September 2006 15:20, Christian wrote:
> > Hi,
> > 
> > can some one please have a look at these bugs. I think they can be marked as 
> > duplicates and fixed with the suggested patch for Bug 107945
> > 
> > Bug 98691: tab completion last name / nickname
> > Bug 76739: Partial string search for contacts in address fields
> > Bug 77342: include nicknames in address lookup
> > Bug 109798: apply distinct on address in dropdown for autocomplete
> > Bug 107945: wrong/strange autocomplete in composer addressfield
> > 
> 
> Couple of things:
> 1. Christian is doing great work
I concur. I am *very* impressed that Christian found a way to implement substring
completion in kmail's composer, I thought it was impossible to do with KCompletion.
I stand corrected, obviously.
I tested the patch and it works fine here, I get a single completion offer with a \
standard form, whether I type the first name, last name, or email.

But... am I right that this patch basically forces popup completion, i.e. \
breaks/disables the other modes of completion? If that's the case, I would advocate \
to either find a way to make them work again, or to disable/remove the option in the \
RMB popup which allows to set the completion mode. I personally don't see why anyone \
really would want automatic completion... but I'm sure that there are people using \
it, maybe because some bug in the popup completion made them switch to it, or for \
whichever other reason :/ In any case the config options have to match the \
capabilities.

Coding comments:

> +    QStringList t  = emailList.grep(item);     //search for current email in list
> +    if( t.empty() )                            //add email if not there
grep to find if an item is in a list is a slow solution, no need to construct a temp \
stringlist etc. The fastest way in qt3 is
  if (emailList.find(item) != emailList.end())

> QStringList sl = QStringList( addr );
can be simply   QStringList sl( addr );

> +    for( QStringList::Iterator sit (mailAddr.begin()), sEnd(mailAddr.end()); sit \
> != sEnd; ++sit){
Should be a ConstIterator.

> +  QMap<QString,QString> mailAddrDistinct;
> +      mailAddrDistinct[(*sit)];  //store mailAddr, QMap will make them unique
I know that qt3 is missing QSet, but maybe this could be a QMap<QString,bool> to \
avoid copying the empty string into every entry and to avoid the somewhat surprising \
notation above, which would be come mailAddrDistinct[(*sit)] = true.

> +  pMatches->operator+=(mailAddrDistinct.keys()); //add emailAddr
Unusual too, to write "operator+=" ;)
The alternative isn't great, I admit, but I think it's a bit easier to read:
(*pMatches) += mailAddrDistinct.keys();

> +  QStringList keyWords;
> for ( it = emails.begin(); it != emails.end(); ++it ) {
> +    keyWords.clear();
Better declare keyWords inside the for loop. Make variables as local as C++,
it helps maintainance and future refactorings.
Same for QStringList mailAddr in postProcessMatches.

In any case - thanks for that work, I am happy to see this completion stuff being
made sane.

-- 
David Faure, faure@kde.org, dfaure@klaralvdalens-datakonsult.se
KDE/KOffice developer, Qt consultancy projects
Klarälvdalens Datakonsult AB, Platform-independent software solutions
_______________________________________________
KMail developers mailing list
KMail-devel@kde.org
https://mail.kde.org/mailman/listinfo/kmail-devel


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