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

[Roman Nazarenko]

> On March 6, 2013, 12:22 p.m., Martin Klapetek wrote:
> > contact-list-widget.h, lines 53-55
> > <http://git.reviewboard.kde.org/r/109289/diff/5/?file=117206#file117206line53>
> >
> >     This should take the respective enums, plus put the variable name here

Neh. Implicit casting from enums is prohibited. We can't connect enumerated group's signal (which type is int, because group is enum-independent) with strongly-typed enumerational slot, building will stop with the compilation error.


> On March 6, 2013, 12:22 p.m., Martin Klapetek wrote:
> > contact-list-widget.cpp, line 101
> > <http://git.reviewboard.kde.org/r/109289/diff/5/?file=117207#file117207line101>
> >
> >     This shouldn't be an int but the setting enum (which is an int)

We can't store enumerations in KConfig, documentation prohibit such behaviour explicitly. So, assuming you're right, we will need to:
1) to static_cast of defaultAppearance 
2) static_cast from readEntry's return value
3) static_cast of this value again, before it's passed to createGroup (since one is an enumerational-polymorphic, it's impossible to use enums there)

So, on one scale we have 3 castings (something like static_cast<KTp::ContactsFilterModel::SubscriptionStateFilterFlag>(guiConfigGroup.readEntry(...)). 3 casts per 3 groups means 9 casts at 12 lines of code. Looks bad.
And on another, we have integers using, which is not so bad since all your internal logic casts enums to the integers anyway; even Qt itself - remember that warning of setSortRole). 


> On March 6, 2013, 12:22 p.m., Martin Klapetek wrote:
> > contact-list-widget.cpp, line 491
> > <http://git.reviewboard.kde.org/r/109289/diff/5/?file=117207#file117207line491>
> >
> >     I don't understand this comment. What checks?

I meant, we cast to enumeration with no boundaries checking. But I purged toggleGroups from the class (we have onGroupingChanged slot instead now), so it makes no sense, removed it from code.


> On March 6, 2013, 12:22 p.m., Martin Klapetek wrote:
> > main-widget.cpp, line 529
> > <http://git.reviewboard.kde.org/r/109289/diff/5/?file=117209#file117209line529>
> >
> >     KAction* action -> KAction *action

Fixed


- Roman


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


On March 6, 2013, 7:31 a.m., Roman Nazarenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109289/
> -----------------------------------------------------------
> 
> (Updated March 6, 2013, 7:31 a.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/20130306/c1280d01/attachment.html>