Review Request: Add Groups context menu

[David Edmundson]

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


Sorry, I've got a few comments, that I want either justifying or changing.




main-widget.cpp
<http://git.reviewboard.kde.org/r/101985/#comment4190>

    What is the point of m_contextMenu? 
    
    I /think/ it works, and you delete everything but it seems to be an over complex solution.
    
    surely could just do (in this method)
    
    KMenu *menu = contactContextMenu(index);
    if (menu) {
     menu->exec();
     menu->deleteLater(); 
    }
    
    //this will only be called when the menu closes (i.e exec finishes)
    
    and remove m_contextMenu completely.



main-widget.cpp
<http://git.reviewboard.kde.org/r/101985/#comment4191>

    This can't be right.
    
    From the Tp docs on Tp::PendingOperation::isValid (http://telepathy.freedesktop.org/doc/telepathy-qt4/a00222.html#a2b4e8575ad69c45cba0b2b070674c51f)
    
    "Equivalent to (isFinished() && !isError())."
    
    (I will agree that isValid is a stupid name for their method, but we can't change that)
    
    This will always return false.
    
    End result, we never monitor whether it was successful or not!
    
    Get rid of this "if"



main-widget.cpp
<http://git.reviewboard.kde.org/r/101985/#comment4192>

    See above isValid comment



main-widget.cpp
<http://git.reviewboard.kde.org/r/101985/#comment4193>

    See above isValid comment



main-widget.cpp
<http://git.reviewboard.kde.org/r/101985/#comment4194>

    typo
    Remve -> Remove


- David


On July 17, 2011, 8:34 p.m., Martin Klapetek wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101985/
> -----------------------------------------------------------
> 
> (Updated July 17, 2011, 8:34 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Summary
> -------
> 
> Adds a context menu for groups - currently contains only Rename and Delete actions. However, there's a problem with Delete as it does not really delete the group, only removes users from it. I need Collabora's help here as I don't know how to actually remove the group itself. Please help :)
> 
> 
> This addresses bug 274232.
>     http://bugs.kde.org/show_bug.cgi?id=274232
> 
> 
> Diffs
> -----
> 
>   main-widget.h 6fd3e8b 
>   main-widget.cpp 20995f6 
> 
> Diff: http://git.reviewboard.kde.org/r/101985/diff
> 
> 
> Testing
> -------
> 
> Renamed and removed group, both working as expected.
> 
> 
> Thanks,
> 
> Martin
> 
>

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