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

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

10 + WindowManager& wm = WindowManager::Default();
11 + // delete all menu items for windows

29 + char menu_item_name[name_size];
30 + sprintf(menu_item_name,"w_list_%d",i);

44 + _menu_items[menu_item_name] = menu_item;

Please, don't add these items to _menu_items. Use instead another container (such as
std::vector<glib::Object<DbusmenuMenuitem>> _windows_menu_items and add the items you generate there in AddMenuItemsWindowList.

22 + for ( auto w: Windows() ) {

Use for (auto const& w : Windows())

At that point call it into GetMenus() putting the items you computed into the result list.

36 + _gsignals.Add(
37 + new glib::Signal<void, DbusmenuMenuitem*, int>(menu_item,

You can use the utility Add function to save some space:
_gsignals.Add<void, DbusmenuMenuitem*, int>(menu_item, DBUSMENU_MENUITEM_SIGNAL_ITEM_ACTIVATED ...

39 + [w,&wm] (DbusmenuMenuitem*,int) -> void {

Just pass w, while generate the WM inside the lambda body (even if that pointer won't change, it's safer not to pass by reference inside a lambda that is going to be executed when the local variable is dead). Also it's not needed to specify the "-> void" return type. g++ will guess it for you.

45 + std::string winName( ( wm.GetWindowName( w->window_id() ) ) );
46 + dbusmenu_menuitem_property_set(menu_item,
47 + DBUSMENU_MENUITEM_PROP_LABEL, winName.c_str() );

Only use:
dbusmenu_menuitem_property_set(menu_item, DBUSMENU_MENUITEM_PROP_LABEL, w->title().c_str());

51 + if (windows_added) {
52 + // add seperator

65 + }

Do this into GetMenus.

Also, long quicklist items should be cut. I'll provide I way to do it as I need it also on another branch, please ping me on IRC in case you'd need help.

Fix the style to match the one we use in unity (2 spaces indentation, brackets on next line)

Need tests.

review: Needs Fixing

« Back to merge proposal