Alexander Wels has posted comments on this change. Change subject: webadmin: UI plugin event handler ......................................................................
Patch Set 1: (4 comments) http://gerrit.ovirt.org/#/c/34570/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractTabPresenter.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/AbstractTabPresenter.java: Line 39: Line 40: @Override Line 41: public void execute() { Line 42: List<ActionButtonDefinition<?>> pluginActionButtonList = actionButtonPluginHandler.getButtons(getProxy().getTargetHistoryToken()); Line 43: if (getTable() != null && pluginActionButtonList != null) { > Isn't it better to ensure that actionButtonPluginHandler.getButtons() alway Done Line 44: for(ActionButtonDefinition<?> buttonDef: pluginActionButtonList) { Line 45: getTable().addActionButton((ActionButtonDefinition) buttonDef); Line 46: } Line 47: } Line 54: Line 55: @Override Line 56: public void onAddTabActionButton(AddTabActionButtonEvent event) { Line 57: if (getProxy().getTargetHistoryToken().equals(event.getHistoryToken())) { Line 58: if (getTable() != null) { > You could reduce some duplicity in code: Done Line 59: getTable().addActionButton((ActionButtonDefinition) event.getButtonDefinition()); Line 60: } Line 61: } Line 62: } Line 68: */ Line 69: protected abstract ActionTable<?> getTable(); Line 70: Line 71: @Inject Line 72: public void setActionButtonPluginHandler(ActionButtonPluginHandler actionButtonPluginHandler) { > Because ActionButtonPluginHandler is lazy (not eager) singleton, its creati We have wayyyyy to many eager singletons as it is (I really want to reduce the number, but that is another patch). This particular event is never fired until the UI plugins are loading, which is way after the first tab presenter is instantiated, so we should be good. Line 73: this.actionButtonPluginHandler = actionButtonPluginHandler; Line 74: } http://gerrit.ovirt.org/#/c/34570/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/ActionButtonPluginHandler.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/ActionButtonPluginHandler.java: Line 9: Line 10: import com.google.gwt.event.shared.EventBus; Line 11: import com.google.inject.Inject; Line 12: Line 13: public class ActionButtonPluginHandler { > Can we please name this class PluginActionButtonHandler to stay consistent Sure, no problem. Line 14: private final Map<String, List<ActionButtonDefinition<?>>> definitionMap = Line 15: new HashMap<String, List<ActionButtonDefinition<?>>>(); Line 16: Line 17: @Inject -- To view, visit http://gerrit.ovirt.org/34570 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I83ad213a1dca8d31cf63c6b2c5f761f3242b43bc Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alexander Wels <[email protected]> Gerrit-Reviewer: Alexander Wels <[email protected]> Gerrit-Reviewer: Einav Cohen <[email protected]> Gerrit-Reviewer: Vojtech Szocs <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
