Review Request: Baby steps for Chat Plasmoid...

Tue Jan 10 10:00:35 UTC 2012

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > I skipped the qml part and the bitching about your nasty coding style, that will come in the next round :P Just two general coding-style issues: if you declare a pointer variable, the star goes to the right, eg. MyObject *object; ... and then ifs, fors etc - if (something) { -- watch for the proper spacing.
> > 
> > Also somewhere you have still "Que" left.

As for the ifs, I'll get KDevelop to fix all of them for me later.
And I know KDE style guide says * before the variable, but I've been programming the other way for 4years and it's a hard habit to break. Anyway, I'll fix those now.

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/conversation-target.h, line 57
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46001#file46001line57>
> >
> >     Why QIcon for avatar? QPixmap would be better (provided that qml can handle that).

Because most Plasma Components like ToolButton and KIconWidget take a QIcon as a parameter.

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/conversation-target.cpp, line 45
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46002#file46002line45>
> >
> >     Not a big deal, but it's usually better when you're using the same terminology everywhere, ie. alias vs. nick. 
> >     
> >     Also not really an issue, but the other ktp code uses contact.data() everywhere (instead of contact.constData()), though I don't think this matters that much.

okay, then which is better? alias or nick?

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/conversation-target.cpp, lines 70-106
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46002#file46002line70>
> >
> >     Use KTp::Presence instead, this method is already there. You can just construct it from Tp::Presence:
> >     
> >     KIcon ConversationTarget::presenceIcon() const
> >     {
> >         KTp::Presence presence(d->contact->presence);
> >         return presence.icon();
> >     }

I just did so on the presenceIcon() method, but some components need Icon source. I'm not sure how to get that using KTp::Presence

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/conversations-model.cpp, line 37
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46006#file46006line37>
> >
> >     You should check the index altogether by index.isValid().

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/messages-model.h, line 74
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46007#file46007line74>
> >
> >     Why is this bool? Afaics you use it only to output debug to console, so it does not really have to be bool. And it's weird.

I made it bool originally because I used it in quite a few onCompleted() handlers.
i.e. :
if(verifyPendingOperation(result)) {
    actually do something;
}

I'd like to keep it because I don't think it's causing any harm...

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/messages-model.cpp, line 48
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46008#file46008line48>
> >
> >     Are you sure you didn't want MessagesModelPrivate?

coz I'm *that* brilliant at renaming stuff ;-)

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/messages-model.cpp, line 248
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46008#file46008line248>
> >
> >     Again too early pressed enter :P

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > lib/messages-model.cpp, lines 250-251
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46008#file46008line250>
> >
> >     Your keyboard seems to have some trouble with the enter key

I think it was the source formatter derping out..

> On Jan. 9, 2012, 12:36 p.m., Martin Klapetek wrote:
> > plasmoid/metadata.desktop, lines 11-12
> > <http://git.reviewboard.kde.org/r/103629/diff/3/?file=46019#file46019line11>
> >
> >     I know this guy and he does not work on ktp.

lol. that file has remained unchanged for sooooooooooo long...

- Lasath

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/103629/#review9658
-----------------------------------------------------------

On Jan. 9, 2012, 11:52 a.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103629/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2012, 11:52 a.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> Right, I decided that I should have reviewed and/or merge what work I've done so far rather than letting it pile up in its corner into a giant worthless clump. 
> 
> To the people that haven't heard (or wasn't at woshibon), this is a chat plasmoid that more or less behaves like facebook and google talk, except it sits in your taskbar :)
> 
> And in terms of feedback, at this stage I think design issues should take priority over sane code because the main reason I'm doing this is because I don't want to have to do any massive restructuring later on.
> 
> And if things don't make sense, ask me (I didn't comment/document anything well and I'm certainly too sleepy now ;)
> 
> PS: All the code is in my scratch repo
> http://quickgit.kde.org/?p=clones%2Ftelepathy-text-ui%2Ffernando%2Fqmlplugin.git&a=shortlog&h=refs/heads/qml_plugins2
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt d1cc185 
>   lib/CMakeLists.txt 5d39a62 
>   lib/conversation-que-manager.h PRE-CREATION 
>   lib/conversation-que-manager.cpp PRE-CREATION 
>   lib/conversation-target.h PRE-CREATION 
>   lib/conversation-target.cpp PRE-CREATION 
>   lib/conversation.h PRE-CREATION 
>   lib/conversation.cpp PRE-CREATION 
>   lib/conversations-model.h PRE-CREATION 
>   lib/conversations-model.cpp PRE-CREATION 
>   lib/messages-model.h PRE-CREATION 
>   lib/messages-model.cpp PRE-CREATION 
>   lib/qml-plugins.h PRE-CREATION 
>   lib/qml-plugins.cpp PRE-CREATION 
>   lib/qmldir PRE-CREATION 
>   lib/telepathy-text-observer.h PRE-CREATION 
>   lib/telepathy-text-observer.cpp PRE-CREATION 
>   plasmoid/CMakeLists.txt PRE-CREATION 
>   plasmoid/contents/ui/ChatWidget.qml PRE-CREATION 
>   plasmoid/contents/ui/ConversationDelegate.qml PRE-CREATION 
>   plasmoid/contents/ui/TextDelegate.qml PRE-CREATION 
>   plasmoid/contents/ui/main.qml PRE-CREATION 
>   plasmoid/metadata.desktop PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/103629/diff/diff
> 
> 
> Testing
> -------
> 
> Um.. yeah... about that...    :/
> 
> 
> Thanks,
> 
> Lasath Fernando
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120110/f923b954/attachment-0001.html>