Telepathy Contact Applet Repo goes public!

[David Edmundson]

First set of review comments for the contact applet.

Your file naming convention is new.
We use my-class-name.cpp
You use MyClassName.cpp
Lots of other code uses myclassname.cpp which would have possibly been
ok, but making something else new isn't.

In your UI file:
 - you don't need spacers to centre align text, just set the QLabel to
be centre aligned, and expanding. But in any case KTitleWidget would
be more appropriate to use for title text.

Pedantic spelling:
 wether -> Whether
 rappresented -> represented

Fetch the new abstract-contact-delegate from the contact list.

In TelepathyContact.cpp:
 Rather than looping through all known contacts, use
ContactManager::ContactForIdentifier.
For bonus points, remove everything from ContactFactory, and only
request Contact features here. This will reduce dbus-traffic and make
your applet faster.

Don't keep the config UI in memory the whole time, you may as well
just create it (and connect signals/slots) when needed. Otherwise we
have an entire model of every contact being kept up-to-date all the
time for no reason. This will need some large changes as you would
need to move where your AccountManager is, but I think the config
class is an odd place to put something crucial to the general running
of the main applet.

I would pass AccountManager as an argument to creating the config
class. I think if you do that you'll make the code a bit easier to
read/understand.

Rather than looping through every account in account manager (line 83
config.cpp) can you use AccountManager::accountForPath() ?

It may be worth playing with Applet::configurationRequired() for first uses.

Dave

On Sat, Sep 3, 2011 at 4:09 PM, Francesco <francesco.nwokeka at gmail.com> wrote:
> On Saturday 03 September 2011 15:19:35 David Edmundson wrote:
>> On Sat, Sep 3, 2011 at 2:32 PM, Francesco <francesco.nwokeka at gmail.com>
> wrote:
>> > Yay!
>> >        My work on the contact-applet has gone public[0]! Just give some
>> > time for the kde sysadmins to update and add it to the reviewboard and
>> > you guys can start fixing/reporting bugs (if there are any :P).
>> > I'd like if someone could give a quick look at my code to see if there
>> > are things out of place or that could be optimized.
>> >
>> > @devs: I'll be adding the applet to our wiki.I'll also need to make the
>> > 0.1 release and inform packagers right?
>>
>> Lets do a proper code review first.
>>
>> To me it seems a bit weird to add things to a release. Let's see the
>> timings for everything else (0.2). Add this to the sprint agenda.
> Fine with me
>
>>
>> > [0]https://projects.kde.org/projects/playground/network/telepathy/telepa
>> > thy- contact-applet
>> > _______________________________________________
>> > KDE-Telepathy mailing list
>> > KDE-Telepathy at kde.org
>> > https://mail.kde.org/mailman/listinfo/kde-telepathy
>>
>> _______________________________________________
>> KDE-Telepathy mailing list
>> KDE-Telepathy at kde.org
>> https://mail.kde.org/mailman/listinfo/kde-telepathy
>