Review Request: Initial implementation of telepathy-approver

[George Kiagiadakis]

> On 2010-12-10 02:58:08, Daniele Elmo Domenichelli wrote:
> > src/approverdaemon.h, line 23
> > <http://git.reviewboard.kde.org/r/100199/diff/1/?file=4956#file4956line23>
> >
> >     Why QObject? If there is a reason maybe the constructor should be ApproverDaemon(QObject* parent) and you should add the Q_OBJECT macro

I use the QObject parent-child relationship to make sure that everything is cleaned up properly in case someone unloads the module while a channel request is still pending. No more features from QObject are used, thus I consider the Q_OBJECT macro or anything else useless.


> On 2010-12-10 02:58:08, Daniele Elmo Domenichelli wrote:
> > src/telepathy_kde_approver.notifyrc, line 2
> > <http://git.reviewboard.kde.org/r/100199/diff/1/?file=4965#file4965line2>
> >
> >     Kopete? :P

Just a random icon name... It just seems more close to what we are doing here, but as it is not even in the oxygen theme, I think that we should adopt a new icon at some point and use it everywhere.


> 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?

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.


> 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...

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.


> 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 :)

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.


On 2010-12-10 02:58:08, George Kiagiadakis wrote:
> > Everything else looks fine, although I agree with George that the KDED module is not the best solution...

Regarding the KDED module, as I told George on irc, the module is not autoloaded. Imho the presence applet should load this module when the user goes online and unload it when the user goes offline, so that it does not interfere with other telepathy clients, like empathy, if the user chooses to use another one instead (and not have our presence plasmoid on the desktop). So, being a KDED module in this case is only good imho. It provides easy loading/unloading through the kded dbus interface and consumes less resources by being a plugin to an already running application.


- George


-----------------------------------------------------------
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/39616eb2/attachment-0001.htm