Review Request: Add Groups context menu

[Martin Klapetek]

> On July 18, 2011, 12:02 p.m., David Edmundson wrote:
> > main-widget.cpp, line 740
> > <http://git.reviewboard.kde.org/r/101985/diff/1/?file=27305#file27305line740>
> >
> >     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.

The initial point was to prevent mem leaks by creating new menus and never delete them. This suggestion of yours seems like it could work too. Will update.


> On July 18, 2011, 12:02 p.m., David Edmundson wrote:
> > main-widget.cpp, line 866
> > <http://git.reviewboard.kde.org/r/101985/diff/1/?file=27305#file27305line866>
> >
> >     Telepathy /does/ support blocking (except on MSN).

This part was done by Keith Rusler some time ago, he did some research and it turned out, that it does not. Maybe it has been fixed/implemented. I will check with oggis.


> On July 18, 2011, 12:02 p.m., David Edmundson wrote:
> > main-widget.cpp, line 949
> > <http://git.reviewboard.kde.org/r/101985/diff/1/?file=27305#file27305line949>
> >
> >     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"

Meh..and it looked so great to have that check in place.. :D


> On July 18, 2011, 12:02 p.m., David Edmundson wrote:
> > main-widget.cpp, line 994
> > <http://git.reviewboard.kde.org/r/101985/diff/1/?file=27305#file27305line994>
> >
> >     typo
> >     Remve -> Remove

How come I always manage to do typos in the display strings? Btw. the rest of the sentences is ok (grammar etc)?


- Martin


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


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/1405a6fc/attachment-0001.htm