Review Request: Initial implementation of telepathy-approver

[Daniele Elmo Domenichelli]

> On 2010-12-10 02:58:08, Daniele Elmo Domenichelli wrote:
> > src/textchannelapprover.cpp, lines 118-120
> > <http://git.reviewboard.kde.org/r/100199/diff/1/?file=4967#file4967line118>
> >
> >     Is this decreased somewhere? If this is static maybe it should count the number of channels to approve, not the number of messages...
> 
> George Kiagiadakis wrote:
>     Hum, good catch, I am not decreasing this iirc. I will fix it.
>     
>     Why count the number of channels? What should it say then? "You have 2 new channels to approve"? I think the user should not know anything about channels.

I'd say "You have <n> incoming conversations". Then, if you want to show also the number of messages, you can add n lines containing something like "<contact name>: <x> messages"
That's because I see no reason in just a sum of the messages from a random number of conversations in the tooltip. You should show the number of conversations and/or the number of message for each of them.


> On 2010-12-10 02:58:08, Daniele Elmo Domenichelli wrote:
> > src/textchannelapprover.cpp, lines 79-80
> > <http://git.reviewboard.kde.org/r/100199/diff/1/?file=4967#file4967line79>
> >
> >     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 :)
> 
> George Kiagiadakis wrote:
>     I tried all signals, none of them worked.
>     
>     About the ignore button... I left it out for the moment because this is a strange case that I am not sure how to handle. If I simply close the channel, the CM will signal it again, with the same messages (according to the spec). To avoid that I have two choices, either confirm all messages and then close the channel or use the Destroyable interface to destroy it. Even if I do that though, if the remote user continues typing, I will receive a new channel. So, there is effectively no way to ignore the chat, if the other user continues sending messages, they will popup. The question is purely a usability one: what would a user expect from the ignore button? I guess I can confirm all the messages and close the chat, just as a shortcut to avoid opening the handler and then close it, but I'm not sure if that's what the user expects.

TpQt4 documentation says: "Approvers wishing to reject channels should call the ChannelDispatchOperation::claim() method, then (if it succeeds) close the channels in any way they see fit."
But then if you just call requestClose() you may be able to drop the messages in the queue at that point, but if you receive another message from the same user, you probably get a new channel to approve.
You might implement the AbstractClientHandler interface to drop all the messages but that's not probably what a user expect.

I'd expect the same behaviour that I would like to see when my status is "Busy":
- No further notification/popup
- No chat opening (so the channel is not supposed to be approved)
- A way to open the chat later with my favourite chat-handler and read all the history

So, an idea that just came to my mind... The text channel should just be accepted, but there should be a method to set the chat handler in passive mode: just accept the message (message delivery status should be accepted but not read), no notification, no chat ui, just a status icon.

(So probably a "block user" button would be more useful than an "ignore" button)


- Daniele Elmo


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