[Kde-pim] KMail Configuredialog

[Thomas McGuire]

Hi,

sorry for the very late reply.

On Wednesday 17 December 2008 21:32:42 Torgny Nyblom wrote:
> I'm working on a patch to extract the ui code for configuredialog into .ui
> files.
>
> How is this patch (and the rest as the other pages get converted) affected
> by the string/feature freezes?
>
> Patch at:
> http://www.nyblom.org/pub/kde/kmail-configdialog-ui.patch
> Right now it only extracts the Misc page.

First of all: thanks for doing this!
Now comments about the patch:

The layout of the folders tab looks wrong. According to 
http://wiki.openusability.org/guidelines/index.php/Design_and_Layout:Layout:Dialogs, 
the labels should be right-aligned.
In a general note, I would prefer first porting the dialogs 1:1 to UI files, 
committing, then doing layout changes. I agree that the current folders tab 
needs a layout overhaul.

Wrong label for the "Enable IMAP resource functionality" checkbox.

At least in the groupware tab, some accelerators changed. I don't know if that 
is important, or even which of the versions is better.

In the "Exchange compatible invitation naming" tooltip, I see <br/>s.

The set(kmail_configure_dialogs...) in the CMake file looks unused.

For the location of the UI files: I'd say we should move all of them (also the 
old ones) in a subdir named "ui", no specific subfolder for the configure 
dialog.

I think it should be also possible to use custom classes like 
KMail::FolderRequester in UI files. After all, that is possible for KCombobox 
& co, so it should be possible here as well.

Please re-check your coding style, for example indentation in 
MiscPageFolderTab::MiscPageFolderTab() is wrong, and sometimes spaces around 
parenthesis. Sorry that I'm always so picky about this.

The "TODO: 4.3 Change these strings into something better." comment now is 
pretty lonely, without any strings nearby :)

There are still some config entries that use the old-style format, not the 
.kfcg, like "empty-trash-on-exit", it would be nice to move them to KConfigXT 
eventually.

Can we also get rid of lines like those:
> mMGTab->grid->setMargin( 0 );
> mMGTab->grid->setColumnStretch( 1, 1 );
There are also some lines with KDialog::spacingHint(), not sure if we or how 
can remove that, so it probably needs to stay.

QStackedWidget are also in designer, any reason why you didn't use it?

It looks like you removed the context from the "When trying to find unread 
messages:" string, and maybe others. I guess there is a way to add those to UI 
files as well, the context is important for translators.

I didn't look at the code changes to closely and hope I didn't overlook other 
things here, one always has to be very careful not to loose or change 
anything.

After porting one page to UI files, it might be worth looking at 
KConfigDialogManager, which can automatically save and load settings from 
GlobalSettings (which inherits KConfigSkeleton). This could save some code in 
the save() and load() functions.
I wonder if it is also possible to get rid of the setWhatsThis() calls this 
way, after all the KConfigSkeleton knows them already, why not set them to the 
widgets when doing KConfigDialogManager::updateWidgets()? that would probably 
require changes to KConfigDialogManager, but I think it is worth a try.

As Ingo said, please commit right after the release when trunk is open again, 
and post a new patch addressing the issues. Looking forward to the patch.

Thanks for your great work, 250 lines of code less and much easier to change 
layouts :)

Regards,
Thomas
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 194 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-pim/attachments/20081222/b4399783/attachment.sig>
-------------- 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/