Review Request 109289: Adds exclusive actions group for contact list grouping

[David Edmundson]

> On March 5, 2013, 11:28 a.m., David Edmundson wrote:
> > contact-list-widget.cpp, line 332
> > <http://git.reviewboard.kde.org/r/109289/diff/2/?file=117041#file117041line332>
> >
> >     alternate way to fix the compile warning
> >     
> >     
> >     d->model->setSortRole(sort ? (int) KTp::ContactPresenceTypeRole : (int)Qt::DisplayRole);
> >     
> >     
> >     (not tested)
> >     
> >     They get casted to an int for setSortRole anyway
> 
> Roman Nazarenko wrote:
>     It's not the way to fix, it's the way to suppress them. Warning appears because mixing enums is a hell of a bad practice. 
>     But I don't really want to dig into it, so, it's up to you. Fix is dismissed.

look at the type being passed to setSortRole, it's an int.

Mixing enums is bad I agree. However that's not what we're doing. We are always implictly casting an enum to an int (which is perfectly valid, it's the other way that isn't) for setSortRole. 
In this case we are trying to cast two different enums to an int, and that's what it's complaining about.

Qt roles are integers. We just use enums as a convenient way to namespace auto-incrementing ints.


> On March 5, 2013, 11:28 a.m., David Edmundson wrote:
> > contact-list-widget.cpp, line 304
> > <http://git.reviewboard.kde.org/r/109289/diff/2/?file=117041#file117041line304>
> >
> >     Why don't we pass KTp::ContactsModel::GroupMode as the arg type. It's safer, and we don't need to cast.
> 
> Roman Nazarenko wrote:
>     I assume, Qt signal-slot mechanism will shit the hell out of itself when we'll try namespaced enumerations in slots' signatures.
>     Have not tested it though.

It does not complain. (also avoid swear words on anything that hits the ML please)


- David


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


On March 5, 2013, 3:37 p.m., Roman Nazarenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109289/
> -----------------------------------------------------------
> 
> (Updated March 5, 2013, 3:37 p.m.)
> 
> 
> Review request for Telepathy.
> 
> 
> Description
> -------
> 
> Adds exclusive actions group for contact list grouping.
> Added enumerated action groups, so we could easily convert checked grouped action into appearance modes. 
> Those groups also allow us to replace onFullView/onNormalView slots with one onAppearanceChanged(int), so we can reduce number of manual signal-slot connections (and improve readability and maintainability). 
> 
> Also moved (config file option name <-> widget text <-> mode enumerator) relations for exclusive action groups into separate static class MenuConfig, so we can avoid manual relations tracking.
> 
> 
> This addresses bug 279023.
>     http://bugs.kde.org/show_bug.cgi?id=279023
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 5802d32 
>   action-group-enumerated.h PRE-CREATION 
>   action-group-enumerated.cpp PRE-CREATION 
>   contact-list-widget.h ab2191c 
>   contact-list-widget.cpp f931913 
>   main-widget.h d72c970 
>   main-widget.cpp 778c71e 
> 
> Diff: http://git.reviewboard.kde.org/r/109289/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Roman Nazarenko
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-telepathy/attachments/20130305/f2f50081/attachment.html>