<div dir="ltr"><div><div>Hello George,<br><br>thanks for your comments!<br><br></div>I did everything you suggested. <br>1) Split the classes into single commits, squashed all other commits.<br>2) Cleanup the coding style, additionally run the astyle command from kdelibs style page.<br>
</div><div>3) Added copyright and license<br></div><div>4) Added pretty headers for every class<br><br></div><div>Took me quite some while, though.<br><br></div><div>You can find the update branch at <a href="https://github.com/mgehre/telepathy-qt/tree/master">https://github.com/mgehre/telepathy-qt/tree/master</a><br>
</div><div><br>The cmake change for QT5 is at <a href="https://github.com/mgehre/telepathy-qt/tree/qt5">https://github.com/mgehre/telepathy-qt/tree/qt5</a><br></div><div>if you want to run it under QT5 final.<br><br></div>
<div>Best wishes,<br>Matthias<br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2013/3/25 George Kiagiadakis <span dir="ltr"><<a href="mailto:kiagiadakis.george@gmail.com" target="_blank">kiagiadakis.george@gmail.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Mar 11, 2013 at 6:34 AM, Matthias Gehre <<a href="mailto:M.Gehre@gmx.de">M.Gehre@gmx.de</a>> wrote:<br>

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