Code review comment for lp:~compiz-team/compiz/compiz.fix_1053280

Revision history for this message
Daniel van Vugt (vanvugt) wrote :

No obvious problems in testing. Just a few things:

1. These should be dropped now:
    Type type (); // BCI: drop this in 0.9.9
If you need to constify functions as part of this proposal then no problem. Otherwise constification and other general clean-ups should be done as separate proposals.

2. BCI, PV? Are those acronyms explained in the source files they're mentioned?

3. I would much prefer that the namespace depth did not affect the indentation of declarations. I would accept:
    namespace a { namespace b {
or
    namespace a {
    namespace b {
Because namespaces should be changeable without affecting the surrounding code. So if the depth changed then you shouldn't ever need to re-indent your code.

4. Two functions:
    PrivateAction::makeActive()
    PrivateAction::makeInactive()
should be one:
    PrivateAction::setActive(bool active)
Which will not only shrink the interface but is much more flexible for callers who want to change the setting based on a bool, like ca::setActionActiveState.

5. End-users of identifiers in namespaces like src/screen.cpp should use:
    using namespace compiz::actions;
    foo
    bar
instead of:
    namespace ca = compiz::actions;
    ca::foo
    ca::bar
It's cleaner to read, and again more flexible should namespaces ever change.

review: Needs Fixing

« Back to merge proposal