> On Nov. 15, 2013, 4:14 p.m., Martin Klapetek wrote:
> > contact/src/contact-wrapper.cpp, lines 241-245
> > <http://git.reviewboard.kde.org/r/113824/diff/3/?file=213859#file213859line241>
> >
> >     Didn't you fix this in some previous patch?

This patch is refactor and next patch is implementation 
(https://git.reviewboard.kde.org/r/113832/ I'm surpriced, why there is 
"removed: Depends On: Refactor of Contact plasmoid" changed after diff r2 
uploading).


> On Nov. 15, 2013, 4:14 p.m., Martin Klapetek wrote:
> > contact/src/contact-wrapper.cpp, line 268
> > <http://git.reviewboard.kde.org/r/113824/diff/3/?file=213859#file213859line268>
> >
> >     If you comment debug, you might as well just remove it completely

It's my mistake to include it to the patch. I know nothing about this message 
utility.


> On Nov. 15, 2013, 4:14 p.m., Martin Klapetek wrote:
> > contact/src/contact-wrapper.cpp, line 204
> > <http://git.reviewboard.kde.org/r/113824/diff/3/?file=213859#file213859line204>
> >
> >     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?)

Done.


> On Nov. 15, 2013, 4:14 p.m., Martin Klapetek wrote:
> > contact/src/contact-wrapper.h, line 112
> > <http://git.reviewboard.kde.org/r/113824/diff/3/?file=213858#file213858line112>
> >
> >     isAccountOnlineChanged() is a weird name for a signal.
> >     
> >     Either keep the original or "accountOnlineChanged()"

Done


> On Nov. 15, 2013, 4:14 p.m., Martin Klapetek wrote:
> > contact/src/contact-wrapper.cpp, line 184
> > <http://git.reviewboard.kde.org/r/113824/diff/3/?file=213859#file213859line184>
> >
> >     I don't think that's correct - account can be offline, in which case 
> > contact has presence status offline

You read my mind, but I just tested it again. Contacts presence remains the 
same after account disconnection. To test it added Text item to main.qml and 
does
text: TelepathyContact.presenceStatus + ", but isAccountOnline is " + 
TelepathyContact.isAccountOnline
It show, e.g. "away, but isAccountOnline is false". May be there is some bug in 
underlying components (I use git-master versions of ktp components, but 
telepathy-qt is only 0.9.3).


- Alexandr


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


On Nov. 13, 2013, 6: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, 6: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

Reply via email to