Daniel Erez has posted comments on this change. Change subject: webadmin: introduce disk profiles ......................................................................
Patch Set 5: (8 comments) http://gerrit.ovirt.org/#/c/29672/5//COMMIT_MSG Commit Message: Line 4: Commit: Gilad Chaplik <gchap...@redhat.com> Line 5: CommitDate: 2014-07-23 21:24:00 +0300 Line 6: Line 7: webadmin: introduce disk profiles Line 8: > I will add a link to feature page. anything specific do you want me to add * Where did u add the feature page? Don't see it in the new patch-set... * Yes, please refer to the relevant section in the feature page for every item here. Line 9: * disk profiles subtab under storage main tab. Line 10: * create/update/remove dialog disk profile. Line 11: * backend integration in GUI. Line 12: http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/DiskProfileBaseModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/DiskProfileBaseModel.java: Line 28: import org.ovirt.engine.ui.uicompat.ConstantsManager; Line 29: import org.ovirt.engine.ui.uicompat.FrontendActionAsyncResult; Line 30: import org.ovirt.engine.ui.uicompat.IFrontendActionAsyncCallback; Line 31: Line 32: public abstract class DiskProfileBaseModel extends Model { > I've build current infra for profiles, that it will later we fitted to othe Is it to difficult to be done as part of this patch? Line 33: private final static StorageQos EMPTY_QOS; Line 34: Line 35: static { Line 36: EMPTY_QOS = new StorageQos(); Line 192: public void onSuccess(Object model, Object returnValue) { Line 193: List<StorageQos> qosList = Line 194: returnValue == null ? new ArrayList<StorageQos>() Line 195: : (List<StorageQos>) ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 196: qosList.add(0, EMPTY_QOS); > we can define disk profile with empty (null) qos object. what's the default? Line 197: getQos().setItems(qosList); Line 198: if (defaultQosId != null) { Line 199: for (StorageQos storageQos : qosList) { Line 200: if (defaultQosId.equals(storageQos.getId())) { Line 195: : (List<StorageQos>) ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 196: qosList.add(0, EMPTY_QOS); Line 197: getQos().setItems(qosList); Line 198: if (defaultQosId != null) { Line 199: for (StorageQos storageQos : qosList) { > since defaultQosId can be null, prefer to leave it like that. ok, can you at least extract it to another method Line 200: if (defaultQosId.equals(storageQos.getId())) { Line 201: getQos().setSelectedItem(storageQos); Line 202: break; Line 203: } Line 207: })); Line 208: } Line 209: Line 210: public boolean validate() { Line 211: getName().validateEntity(new IValidation[] { new NotEmptyValidation(), new AsciiNameValidation() }); > Done good, did u verify I18 won't be problematic here? Line 212: Line 213: return getName().getIsValid(); Line 214: } Line 215: http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/DiskProfileListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/DiskProfileListModel.java: Line 179: })); Line 180: } Line 181: Line 182: public StorageQos getStorageQos(Guid qosId) { Line 183: return qosMap.get(qosId); > why is it needed? for consistency, but not mandatory.. Line 184: } Line 185: Line 186: @Override Line 187: protected void entityPropertyChanged(Object sender, PropertyChangedEventArgs e) { http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 3804: Line 3805: @DefaultStringValue("Count") Line 3806: String iopsCountLabelQosPopup(); Line 3807: Line 3808: @DefaultStringValue("QoS") > prefer to have a designated constant for each string. ok Line 3809: String diskProfileQosLabel(); Line 3810: Line 3811: @DefaultStringValue("Disk Profiles") Line 3812: String diskProfilesSubTabLabel(); http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/storage/SubTabStorageDiskProfileView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/storage/SubTabStorageDiskProfileView.java: Line 28: Line 29: void initTable(final ApplicationConstants constants) { Line 30: getTable().enableColumnResizing(); Line 31: Line 32: TextColumnWithTooltip<DiskProfile> nameColumn = > will be added in a later patch. ok but it should be straight forward to add it. anything special that blocks you here? Line 33: new TextColumnWithTooltip<DiskProfile>() { Line 34: @Override Line 35: public String getValue(DiskProfile object) { Line 36: return object.getName(); -- To view, visit http://gerrit.ovirt.org/29672 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibe5cea3c563cf68efca0468749338a532894f709 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@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
