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