Review Request: adiumxtra-protocol-handler: supporting chat styles and emoticons

[Dominik Schmidt]

> On 2010-09-29 20:58:15, David Edmundson wrote:
> > /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.cpp, line 30
> > <http://svn.reviewboard.kde.org/r/5483/diff/1/?file=38709#file38709line30>
> >
> >     is "bla" a good name for this variable?

Oops :-D

Was just testing something ... 


> On 2010-09-29 20:58:15, David Edmundson wrote:
> > /trunk/playground/network/telepathy-chat-handler/lib/chatwindowstylemanager.cpp, line 216
> > <http://svn.reviewboard.kde.org/r/5483/diff/1/?file=38710#file38710line216>
> >
> >     This file doesn't actually need to exist. See Adium spec.
> >     
> >     
> >     also I would personally prefer:
> >     
> >     bool validResult = true;
> >     
> >     if (! currentDir...)
> >     {
> >      validResult = false;
> >     }
> >     if (! currentDir...)
> >     {
> >      validResult = false
> >     
> >     etc.

I just reenabled the old Kopete code. Probably it's even wrong sometimes (as soon as more than one theme is in a bundle).

I wouldn't like to change that now because the chatstylemanager needs some refactoring anyways.


> On 2010-09-29 20:58:15, David Edmundson wrote:
> > /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.cpp, line 32
> > <http://svn.reviewboard.kde.org/r/5483/diff/1/?file=38709#file38709line32>
> >
> >     Should you be checking the return value of this?

Actually yes. But atm it always returns 0. Should fix both though.


> On 2010-09-29 20:58:15, David Edmundson wrote:
> > /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.cpp, line 49
> > <http://svn.reviewboard.kde.org/r/5483/diff/1/?file=38709#file38709line49>
> >
> >     Don't use "magic numbers" always define an enum where possible.

Ack.


> On 2010-09-29 20:58:15, David Edmundson wrote:
> > /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.h, line 49
> > <http://svn.reviewboard.kde.org/r/5483/diff/1/?file=38708#file38708line49>
> >
> >     This does not conform to KDE coding libs standard (read about "d pointers").
> >     
> >     I'm trying to move all the chatwindow/lib code to follow this. Though it's ongoing at present.

Did not think about this, but sounds reasonable.


- Dominik


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5483/#review7889
-----------------------------------------------------------


On 2010-09-29 20:41:05, Dominik Schmidt wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5483/
> -----------------------------------------------------------
> 
> (Updated 2010-09-29 20:41:05)
> 
> 
> Review request for telepathy and Gustavo Boiko.
> 
> 
> Summary
> -------
> 
> The main.cpp is a little hacky.
> 
> Should I subclass KApplication and put the install function there?
> This would avoid the global app pointer and I could then add a slot displaySuccess() or something which would then show a knotification.
> Other suggestions how to solve this?
> 
> 
> If you want to test it, just go to http://www.adiumxtras.com and click an install link for chat styles or emoticons. It's broken in KDE (at least) trunk though :-( 
> You can test it simply this way:
> adiumxtra-protocol-handler adiumxtra://www.adiumxtras.com/download/2463 (for Gone Dark theme)
> adiumxtra-protocol-handler adiumxtra://www.adiumxtras.com/download/4058 (for cool_cat emoticon theme)
> 
> 
> Diffs
> -----
> 
>   /trunk/playground/network/telepathy-chat-handler/lib/chatwindowstylemanager.cpp 1180207 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/emoticonsetinstaller.h PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/emoticonsetinstaller.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/main.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.h 1180207 
>   /trunk/playground/network/telepathy-chat-handler/lib/chatstyleplistfilereader.cpp 1180207 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/chatstyleinstaller.cpp PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/chatstyleinstaller.h PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/CMakeLists.txt 1180207 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/CMakeLists.txt PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/adiumxtra-protocol-handler.notifyrc PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/adiumxtra.protocol PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/bundleinstaller.h PRE-CREATION 
>   /trunk/playground/network/telepathy-chat-handler/adiumxtra-protocol-handler/bundleinstaller.cpp PRE-CREATION 
> 
> Diff: http://svn.reviewboard.kde.org/r/5483/diff
> 
> 
> Testing
> -------
> 
> Yes, works for me :-)
> 
> 
> Screenshots
> -----------
> 
> Chatstyle Installation
>   http://svn.reviewboard.kde.org/r/5483/s/514/
> Emoticonset Installation
>   http://svn.reviewboard.kde.org/r/5483/s/515/
> 
> 
> Thanks,
> 
> Dominik
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20100929/2ff04096/attachment-0001.htm