Code review comment for lp://qastaging/~3v1n0/unity/lim

> 44 +const std::string SETTING_NAME("com.canonical.indicator.appmenu");
> 45 +const std::string SET
> TING_KEY("menu-mode");
>
> Maybe they shuld be in a unnamed namespace?

Right. I thought I had used.

> 48 + : Indicator(name),
> 49 + gsettings_(g_settings_new(SETTING_NAME.c_str())),
> 50 + integrated_(false)
>
> For the next time... :
>
> 48 + : Indicator(name)
> 49 + , gsettings_(g_settings_new(SETTING_NAME.c_str()))
> 50 + , integrated_(false)

Well, I don't like this syntax with prefixed commas... But if you all prefer that...

> setting_changed_.Connect(gsettings_, "changed::menu-mode", [&] (GSettings*,
> gchar* key) {
> 53 + CheckSettingValue();
> 54 + });
>
> If you're not using the argument gchar* key, please use an unnamed variable
> too...

Right, I forgot to remove it (at the beginning I used the key).

> g_idle_add_full (G_PRIORITY_DEFAULT, (GSourceFunc) send_show_entry, data,
> NULL);
> g_variant_get (parameters, "(s(iiuu))", &entry_name, &geo.x, &geo.y,
> &geo.width, &geo.height);
>
> Remove the whitespace between the function name and the bracket.

Damned you! :)

> AppmenuIndicator *appmenu = dynamic_cast<AppmenuIndicator*>(indicator.get());
>
> :(

We don't have any way. But it's totally safe: we do two checks, first IsAppmenu(), then this one.

> 751 - * Copyright (C) 2010 Canonical Ltd
> 752 + * Copyright (C) 2012 Canonical Ltd
>
> Please do 2010-2012

Sure.

> ---
>
> 760 +#include <NuxCore/Rect.h>
>
> I think you can use a forward declaration.

I thought too... But it didn't work when I firstly compiled. Now I removed it again and it works... Magic!

> ---
>
> + bool IsVisible();
>
> Const?
>
> ---+ bool IsVisible();

That is done in lp:~3v1n0/unity/lim-panel... Here just focus on the UnityCore unity-panel-service changes.

> Btw all of these are not blocking ;)

If they are not blocking, please don't set your review as needs fixing! :)