Review Request: Initial implementation of telepathy-approver

[Martin Klapetek]

> On 2010-12-10 02:58:08, Daniele Elmo Domenichelli wrote:
> > src/textchannelapprover.cpp, lines 35-40
> > <http://git.reviewboard.kde.org/r/100199/diff/1/?file=4967#file4967line35>
> >
> >     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?
> 
> George Kiagiadakis wrote:
>     I show a popup for each message, yes. This is what empathy is doing and so far I like it. I think kopete also does that. Note that popups normally group together and autohide after a while, so I think it is fine from a usability pov. And clicking respond on any of them will make them all disappear.
>     
>     Starting a chat in composing state should not happen imho. If the CM signals it, I would consider this a CM bug. It is just wrong to show anything to the user if there is still a chance that this channel will not receive any messages.

If I may interject here, showing just one popup and appending the messages there is IMHO better. Think about it in this way - you get four or five messages in rather rapid succession, so only one will be shown and the rest will stay hidden. Now if the last one would be shown, you would see the fourth (fifth) message without seeing the previous ones, which is not right. It works better for displaying the first message and hiding the rest, but then why even show the rest when they will autohide all together (because of the rapid succession, they'll all have almost the same timeout). Therefore, it's much better to display just one popup and append the messages there.


- Martin


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


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/496b2ee5/attachment.htm