----------------------------------------------------------- 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 > >
_______________________________________________ KDE-Telepathy mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-telepathy
