Review Request: Enable call holding in ktp-call-ui

[George Kiagiadakis]

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



src/call-window.h
<http://git.reviewboard.kde.org/r/105829/#comment13185>

    No, the hold button should not be enabled/disabled externally. It's part of the UI's state and this is exactly why the setStatus() method exists. So, all changes in the hold button state should be done inside setStatus().



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13187>

    Although I wrote this, I believe that KToggleAction is not the right class for this thing. The main problem is that it remembers its state, so if you try to unhold the call and it fails and you try again, the second time it will try to hold... Also, what happens if you accidentally double-click it (i.e. hold and unhold before hold has finished)? The CM will be confused.
    
    I believe this should be a simple KAction and in the slot decide whether to hold or not based on the current hold status. Also, it would be a good idea to disable this action while the hold state is changing (PendingHold, PendingUnhold states).



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13177>

    Bad name. What operation finished?



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13178>

    i18n()



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13188>

    Bad debug string. We are not updating the hold status, the CM does that and informs us. "Hold status changed" would be far better.



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13184>

    1) StatusArea does not implement the Error message type, so these are essentially no-op. I thought this was why you wanted the KMessageWidget...
    
    2) The messages are not very clear. You need to tell the user that the call is on hold, somehow. Especially in the case of ResourceNotAvailable, the string should be something like "Failed to put the call off hold: Some devices are not available", because this can only happen if you are trying to put the call off hold and the backend fails to open some input/output device (camera, sound card).



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13180>

    This cannot happen in StateUnheld. It should also be handled as unknown error.



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13182>

    I am not a native english speaker, but I believe "hold" is not normally used as a verb but as part of the expression "put <foo> on hold". Also, I don't like "trying" here. Something like "Putting the call on hold..." would be better imho.
    
    Native english speakers, what's your opinion?



src/call-window.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13183>

    As above, "Resuming the call..." would be better imho.



src/call-window.ui
<http://git.reviewboard.kde.org/r/105829/#comment13186>

    Afaict, the errorWidget here is on the same page as the user avatar label, which means that if we are on a video call, this widget will not appear...



src/main.cpp
<http://git.reviewboard.kde.org/r/105829/#comment13181>

    This will cause a failure if the hold interface is not available in the call, but I think both gabble and rakia implement it so I think we can get away with it for now


- George Kiagiadakis


On Aug. 3, 2012, 9:56 p.m., Rohan Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105829/
> -----------------------------------------------------------
> 
> (Updated Aug. 3, 2012, 9:56 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Enables call holding in ktp-call-ui
> 
> 
> Diffs
> -----
> 
>   src/main.cpp 4ab3a23 
>   src/call-window.ui 14144cc 
>   src/call-window.h d8741c3 
>   src/call-window.cpp e4aed4d 
>   src/call-manager.cpp df6e701 
> 
> Diff: http://git.reviewboard.kde.org/r/105829/diff/
> 
> 
> Testing
> -------
> 
> Successfully held and unheld calls using ktp-call-ui
> 
> 
> Thanks,
> 
> Rohan Garg
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20120804/951d9186/attachment-0001.html>