Code review comment for lp://staging/~attente/unity/gnome-key-grabber

+bool PanelMenuView::SetMenuBarVisible(bool visible)
75 +{
76 + menu_bar_visible_ = visible;
77 + QueueDraw();
78 + return true;
79 +}
80 +

Only queuedraw and return true if value really changed (likely to happen, but nicer to see :)).

Also, in PanelMenuView you added a new var "menu_bar_visible_"... We already had show_now_activated_ for this purpose... I'm not sure if them might clash here, but what about just using one?

Also, to make the panel show the menu items correctly underlined, each PanelIndicatorEntry should have the show-now state...

251 + current_action_id_++;
Prefixed increment?

261 + CompAction& added(actions_.back());

Mnh, this is needed to access to the reference of it?
As I'd just use action instead... But maybe compiz definitions makes things harder.

262 + if (grabs_by_binding_[added.key()]++ == 0)
263 + screen_->addAction(&added);

A little hard to read... Probably using (with proper spacing):
auto& grab = grabs_by_binding_[added.key()];
if (grab == 0) { screen_->addAction(&added) }
++grab;

270 + std::map<const CompAction*, unsigned int>::const_iterator i(action_ids_by_action_.find(&action));

Auto is your friend here...
auto i = action_ids_by_action_.find(&action);
And everywhere we have iterators or complex or duplicated types (like A = new A()), just use it.

283 + if (--grabs_by_binding_[j->key()] == 0)
284 + screen_->removeAction(&*j);

As before... Also this &* thing is not the best to see...
248 +unsigned int GnomeKeyGrabber::Impl::addAction(const CompAction& action,
249 + bool addressable)

As for general style, we generally try to keep method names (with parameters) in just one line, unless it really becomes too long (where "too long" is not exactly specified) :P
Also we use Type const&, not const Type&...

315 + g_variant_builder_init(&builder, G_VARIANT_TYPE("au"));

This is, fine... But for your convenience you can use glib::Variant::FromVector for generating arrays such this...

action.setInitiate(boost::bind(&GnomeKeyGrabber::Impl::actionInitiated, this, _1, _2, _3));

std::bind? Or probably you can just use lambdas here, as they improve readability for small cases such these ones.

568 + std::map<const CompAction*, unsigned int> action_ids_by_action_;
569 + std::map<unsigned int, const CompAction*> actions_by_action_id_;

Mh, I'm not a fan of using non-smart pointers in stl containers, even though action_ vector is the owner... Could be possible to use something better than pure pointers here?

Can you also unit-tests for GnomeKeyGrabber (in the same way I did for GnomeSessionManager)?

Also I'd move the whole KeyGrabber thing on a separate library/folder... That links with unity-shared-compiz.

938 + priv->info_by_entry = g_hash_table_new_full (NULL, NULL, NULL, action_info_free);

720 + <option name="show_menu_bar" type="key">

Since this will clash with Hud reveal key, could you include in both descriptions that one is a "Tap", while the other is a key-press?

808 + PanelService *service;

As panelservice uses a singleto, I think you don't need to pass the service to every entry... Just use the global instance.

938 + priv->info_by_entry = g_hash_table_new_full (NULL, NULL, NULL, action_info_free);

action(_info)_by_entry would be a better name imho.

However, as for general design thing... Whouldn't be possible to handle the Activation, and registration of actions inside unity itself (instead of doing the work by dbus)? I know it's the standard way at this point, but maybe it would have kept the panel service simpler...