Code review comment for lp:~osomon/unity-2d/one-panel-per-screen

Revision history for this message
Aurélien Gâteau (agateau) wrote :

No time to test tonight, but the way you implemented it looks good. Here are a few remarks to keep things moving:

- Please rename PanelsManager to PanelManager

- Can you make the helper functions in panelmanagers static? this makes it more explicit they are not meant to be used from outside (they were not when they were in main.cpp because I assumed it was clear no code from main.cpp would ever be called from somewhere else)

- Adjust the registrar singleton method to something like this:

Registrar* Registrar::instance()
{
    static Registrar singleton;
    return &singleton;
}

It's almost the same, except memory checkers such as valgrind won't complain about not released memory.

- You don't need to mark Registrar constructor explicit. This is only useful for one-parameter constructors.

I may add a few other requests tomorrow morning, but I have to go now.

review: Needs Fixing

« Back to merge proposal