[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kopete-devel
Subject:    Re: [kopete-devel] Skype plugin
From:       Matt Rogers <mattr () kde ! org>
Date:       2005-05-08 0:01:19
Message-ID: 200505071901.22903.mattr () kde ! org
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Saturday 07 May 2005 01:37 pm, Michal Vaner (Vorner) wrote:
> I've been working on the skype plugin for a while and I decided I will
> publish it now. It supports chats (one to one, conferences are not
> supported yet), loading contacts on startup and online statuses. Of course,
> there is really much to do still.
>
> Please look at it someone and tell me if I should continue and ask for SVN
> account to maintain it.
>
> I thing there will be something that will not suit too well with KDE
> policies, if so, tell me, I will try to change to.
>
> It requires DBus libraries and dbus-capable skype
> ( http://www.skype.com/community/devzone/SkypeAPIforLinux.html )
>
> Because the patch was too big, I put it here: (it has something about 200
> kB) http://skype-kopete.webzdarma.cz/skype.patch
>
> Thank you a lot.

Comments and nitpicks.

- get rid of qDebug() in favor of kdDebug(). qDebug() doesn't turn itself off 
in linux when compiled w/o debug info. (however, i wouldn't do this inside of 
the d-bus integration stuff)
- add proper copyright for yourself.
- follow "standard" kopete license and copyright style. see 
kdenetwork/kopete/protocols/oscar/liboscar/ or kdenetwork/kopete/libkopete 
for examples.
- too many comments, not every line needs a comment. the doxygen comments for 
the header are ok, although some of them have really long lines that should 
be split up
- the disconnect call in SkypeChatSession::knowId(const QString& id) is wrong, 
IMHO. it should be:
  disconnect(&d->account, SIGNAL(gotMessageId(const QString&)), this, 
SLOT(knowId(const QString&)))
- the following members of the d-pointer should be pointers, IMHO:
  SkypeProtocol &protocol => SkypeProtocol *protocol
  SkypeAccount &account => SkypeAccount *account
- the constructList function could be moved into the d-pointer, but that's 
purely a style thing.
- you don't need to cast 0L to (char*)
- includes of the form '#include "skypedbus/skypeconnection.h' should become
'#include "skypeconnection.h"' and the full path to the skypedbus directory 
should be added to the Makefile.am. Also, please do the same with other 
relative pathes in the files.
- The Skype constructor should take a SkypeAccount* instead of SkypeAccount& 
IMHO. 
- run fixuifiles over your .ui files to remove unnecessary accelerator text 
(the accelerators won't be changed, but the XML in the .ui file will be 
removed so that translators will have less to translate.
- indicate in the configure.in.bot for protocols what people will need to get 
skype support working.

Hope these initial comments help. I haven't actually taken the time to test 
your patch yet though, and assume that it does work. You should probably ask 
for an svn account if you do indeed intend to maintain and work on the skype 
support for the long term. :)

Thanks,
Matt

[Attachment #5 (application/pgp-signature)]

_______________________________________________
kopete-devel mailing list
kopete-devel@kde.org
https://mail.kde.org/mailman/listinfo/kopete-devel


[prev in list] [next in list] [prev in thread] [next in thread]