Code review comment for lp://qastaging/~unity-team/unity/lazy-places

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

With my less trained eyes in that area of code, it seems safe to me.

Just a few questions/remarks:
1.
210 + // In the worst case that the PlaceEntry wasn't connected and ready by the time we need to
211 + // show the menu
212 + if (_n_sections)
213 + {
214 + menu_item = dbusmenu_menuitem_new ();
215 + dbusmenu_menuitem_property_set (menu_item, DBUSMENU_MENUITEM_PROP_TYPE, DBUSMENU_CLIENT_TYPES_SEPARATOR);
216 + dbusmenu_menuitem_property_set_bool (menu_item, DBUSMENU_MENUITEM_PROP_ENABLED, true);
217 + dbusmenu_menuitem_property_set_bool (menu_item, DBUSMENU_MENUITEM_PROP_VISIBLE, true);
218 + _current_menu.push_back (menu_item);
219 + }

So basically, if it's the case (we are not connected), the first click will show nothing, but the second will show the place entries section, isn't it? Would be nice for Oneiric to add a FIXME so that we update the Quicklist as soon as it's connected (more generally, we need dynamic Quicklist to be more "dynamic" and react when even remote application are updating an opened Quicklist).

2.
427 + _home_button_hover = ubus_server_register_interest (ubus, UBUS_HOME_BUTTON_BFB_UPDATE,
428 + (UBusCallback)&PlacesView::ConnectPlaces,
429 + this);
This isn't the only way to show the launcher.
Ok, you Connect() the places in the other case by hovering (MouseEnter) the LauncherIcon which takes the case by revealing the launcher by pressing super or on edge activation. However, I think we can easily send an ubus signal in the launcher when Hidden turns to False (just need to check it's really False at the very beginning of the session and doesn't run to False to not emit the signal at session startup which will void your effort :-). Are you interested in this?

Other than that, unity is working fine here with your branch, will have a look again running it tomorrow as well :)

« Back to merge proposal