Review of Contact List Code

[Martin Klapetek]

2011/3/10 David Edmundson <david at davidedmundson.co.uk>

> This is really just for people working on the contact list.
>
> I've been doing a few minor edits on it recently and there is a lot of code
> that really needs tidying up before it can be merged. I thought I would
> highlight these here so we can all work on that, and get it fixed soon.
>
> 1) Coding Style
>
> We follow this
> http://techbase.kde.org/Policies/Kdelibs_Coding_Style
>
> In particular
> if (something) { <- brace on the end of the line. Always put the brace,
> even if it's for 1 line.
>
> }
>

Yes, I'm following this now with new code. Though I want to do a major code
polishing before merging it to the main tree. So all the code will follow
the kde style in the end (and I'n also trying to replace it whenever I see
it in the code I'm working with)


>
> --------
>
> 2) Don't hardcode fonts and colours in the delegate code.
>
> If you write font.setSize(10); that doesn't work if someone has vision
> issues or even just owns a massive resolution monitor.
>
> Use KGlobalSettings::smallestReadableFont()
>  - and its equivalents for other fonts.
>

Ok.


>
> With this sizeHint code needs to be more complex, and use font metric
> information to work out the minimum height the box needs to be.
>

Well the minimum size is determined by the icon size, so as long as we use
constat icon size, we can leave the minimum size same as the icon size (+
some padding). Though it makes sense to return bigger sizeHint in case of
bigger fonts. I'll try to take care of this.


>
> Same for colours. If you switch your colour scheme in systemsettings to
> "Obsidian Coast" a dark colour scheme, your delegate with the hardcoded
> background colour stands out really obviously as being wrong.
> QPalette should have everything you need.
>

Ah, I didn't thought about that. Very good point, thanks. I'll work on this
as well.


>
> ---------
>
> 3) Looking at whether the alias is blank or not seems a slightly hacky way
> to try and separate headings in the model from contacts.
>
> I'm not sure what a better approach is, but there must be one.
>
>
Well the AliasRole is classified as a contact role, so it *should* be
non-empty only if the current item is contact. I have to look at the Tp-Qt4
Yell how they handle this.


> --------
>
> Overall it's still pretty awesome, hopefully this will speed up the process
> of having it merged into the real master.
>

> I'm happy to help with any of these.
>

You could do something about those fonts and the sizeHint() ;)


Marty
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20110310/ca872a37/attachment.htm