Review Request: Make it possible to start conversations from the Quick Chat plasmoid

[David Edmundson]

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


I like the concept. 

I've got a big "no" on the review, for a TpQt reason that isn't obviously clear from just reading the docs.


plasmoid/declarative-plugin/pinnedcontactsmodel.cpp
<http://git.reviewboard.kde.org/r/107811/#comment18835>

    Argh!
    
    We now have two account factories in the same application. One from here, and one in the client registrar.
    
    If you have two account factories, you can end up with different Tp::Account* objects representing the same account, such that account1 != account2 even though account1->uniqueIdentifier() == account2->uniqueIdentifier();
    
    I guess you may have figured this out because you have the line:
    "if p.account->uniqueIdentifer == account->uni..." rather than the more obvious  "if p.account=account"
    
    This isn't a situation I want to be in. Is there a way to share the factories (or the AccountManager) between here and the TelepathyTextObserver?
    It may need a bit more moving code about, but it'll be worth it.
    
    



plasmoid/declarative-plugin/pinnedcontactsmodel.cpp
<http://git.reviewboard.kde.org/r/107811/#comment18834>

    what file does this write into?
    
    AFAIK plasma has a special config group you're meant to use.



plasmoid/declarative-plugin/pinnedcontactsmodel.cpp
<http://git.reviewboard.kde.org/r/107811/#comment18832>

    qdebug -> kdebug
    
    (everywhere)



plasmoid/declarative-plugin/pinnedcontactsmodel.cpp
<http://git.reviewboard.kde.org/r/107811/#comment18831>

    if the pending operation fails to find the contact (like if you've pinned a contact you've now removed) you get a crash.
    



plasmoid/declarative-plugin/pinnedcontactsmodel.cpp
<http://git.reviewboard.kde.org/r/107811/#comment18833>

    if () {
    
    }


- David Edmundson


On Jan. 2, 2013, 5:34 p.m., Aleix Pol Gonzalez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/107811/
> -----------------------------------------------------------
> 
> (Updated Jan. 2, 2013, 5:34 p.m.)
> 
> 
> Review request for Telepathy and David Edmundson.
> 
> 
> Description
> -------
> 
> I've wanted to use this plasmoid for longtime, I fear that the reason why I'm not using it yet is because I can't start chats from there. The plan is that this will improve the situation, to some extent.
> 
> The important part about the patch is that the root element is refactored into a grid that can have different elements. Now I added a button with a contactlist that probably should go, but eventually i'd like to have non-conversation buttons so that one can pin contacts.
> 
> 
> Diffs
> -----
> 
>   plasmoid/declarative-plugin/CMakeLists.txt 48ba8a7 
>   plasmoid/declarative-plugin/contactpin.h PRE-CREATION 
>   plasmoid/declarative-plugin/contactpin.cpp PRE-CREATION 
>   plasmoid/declarative-plugin/conversation-target.h cd45f2d 
>   plasmoid/declarative-plugin/conversation-target.cpp f9c285d 
>   plasmoid/declarative-plugin/conversation.h 6eeab86 
>   plasmoid/declarative-plugin/conversation.cpp 152d940 
>   plasmoid/declarative-plugin/conversations-model.h f9dc047 
>   plasmoid/declarative-plugin/conversations-model.cpp faaa60b 
>   plasmoid/declarative-plugin/ktp-metatypes.h PRE-CREATION 
>   plasmoid/declarative-plugin/pinnedcontactsmodel.h PRE-CREATION 
>   plasmoid/declarative-plugin/pinnedcontactsmodel.cpp PRE-CREATION 
>   plasmoid/declarative-plugin/qml-plugins.cpp 23a4291 
>   plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ChatWidget.qml ea68f41 
>   plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ContactList.qml PRE-CREATION 
>   plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ConversationDelegate.qml 8a8d851 
>   plasmoid/org.kde.ktp-chatplasmoid/contents/ui/ConversationDelegateButton.qml PRE-CREATION 
>   plasmoid/org.kde.ktp-chatplasmoid/contents/ui/main.qml feb766b 
> 
> Diff: http://git.reviewboard.kde.org/r/107811/diff/
> 
> 
> Testing
> -------
> 
> Very little, mostly sending this review for starting a discussion on where we'd like to go with this plasmoid.
> 
> 
> Screenshots
> -----------
> 
> i am pinned
>   http://git.reviewboard.kde.org/r/107811/s/952/
> pin me
>   http://git.reviewboard.kde.org/r/107811/s/953/
> 
> 
> Thanks,
> 
> Aleix Pol Gonzalez
> 
>

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