[Telepathy] Work on telepathy-qt service
kiagiadakis.george at gmail.com
Mon Mar 25 03:10:20 PDT 2013
On Mon, Mar 11, 2013 at 6:34 AM, Matthias Gehre <M.Gehre at gmx.de> wrote:
> I have been working on a connection manager for whatsapp  using
> telepathy-qt. As the telepathy-qt service part is lacking, I implemented 
> the interfaces I needed (see appendix). My changes mainly add code and only
> touch code in the service part (which is not built by default), so I hope
> this can be merged. I would like your opinions on what how to improve the
> code before merging.
> The branch also includes three generic commits: One which replaces
> QString::toAscii with QString::toLatin1, because the former is no longer
> available in QT 5. On which does the same for QWeakPointer -> QPointer.
> And a commit that makes cmake find QT5. Do not merge that commit, it will
> break QT4. Some better way has to be found there.
> Best wishes,
> Implemented interfaces:
>  https://github.com/mgehre/telepathy-whosthere
>  https://github.com/mgehre/telepathy-qt
Sorry for the delayed reply. It is great to see somebody is working on
this feature of tp-qt and that there is at last a connection manager
implemented with Qt :)
I had a quick look at your branch and it looks like you are doing some
nice work. I would like to request a few things before I make a proper
review of the logic of the code:
1) Please try to polish the commits so that they are easier to review:
squash unnecessary "fix" commits into others, so that there are no
code removals in new code, and in general try to adopt a commit
history that looks like:
* Implement Foo
* Implement Bar
...without anything else. This will make it much easier for me to review.
2) On coding style: Please go over the code again and try to use the
coding style of the rest of tp-qt. You have done it for most of the
code, but not everywhere. For example:
* void BaseChannel::setInitiatorID( const QString& initiatorID )
-> void BaseChannel::setInitiatorID(const QString &initiatorID)
* #ifndef BASECHANNEL_H
-> #ifndef _TelepathyQt_base_channel_h_HEADER_GUARD_
3) Your code is missing a copyright & license. It obviously cannot be
merged like this.
4) To follow the style of the rest of tp-qt, pretty headers should
exist for every class, not just for BaseChannel.
This is all for now. Thanks a lot for your work and I hope to see it merged.
More information about the telepathy