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

Revision history for this message
Marco Trevisan (TreviƱo) (3v1n0) wrote :

110 + auto accelerator(gtk_accelerator_name(gdk_keyval_to_lower(gdk_unicode_to_keyval(mnemonic)), GDK_MOD1_MASK));

use glib::String here.

111 + auto action(std::make_shared<CompAction>());

Why not just auto action = std::make_shared... ? g++ would just use the best way to initialize it. Same in other places...

115 + action->setInitiate([=](CompAction* action, CompAction::State state, CompOption::Vector& options)
116 + {

Just pass [this, id] to the lambda, where id is the entry->id() copied to a string.

1061 + CompScreen* screen_;
FYI CompScreen is exported by <core/screen.h> as a global "screen" variable and is always available since the plugin initialization, so you can just use that reference if you want.

884 + action.setInitiate([=]
892 + action.setTerminate([=]

It doesn't seem you need to pass anything a part from [this] to these lambdas, so just avoid to copy everything please.

1066 + std::map<CompAction const*, unsigned int> action_ids_by_action_;
1067 + std::map<unsigned int, CompAction const*> actions_by_action_id_;

As you're never iterating over them, using std::unordered_map here might give us some benefit.

« Back to merge proposal