Review Request 113824: Refactor of Contact plasmoid

[Alexandr Akulich]

> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20131125/9fc96910/attachment-0001.html>