[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-25 21:25:49
Message-ID: 200609252325.51221 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Monday 25 September 2006 23:02, Chr. Sch. wrote:
> > The following code looks evil:
>
> it is!
> got the idea from looking at KLineEdit::createPopupMenu
>
> > +    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.
>
> I don't like the idea of offering options which are disabled.
> the only "nice" way I see is to extend KLineEdit with
> disableCompletionMode( KGlobalSetting::Completion )
> and then check in KLineEdit::createPopupMenu() whether mode is
> disabled or not. Will this suggestion have a chance to get into SVN ?

Probably not, but it can't hurt to ask. Can you prepare a patch and also 
a good explanation for why we can't support those two completion modes 
in KMail?

> > 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
>
> it is not always correct, but for names it should do the trick.

Hmm, I'm not really convinced that this is a good idea.

> > any case all matching should be done case insensitive. If
> > KCompletion.ignoreCase doesn't work then apply QString::lower() to
> > all involved strings.
>
> but then all the matches will be lower case as well ...
> there must be an other way...

Fix KCompletion.ignoreCase. :-)

> > 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?)?
>
> Completion contains key words and complete emal addresses of the
> form name <email> (or "name" <email>, if we agree upon using the
> quoted version). Before we return matches to the caller, all key
> words have to be removed/replaced. For substringCompletion/allMatches
> this is done in postProcessMatches by converting keyword to
> fullEmail. makeCompletion is a little different because it is called
> only for CompletionAuto, we have to return a string that contains an
> email and starts with searchString
>
> if match contains '@' or '<.*>' this is an email and we can return
> the match directly. The rest of the loop is to skip key words. The
> isEmail part is for local addresses without domain.

Are raw email addresses not used as keywords?

> and yes, dist lists are missing, I'll fix that

Great. And thanks for the explanation. The code really seems to be 
rather complicated. Maybe we should throw a Marc Mutz on the original 
developers to teach them about the Strategy pattern.

> > > 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.
>
> has been fixed (thanks to  Carsten Pfeiffer) I'll remove the
> workaround in the next version ot the patch.

Excellent.

> > 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?
>
> old habbits die hard... Is there a beautifier out there you can
> recomend?

I don't know of such a thing. If you are using X?Emacs then you can 
install the kde-emacs extension from kdesdk. This extension will 
automatically add spaces after parenthesis and some other stuff.

> > And please put KMailCompletion into files of its own.
>
> I know... Getting to know the code and the build system all at once
> was a bit to much for starters. But now that source code and I are
> starting to become good friends :-), I'll move the class to a
> seperate file.
>
> Thank you for taking the time to make such detailed comments.

No problem. We have to thank you for working on this.

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]