Tomas Jelinek has posted comments on this change. Change subject: webadmin: Introduce CPU profiles ......................................................................
Patch Set 2: (7 comments) http://gerrit.ovirt.org/#/c/31627/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/CpuProfileListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/CpuProfileListModel.java: Line 38: } Line 39: Line 40: @Override Line 41: protected VDSGroup getParentEntity() { Line 42: return (VDSGroup) ((super.getEntity() instanceof VDSGroup) ? super.getEntity() : null); Is there a chance that the super.getEntity() is not a VDSGroup? Because: - if not I would remove the condition here because it is not needed - if yes than you need to handle this also in ProfileListModel.fetchProfiles() which expects it to be non null and calls getId() on the result directly Line 43: } Line 44: Line 45: @Override Line 46: protected Guid getStoragePoolId() { http://gerrit.ovirt.org/#/c/31627/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/ProfileBaseModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/ProfileBaseModel.java: Line 137: VdcReturnValueBase returnValue = result.getReturnValue(); Line 138: stopProgress(); Line 139: Line 140: if (returnValue != null && returnValue.getSucceeded()) { Line 141: cancel(); I'd say you should call cancel() also when it did not succeeded otherwise the window will be stuck there. Or is there a specific reason to leave the window opened in case of failure? Line 142: } Line 143: } Line 144: }, Line 145: this); Line 188: protected abstract void postInitQosList(List<Q> list); Line 189: Line 190: public boolean validate() { Line 191: getName().validateEntity(new IValidation[] { new NotEmptyValidation(), new SpecialAsciiI18NOrNoneValidation() }); Line 192: the description should have something like: getDescription().validateEntity(new IValidation[] { new AsciiOrNoneValidation() }); Line 193: return getName().getIsValid(); Line 194: } Line 195: http://gerrit.ovirt.org/#/c/31627/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/uicommon/ClusterModule.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/gin/uicommon/ClusterModule.java: Line 300: @Singleton Line 301: public SearchableDetailModelProvider<CpuProfile, ClusterListModel, CpuProfileListModel> getStorageCpuProfileListProvider(EventBus eventBus, Line 302: Provider<DefaultConfirmationPopupPresenterWidget> defaultConfirmPopupProvider, Line 303: final Provider<CpuProfilePopupPresenterWidget> newProfilePopupProvider, Line 304: final Provider<CpuProfilePopupPresenterWidget> editProfilePopupProvider, it is enough to have only one provider of type CpuProfilePopupPresenterWidget injected Line 305: final Provider<RemoveConfirmationPopupPresenterWidget> removeConfirmPopupProvider) { Line 306: return new SearchableDetailTabModelProvider<CpuProfile, ClusterListModel, CpuProfileListModel>(eventBus, Line 307: defaultConfirmPopupProvider, Line 308: ClusterListModel.class, Line 310: @Override Line 311: public AbstractModelBoundPopupPresenterWidget<? extends Model, ?> getModelPopup(CpuProfileListModel source, Line 312: UICommand lastExecutedCommand, Line 313: Model windowModel) { Line 314: if (lastExecutedCommand == getModel().getNewCommand()) { if you will have only one CpuProfilePopupPresenterWidget injected you can join this two branches to one if(lasteExecutedCommand == new... or edit...) Line 315: return newProfilePopupProvider.get(); Line 316: } else if (lastExecutedCommand == getModel().getEditCommand()) { Line 317: return editProfilePopupProvider.get(); Line 318: } else { http://gerrit.ovirt.org/#/c/31627/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/CpuProfilePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/CpuProfilePopupView.java: Line 59: public CpuProfilePopupView(EventBus eventBus, ApplicationResources resources, ApplicationConstants constants) { Line 60: super(eventBus, resources); Line 61: clusterEditor = new ListModelListBoxEditor<VDSGroup>(new NullSafeRenderer<VDSGroup>() { Line 62: @Override Line 63: public String renderNullSafe(VDSGroup VDSGroup) { s/VDSGroup/vdsGroup for variable name Line 64: return VDSGroup.getName(); Line 65: } Line 66: }); Line 67: qosEditor = new ListModelListBoxEditor<CpuQos>(new NullSafeRenderer<CpuQos>() { http://gerrit.ovirt.org/#/c/31627/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/CpuProfilePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/CpuProfilePopupView.ui.xml: Line 5: xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog" Line 6: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor" Line 7: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic" > Line 8: Line 9: <ui:with field='constants' type='org.ovirt.engine.ui.webadmin.ApplicationConstants' /> it seems this is not used in this file so can be removed Line 10: Line 11: <d:SimpleDialogPanel width="470px" height="220px"> Line 12: <d:content> Line 13: <g:FlowPanel> -- To view, visit http://gerrit.ovirt.org/31627 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c51fa8559f9538bb29b41d1d1b24fdfdabc8cb1 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
