Review Request 113824: Refactor of Contact plasmoid

[Martin Klapetek]

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


Good that you're working on this! Thanks, below are couple points...


contact/src/contact-wrapper.h
<http://git.reviewboard.kde.org/r/113824/#comment31408>

    isAccountOnlineChanged() is a weird name for a signal.
    
    Either keep the original or "accountOnlineChanged()"



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31407>

    You can also do 
    
    return m_account && m_account->currentPresence().type() != Tp::Presence::offline().type();
    
    ...but up to you.



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31409>

    I don't think that's correct - account can be offline, in which case contact has presence status offline



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31410>

    I know you're just moving it, but we could rename it in the process too - disconnectAccountSignals (Tp::Account has different kinds of connections ;)
    
    Possibly rename the setupAccountConnects too (connectAccountSignals?)



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31411>

    Same as above



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31412>

    Didn't you fix this in some previous patch?



contact/src/contact-wrapper.cpp
<http://git.reviewboard.kde.org/r/113824/#comment31413>

    If you comment debug, you might as well just remove it completely


- Martin Klapetek


On Nov. 13, 2013, 12:18 p.m., Alexandr Akulich wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/113824/
> -----------------------------------------------------------
> 
> (Updated Nov. 13, 2013, 12:18 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Repository: ktp-desktop-applets
> 
> 
> Description
> -------
> 
> TelepathyContact:
>     Removed qmlObject related stuff.
>     m_contact renamed to m_contactWrapper as it should be.
>     Setup context property before loading qml file.
> 
> ContactWrapper:
>     Properties placed there them should be.
>     Added property isContactOnline;
>     Added notifier-signals information for properties.
>     Property canStartVideo renamed to canStartVideoCall (like canStartAudioCall).
> 
>     undoAccountConnects() reimplemented to don't break connections possible maked outside.
>     Same about undoContactConnects().
> 
>     Methods reordered in cpp file to group simular methods together.
>     Implemented updateProperties(). In future it's possible to implement granulated properties updating.
>     Added checks for isAccountOnline in presenceStatus().
>     Fixed coding-style.
> 
> QML part:
>     Get rid a lot of functions, that anyway doesn't work properly.
>     Get rid a lot of useless properties.
>     Don't check avatarPresenceStatus for empty string and don't replace it with default one, because it's already done in c++ part.
>     Get rid a lot of "anchors.fill: parent" and such, because it's bad practice to let children position itself on parent.
>     Comminication buttons reordered like in KDE IM Contacts application.
> 
> Side effects:
>     Fixed losted after few went offline/online presenceStatus.
>     Fixed communication buttons remaining available after account becomes offline.
> 
> 
> Diffs
> -----
> 
>   contact/src/contact-wrapper.h 7b37907 
>   contact/src/contact-wrapper.cpp 5ef5919 
>   contact/src/declarative/contents/ui/Avatar.qml 6133563 
>   contact/src/declarative/contents/ui/Contact.qml ab434c8 
>   contact/src/declarative/contents/ui/DropDownMenu.qml 7177c70 
>   contact/src/declarative/contents/ui/main.qml debb0ed 
>   contact/src/telepathy-contact.h fe95c83 
>   contact/src/telepathy-contact.cpp 67c8a32 
> 
> Diff: http://git.reviewboard.kde.org/r/113824/diff/
> 
> 
> Testing
> -------
> 
> Plasmoid state properly response on account and/or contact online/offline changing. Contact capabilities showed correctly.
> 
> 
> Thanks,
> 
> Alexandr Akulich
> 
>

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