KTp Contact runner

David Edmundson david at davidedmundson.co.uk
Sun Mar 11 23:12:43 UTC 2012


Nicely done. I'm all for getting this into 0.4.

Review comments:

1)
Strings:
"behaviour" -> "behavior" (sorry, IMHO you spelt it correctly)
"that are capable of text chat" -> "that are capable of text chats"
(same for audio/video)
"as default action" -> "as the default action"
"sens file" -> "send a file".

2)
    if (contactItem->data(AccountsModel::FileTransferCapabilityRole).toBool())
        actions.append(action("start-file-transfer"));

you also need to check the capabilities of selfContact. If the other
person can send/recieve files but you can't, then you can't show this
option.
(same for call and video)

3)
         switch (contactIndex.data(AccountsModel::PresenceTypeRole).toInt()) {
                case Tp::ConnectionPresenceTypeAvailable:
                    iconName = "im-user";

use methods in KPresence for getting the correct icon name.

4)

Also I'm not sure I like this idea of showing the avatar if there's
one available, otherwise showing the status. If the user has an avatar
you wouldn't get to see their status which is a bit lame. Maybe some
sort of overlay of presence on top of avatar.

5)

Watch your coding style:
http://techbase.kde.org/Policies/Kdelibs_Coding_Style#Braces

if (foo)
  bar();

should be written as

if (foo) {
  bar();
}


Misc brainstorming:
This may be wrong but; rather than keeping an entire model up to date
constantly in krunner, it may be less wasteful to query each
ContactManager for alContacts() each time in match(). Obviously that's
a very large rewrite, so I'm not really suggesting you do it, just
thinking out loud. I'm not even sure you can do async stuff in
AbstractRunner::match().


More information about the KDE-Telepathy mailing list