Vojtech Szocs has posted comments on this change. Change subject: webadmin: UI plugin event handler ......................................................................
Patch Set 1: (6 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() always returns some (even empty) list rather than null? 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: void addActionButtons(List<ActionButtonDefinition<?>> buttonDefs) { if (getTable() != null) { for (ActionButtonDefinition<?> b : buttonDefs) { getTable().addActionButton(b); } } } // inside deferred command's execute method List<ActionButtonDefinition<?>> pluginActionButtonList = ... addActionButtons(pluginActionButtonList); // inside registerHandler's onAddTabActionButton method if (getProxy().getTargetHistoryToken().equals(event.getHistoryToken())) { ActionButtonDefinition<?> button = event.getButtonDefinition(); addActionButtons(Arrays.asList(button)); } 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 creation will be triggered by creation of concrete AbstractTabPresenter instance. Don't we want to bind ActionButtonPluginHandler as eager singleton (created right upon application startup) instead, so that we'll never miss any "add action button" event? 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 with "plugin action button" name convention? Line 14: private final Map<String, List<ActionButtonDefinition<?>>> definitionMap = Line 15: new HashMap<String, List<ActionButtonDefinition<?>>>(); Line 16: Line 17: @Inject http://gerrit.ovirt.org/#/c/34570/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/DynamicTabContainerPresenter.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/DynamicTabContainerPresenter.java: Line 83 Line 84 Line 85 Line 86 Line 87 +1 on removing @ProxyEvent here. http://gerrit.ovirt.org/#/c/34570/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/MainContentPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/MainContentPresenter.java: Line 36 Line 37 Line 38 Line 39 Line 40 +1 on removing @ProxyEvent here. -- 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
