Review Request: Initial implementation of telepathy-approver

[Daniele Elmo Domenichelli]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/100199/#review477
-----------------------------------------------------------

Ship it!


Just a couple of comments, even if you already committed it...


src/approverdaemon.h
<http://git.reviewboard.kde.org/r/100199/#comment351>

    Why QObject? If there is a reason maybe the constructor should be ApproverDaemon(QObject* parent) and you should add the Q_OBJECT macro



src/telepathy_kde_approver.notifyrc
<http://git.reviewboard.kde.org/r/100199/#comment353>

    Kopete? :P



src/textchannelapprover.cpp
<http://git.reviewboard.kde.org/r/100199/#comment354>

    I might be wrong here...
    If I understand correctly, you show a popup for each message in the channel message queue. Then you show more popups when a new message is received.
    
    Isn't the chat handler supposed to show just ONE notification when someone starts a chat with you (maybe just showing the first message or appending the text of the following messages)? I would hate having several notification asking me to accept the same chat.
    
    Also what happens if the channel is started in status ChannelChatStateComposing? Actually I'm not sure whether this can happen and how we are supposed to handle it. Should we wait for the first message to show the accept channel popup? But then what happens if the other side stops writing and doesn't send any message?



src/textchannelapprover.cpp
<http://git.reviewboard.kde.org/r/100199/#comment356>

    Maybe the respond button doesn't work because the Respond button is for some reason not considered the default action by knotify. Maybe action1Activated() or activated(unsigned int action) will work...
    
    Also an "Ignore" button should be added here :)



src/textchannelapprover.cpp
<http://git.reviewboard.kde.org/r/100199/#comment355>

    Is this decreased somewhere? If this is static maybe it should count the number of channels to approve, not the number of messages...


Everything else looks fine, although I agree with George that the KDED module is not the best solution...

- Daniele Elmo


On 2010-12-09 20:27:51, George Kiagiadakis wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/100199/
> -----------------------------------------------------------
> 
> (Updated 2010-12-09 20:27:51)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> This is the initial implementation of the approver. It can approve p2p text chats only, for now. It is build as a kded module. When a new message arrives, it shows a knotify popup, it plays a sound and shows a KStatusNotifierItem flashing in the tray.
> 
> gitweb url:
> http://gitweb.kde.org/clones/telepathy-approver/gkiagia/telepathy-approver.git/shortlog/refs/heads/implementation
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt PRE-CREATION 
>   src/CMakeLists.txt PRE-CREATION 
>   src/approverdaemon.h PRE-CREATION 
>   src/approverdaemon.cpp PRE-CREATION 
>   src/channelapprover.h PRE-CREATION 
>   src/channelapprover.cpp PRE-CREATION 
>   src/dispatchoperation.h PRE-CREATION 
>   src/dispatchoperation.cpp PRE-CREATION 
>   src/handlewithcaller.h PRE-CREATION 
>   src/handlewithcaller.cpp PRE-CREATION 
>   src/telepathy_kde_approver.desktop PRE-CREATION 
>   src/telepathy_kde_approver.notifyrc PRE-CREATION 
>   src/textchannelapprover.h PRE-CREATION 
>   src/textchannelapprover.cpp PRE-CREATION 
>   src/tpkdeapprovermodule.cpp PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/100199/diff
> 
> 
> Testing
> -------
> 
> Tested with empathy as the chat handler. Currently the "respond" button on the popup doesn't seem to work, but I blame knotify's crappiness for that. Clicking on the flashing icon works perfectly, though.
> 
> 
> Thanks,
> 
> George
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-telepathy/attachments/20101210/a668e109/attachment.htm