Review Request: Baby steps for Chat Plasmoid...

[David Edmundson]

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


Enough comments to keep you busy for a while :-)


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

    This class is full of potential crashes.
    
    By default this class is made with a Tp::ContactPtr which doesn't point to any actual contact
    
    that means all the code that says
    
    d->contact->presence()->something()
    
    will (probably) just explode horribly if the contact isn't set.
    
    
    Is there a reason for needing a setContact() method, rather than just forcing you to set a valid one in the constructor?
    
    If not - simplify your code, just force a valid Tp::ContactPtr in the constructor.
    
    If so - we need to fix the code up in the accessors to be safer.



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

    const Tp::ContactPtr &contact



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

    still put const & stuff in slots.
    
    (though don't put them in your "connect(this, SIGNAL( ..." line.



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

    QIcon -> KIcon



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

    Why would you need the icon name in rather than a KIcon?



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

    I'd rather we used Ktp::PResence here
    (that's new, I'll help with that)
    
    



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

    When could this happen?
    
    (or the one beneath)



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

    data is a poor name for a variable, it could be anything.



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

    IIRC you need to check channel types here too. Check code from main Text-UI.
    



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

    Actually you can. (such as the int that you have in the line above!) 
    
    But you would never need to deal with textChannel stuff directly from QML, so I would delete this.



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

    Whitespace in this class is a bit mental.



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

    it's fine to keep debug code in here, but comment it as such.



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

    a slot that is non-void is unusual.



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

    I bet there's a proper method to do this.



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

    que -> queue


- David Edmundson


On Jan. 4, 2012, 1:47 p.m., Lasath Fernando wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/103629/
> -----------------------------------------------------------
> 
> (Updated Jan. 4, 2012, 1:47 p.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/chat-window-style.cpp 7b770cf 
>   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/20120105/77020671/attachment-0001.html>