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

List:       kmail-devel
Subject:    Re: Address completion (Re: Duplicates)
From:       Ingo =?iso-8859-15?q?Kl=F6cker?= <kloecker () kde ! org>
Date:       2006-09-24 21:38:28
Message-ID: 200609242338.36960 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Sunday 24 September 2006 01:56, Christian wrote:
> Hi David and Ingo!
>
> thanks for your comments. I have put some more thoughts into the
> problem and fixed some other stuff that came up during testing. From
> my point of view the patch is complete and can be committed. What do
> you think?
>
> Regards
> Christian
>
> kdepim.diff contains the following changes:
> 1) RecipientsEditor, RecipientsView RecipientsLine ... KMComposeWin
> store and keep track of completionMode
> I have made View and Line friends, IMHO makes the code easier to read
> than the alternative: forward signals from and  to
> RecipientsLineEdit->RecipientsView->RecipientsEditor->...

You don't need to add a slot to RecipientsEditor just to forward a 
signal because you can connect signals to signals, i.e.
  
+  connect( line->mEdit, 
SIGNAL( completionModeChanged( KGlobalSettings::Completion ) ),
+    SIGNAL( completionModeChanged( KGlobalSettings::Completion ) ) );


In RecipientsView::setCompletionMode() it might be a good idea to 
enclose the line
  line->mEdit->setCompletionMode( mode );
in
  line->mEdit->blockSignals( true );
  line->mEdit->setCompletionMode( mode );
  line->mEdit->blockSignals( false );
in order to prevent too much recursion (because 
line->mEdit->setCompletionMode( mode ) will emit the 
completionModeChanged signal.

Moreover you should add a check whether the completion mode did actually 
change at the begin of RecipientsView::setCompletionMode(), i.e.
  if ( mode == mCompletionMode )
    return;
It's advisable always to do this in setters which do more than just 
assign a value to a member varialble.

> 2) AddresseeLineEdit
> - CompletionAuto will use these strings for lookup
> nickname <email>
> given last <email>
> last given <email>
> email
> - Other CompletionModes (Popup) will use KMailCompletion
> same lookups but popup contains always addr.fullEmail()
> - removed PopupAuto and CompletionMan from RMB-menu

-#include <kdebug.h>
+#include <kde/kdebug.h>
??? This change can't be necessary.

I think the code would be a bit better readable if you'd add
   for ( it = emails.begin(); it != emails.end(); ++it ) {
+    const QString email = *it;
and then replace (*it) by email everywhere in the body of the for-loop.

Why do you change the line which sets byLastName? familyName and 
givenName should definitely be separated by a comma. And since a comma 
makes quoting necessary we can immediately add the quotes instead of 
calling KPIM::quoteNameIfNecessary().

You could add
  const QString domain = email.mid( email.find('@')+1 );
to the list of keywords.

I don't see a reason to add
  givenName + " " + familyName
and the other combination to the keywords. Moreover, if for example the 
familyName would be missing then the givenName would not be added to 
the keywords.

You can prepend QString givenName, etc., with "const ".

The following code looks evil:
+    QPopupMenu *subMenu = menu->findItem( menu->idAt(12) )->popup();
+    subMenu->setItemVisible( subMenu->idAt(4), false ); // 
KGlobalSettings::CompletionMan
+    subMenu->setItemVisible( subMenu->idAt(5), false ); // 
KGlobalSettings::CompletionPopupAuto 

Did you count menu items to find the correct numbers? You must not do 
this. I'm not even sure the order of the items is the same for all 
languages. But even if the order is always the same for all languages 
the items might be reordered for some reason. You have to find another 
way to disable the unwanted completion modes. Alternatively, you could 
show an error message (KMessageBox::sorry()) when the user selects one 
of the unsupported completion modes.


In KMailCompletion:

+    //KCompletion.ignoreCase does not work, bug or feature?
+    match = KCompletion::makeCompletion( string.left(1).upper() + 
string.mid(1) );

Why should it be always correct to capitalize the first character? In 
any case all matching should be done case insensitive. If 
KCompletion.ignoreCase doesn't work then apply QString::lower() to all 
involved strings.

I don't understand the code in if ( !match.isEmpty ) {...}. Why are 
matches containing '@' or '<.*>' excluded? And why is the while-loop 
exited when match is an email address (which is pretty much impossible 
because then it would have contained a '@' in the first place and the 
while-loop would already have been exited; or am I missing something?)?

I would add a
  if ( !keyWords )
    return;
at the begin of KMailCompletion::addItemWithKeys(). Pointer arguments 
should always be checked for being non-NULL. The same needs to be done 
in KMailCompletion::postProcessMatches(). If a NULL value should never 
occur then adding an assert would probably be better because this way 
one could find programming errors.

> 3) LDAP will repeat last doComplete instead of new one
>
> 4) Cancel should no longer delete line
>
> I think that there is a bug in KCompletion::substringCompletion, fix
> is included in kcompletion_substringCompletion.diff
> I am not sure if this should go into 3.5, as it can break other
> peoples workarounds. mine for example :-)

I won't comment on this one.

> and then there are the double quotes from
> Addressee::setNameFromString This method cannot handle strings with
> quotes
> "last, first" --> givenName = first"; familyName="last
> fixed in addressee_setNameFromString.diff

You should change !str.isEmpty() to s.length() >= 2 because otherwise 
s.length() - 2 in the next line will be negative if str == "\"".
Moreover a comment would be nice, e.g.
// Remove enclosing double quotes
or something similar.

Last, but not least, a general comment: Please check the code for 
correct usage of whitespace. There are several spaces missing before or 
after parenthesis. Moreover, the curley braces '{' which start a method 
should be on a line of their own. Did we already refer you to the 
kdepim coding style guide?

And please put KMailCompletion into files of its own.

Regards,
Ingo

[Attachment #5 (application/pgp-signature)]

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