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

Hi Chris and Marco,

Thanks for the comments, I've resolved most of them. The MP is quite a bit smaller too, thanks for that!

> 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.

Yes, you're correct, we must pass a non-const pointer to compiz.

> 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...

Thanks, I'm leaving this as is since we need it to type properly if we get an empty array. Also, it might be more efficient than constructing an intermediate vector.

> 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?

So, while not as nice looking, I think I can justify it for a couple of reasons. We never end up de-referencing those pointers; their use is only for lookups of action ids. Also, I don't want the key grabber to keep those objects alive with a shared_ptr, and using weak_ptrs might have bad effects with the map when they get zeroed out.

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

I added a few autopilot tests for this.

> 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...

Thanks, moved it into the panel implementation instead :)