[Kde-pim] [RFC, Patch] KMail account wizard

[Thomas McGuire]

Hi,

On Thursday 14 August 2008 22:16:01 Togge wrote:
> I've been working on getting KMail to be able to show the account wizard
> from the toolbar and to get it to be able to handle multiple
> accounts/identities.
>
> I've made a patch that I could use some feedback on:
> http://www.nyblom.org/pub/kde/kmail-accountwizard.patch

%LATE_REPLY_APOLOGY

Thanks for the patch. There are some problems which need to be fixed before it 
can be committed:

When invoking the wizard from the tool menu, it says "It seems you have 
started KMail the first time...". Please fix that.

The layout of the identity chooser page is broken, see attached screenshot.

The identity which created is named "Unnamed". Maybe use the same naming 
scheme as for the auto-created accounts?

When re-using an existing identity, it overwrites the special transport 
setting. Please don't change the identity at all when reusing it. In fact, I 
think createIdentity shouldn't do anything when reusing an identity.

Factor out the "if( manager->identities().count() == 1 && !manager-
>defaultIdentity().mailingAllowed() )" into a common method to avoid code 
duplication, for example onlyDefaultIdentity() or something like that.

Also factor out the clear() calls to a separate method.
Actually, I wonder why this is called at all, isn't the text for those fields 
later overwritten in slotCurrentPageChanged() anyway, or am I missing 
something?

Coding style wrt spaces around parenthesis, for example in the 
setAppropriate() call or at "if( mIdentityButtonGroup...".
Change to "setAppropriate( mIdentityChooserPage, ..." and
"if ( mIdentityButtonGroup...".

BTW, overall I think the patch is quite good, minus the few issues mentioned 
above.

> The commented code in kmmainwidget.cpp, should that be there or not?

No, it should not be there, I removed it in r848394. If you're stumbeling on 
something which seems strange, use svn annotate / websvn to find out why that 
line of code is there. In this case, it was a merging error introduced in 
r691932.

> Any idea as to how I can avoid the duplicated code in accountwizard.cpp
> createIdentity()?

Err, you changed that already? Seems ok to me.

Regards,
Thomas



-------------- next part --------------
A non-text attachment was scrubbed...
Name: snapshot23.png
Type: image/png
Size: 34746 bytes
Desc: not available
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20080819/525406f0/attachment.png>
-------------- next part --------------
_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/