Review Request: Baby steps for Chat Plasmoid...

Mon Jan 9 12:36:28 UTC 2012

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

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.

lib/conversation-target.h
<http://git.reviewboard.kde.org/r/103629/#comment7925>

    Why QIcon for avatar? QPixmap would be better (provided that qml can handle that).

lib/conversation-target.h
<http://git.reviewboard.kde.org/r/103629/#comment7924>

    KIcon if possible

lib/conversation-target.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7920>

    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.

lib/conversation-target.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7921>

    KIcon

lib/conversation-target.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7922>

    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();
    }

lib/conversation-target.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7923>

    Get rid of the presenceIconSourceChanged(..) and use only

    KTp::Presence kpresence(presence);
    Q_EMIT presenceIconChanged(kpresence.icon());

    (add the "presence" as the param name in teh declaration)

lib/conversations-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7926>

    You should check the index altogether by index.isValid().

lib/conversations-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7927>

    You should cast it to Conversation* rather than QObject*

lib/messages-model.h
<http://git.reviewboard.kde.org/r/103629/#comment7928>

    What kind of sorcery is this??

lib/messages-model.h
<http://git.reviewboard.kde.org/r/103629/#comment7929>

    constify

lib/messages-model.h
<http://git.reviewboard.kde.org/r/103629/#comment7931>

    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.

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7930>

    Are you sure you didn't want MessagesModelPrivate?

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7932>

    Looks like you hit enter too early ;)

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7934>

    I think you can 'break' out after you have found it, there's no point for the loop to continue.

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7935>

    Check for index.isValid().

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7936>

    Again too early pressed enter :P

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7937>

    "Que" -> "Queue"

lib/messages-model.cpp
<http://git.reviewboard.kde.org/r/103629/#comment7938>

    Your keyboard seems to have some trouble with the enter key

plasmoid/metadata.desktop
<http://git.reviewboard.kde.org/r/103629/#comment7939>

    I know this guy and he does not work on ktp.

- Martin Klapetek

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/20120109/8f0c34e5/attachment-0001.html>