On Mon, Mar 11, 2013 at 6:34 AM, Matthias Gehre <[email protected]> wrote: > Hi, > > I have been working on a connection manager for whatsapp [1] using > telepathy-qt. As the telepathy-qt service part is lacking, I implemented [2] > 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, > Matthias > > Implemented interfaces: > BaseChannel, > BaseChannelTextType, > BaseChannelMessagesInterface, > BaseChannelServerAuthenticationType, > BaseChannelCaptchaAuthenticationInterface, > BaseConnectionRequestsInterface > BaseConnectionContactsInterface > BaseConnectionSimplePresenceInterface > BaseConnectionContactListInterface > BaseConnectionAddressingInterface > > > [1] https://github.com/mgehre/telepathy-whosthere > [2] https://github.com/mgehre/telepathy-qt >
Hello, 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. Regards, George _______________________________________________ telepathy mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/telepathy
