Code review comment for lp:~lukas-vacek/unity/bamficon_windowlist-raring

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

36 + // delete all menu items for windows
37 + _menu_items_windows.clear();

Please do this on EnsureMenuItemsWindowsReady

17 + _gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED,
18 + [w] (DbusmenuMenuitem*,int) {

This is ok, but since using [w] means copying the the WindowPtr, I'd prefer if you instead do:

Window xid = w->window_id();
_gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED, [xid] (DbusmenuMenuitem*,int) {
...
wm.Raise(xid);
}

22 + } );

Remove this padding, please (use also for the lambda two spaces for indenting).

24 + dbusmenu_menuitem_property_set(menu_item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, "300");

That property is an integer, you should do instead:

dbusmenu_menuitem_property_set_int(menu_item, QuicklistMenuItem::MAXIMUM_LABEL_WIDTH_PROPERTY, MAXIMUM_QUICKLIST_WIDTH);

Where MAXIMUM_QUICKLIST_WIDTH is a const int that you should define in the unnamed namespace that we have at the top of the ApplicationLauncherIcon.cpp file ;).

40 + // add windows menu items
41 + if (_menu_items_windows.size() > 1 )

Mh, I'd prefer to iterate one more time than allocating some extra memory here, so just use if (Windows().size() > 1), and move EnsureMenuItemsWindowsReady under this check.

Thanks ;)

« Back to merge proposal