Gilad Chaplik has posted comments on this change. Change subject: webadmin: introduce disk profiles ......................................................................
Patch Set 5: (32 comments) new patch uploaded 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: > please elaborate on each item I will add a link to feature page. anything specific do you want me to add 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: Line 8: 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: > please consider adding screenshots will be added in feature page. Line 13: Change-Id: Ibe5cea3c563cf68efca0468749338a532894f709 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: Line 13: Change-Id: Ibe5cea3c563cf68efca0468749338a532894f709 > add feature page url Done 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 { > can u share code with VnicProfileModel? (creating abstract base/etc..) I've build current infra for profiles, that it will later we fitted to other entities as well (like vnic profiles). this will be done in a later patch. Line 33: private final static StorageQos EMPTY_QOS; Line 34: Line 35: static { Line 36: EMPTY_QOS = new StorageQos(); Line 39: } Line 40: Line 41: private EntityModel<String> name; Line 42: private EntityModel<String> description; Line 43: private final EntityModel sourceModel; > use generics if applicable it's not. Line 44: private ListModel<StorageDomain> storageDomains; Line 45: private ListModel<StorageQos> qos; Line 46: private DiskProfile diskProfile = null; Line 47: private final Guid defaultQosId; Line 42: private EntityModel<String> description; Line 43: private final EntityModel sourceModel; Line 44: private ListModel<StorageDomain> storageDomains; Line 45: private ListModel<StorageQos> qos; Line 46: private DiskProfile diskProfile = null; > why initialize? Done Line 47: private final Guid defaultQosId; Line 48: private final VdcActionType vdcActionType; Line 49: Line 50: public EntityModel<String> getName() Line 89: this.qos = qos; Line 90: } Line 91: Line 92: public DiskProfileBaseModel(EntityModel sourceModel, Line 93: Version dcCompatibilityVersion, > Is this being used? no, removed. Line 94: Guid dcId, Line 95: Guid defaultQosId, Line 96: VdcActionType vdcActionType) { Line 97: this.sourceModel = sourceModel; 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); > why adding an empty one? we can define disk profile with empty (null) qos object. 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) { > try using 'setItems(Iterable<T> value, T selectedItem)' since defaultQosId can be null, prefer to leave it like that. 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() }); > why not using SpecialAsciiI18NOrNoneValidation ? Done 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 25: import org.ovirt.engine.ui.uicompat.ConstantsManager; Line 26: import org.ovirt.engine.ui.uicompat.PropertyChangedEventArgs; Line 27: Line 28: @SuppressWarnings("unused") Line 29: public class DiskProfileListModel extends SearchableListModel > can u share code with VnicProfileListModel (abstract/etc..) same as previous reply to same comment. Line 30: { Line 31: private UICommand newCommand; Line 32: private UICommand editCommand; Line 33: private UICommand removeCommand; Line 70: removeCommand = value; Line 71: } Line 72: Line 73: private Version getCompatibilityVersion() { Line 74: // TODO: > please do? :) Done Line 75: return null; Line 76: } Line 77: Line 78: public void newProfile() { Line 91: if (getWindow() != null) { Line 92: return; Line 93: } Line 94: Line 95: EditDiskProfileModel model = > format Done Line 96: new EditDiskProfileModel(this, Line 97: getCompatibilityVersion(), Line 98: (DiskProfile) getSelectedItem(), Line 99: getEntity().getStoragePoolId()); Line 121: setWindow(null); Line 122: } Line 123: Line 124: @Override Line 125: public StorageDomain getEntity() { > please gather all getters/setters in one location Done Line 126: return (StorageDomain) ((super.getEntity() instanceof StorageDomain) ? super.getEntity() : null); Line 127: } Line 128: Line 129: public void setEntity(StorageDomain value) { Line 165: for (StorageQos storageQos : qosList) { Line 166: qosMap.put(storageQos.getId(), storageQos); Line 167: } Line 168: } Line 169: Frontend.getInstance().runQuery(VdcQueryType.GetDiskProfilesByStorageDomainId, > too many canonical queries :) please try to extract properly I think it's okay, not too complicated (the inner part is basically setItems, imo totally readable). but if you insist, I have no problem changing that. Line 170: new IdQueryParameters(DiskProfileListModel.this.getEntity().getId()), Line 171: new AsyncQuery(new INewAsyncCallback() { Line 172: Line 173: @Override Line 171: new AsyncQuery(new INewAsyncCallback() { Line 172: Line 173: @Override Line 174: public void onSuccess(Object model1, Object returnValue1) { Line 175: DiskProfileListModel.this.setItems((List<DiskProfile>) ((VdcQueryReturnValue) returnValue1).getReturnValue()); > prefix could be removed once propely extracted same. Line 176: } Line 177: })); Line 178: } Line 179: })); Line 178: } Line 179: })); Line 180: } Line 181: Line 182: public StorageQos getStorageQos(Guid qosId) { > please gather all getters/setters in one location Done Line 183: return qosMap.get(qosId); Line 184: } Line 185: Line 186: @Override Line 179: })); Line 180: } Line 181: Line 182: public StorageQos getStorageQos(Guid qosId) { Line 183: return qosMap.get(qosId); > setter? why is it needed? 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/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/RemoveDiskProfileModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/profiles/RemoveDiskProfileModel.java: Line 19: Line 20: public class RemoveDiskProfileModel extends ConfirmationModel { Line 21: Line 22: private final List<DiskProfile> profiles; Line 23: private final boolean fullMsg; > needed as class member removed altogether Line 24: private final ListModel sourceListModel; Line 25: Line 26: public RemoveDiskProfileModel(ListModel sourceListModel, List<DiskProfile> profiles, boolean isFullMsg) { Line 27: setHelpTag(HelpTag.remove_disk_profile); Line 29: setHashName("remove_disk_prfoile"); //$NON-NLS-1$ Line 30: Line 31: this.sourceListModel = sourceListModel; Line 32: this.profiles = profiles; Line 33: this.fullMsg = isFullMsg; > what's the usage of this flag? please add comment on this Done Line 34: Line 35: ArrayList<String> items = new ArrayList<String>(); Line 36: for (DiskProfile profile : profiles) { Line 37: if (isFullMsg) { Line 34: Line 35: ArrayList<String> items = new ArrayList<String>(); Line 36: for (DiskProfile profile : profiles) { Line 37: if (isFullMsg) { Line 38: items.add(getRemoveDiskProfileFullMsg(profile)); > why do we need both versions (full/short..)? Done Line 39: } else { Line 40: items.add(profile.getName()); Line 41: } Line 42: } Line 41: } Line 42: } Line 43: setItems(items); Line 44: Line 45: UICommand tempVar = new UICommand("OnRemove", this); //$NON-NLS-1$ > rename Done Line 46: tempVar.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 47: tempVar.setIsDefault(true); Line 48: getCommands().add(tempVar); Line 49: UICommand tempVar2 = new UICommand("Cancel", this); //$NON-NLS-1$ Line 45: UICommand tempVar = new UICommand("OnRemove", this); //$NON-NLS-1$ Line 46: tempVar.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 47: tempVar.setIsDefault(true); Line 48: getCommands().add(tempVar); Line 49: UICommand tempVar2 = new UICommand("Cancel", this); //$NON-NLS-1$ > rename Done Line 50: tempVar2.setTitle(ConstantsManager.getInstance().getConstants().cancel()); Line 51: tempVar2.setIsCancel(true); Line 52: getCommands().add(tempVar2); Line 53: } Line 56: if (getProgress() != null) { Line 57: return; Line 58: } Line 59: Line 60: ArrayList<VdcActionParametersBase> list = new ArrayList<VdcActionParametersBase>(); > rename - a bit more specific :) Done Line 61: for (DiskProfile profile : getProfiles()) { Line 62: VdcActionParametersBase parameters = getRemoveDiskProfileParams(profile); Line 63: list.add(parameters); Line 64: Line 69: Frontend.getInstance().runMultipleAction(VdcActionType.RemoveDiskProfile, list, Line 70: new IFrontendMultipleActionAsyncCallback() { Line 71: @Override Line 72: public void executed(FrontendMultipleActionAsyncResult result) { Line 73: > format Done Line 74: stopProgress(); Line 75: cancel(); Line 76: Line 77: } http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/StorageListModel.java: Line 1280: // Create storage connection. Line 1281: StorageServerConnections connection = new StorageServerConnections(); Line 1282: connection.setconnection(path); Line 1283: connection.setstorage_type(posixModel.getType()); Line 1284: connection.setVfsType(posixModel.getVfsType().getEntity()); > same for the rest of this file :) sorry for all the noise... damn auto formatter :) added a depending patch that fix it. Line 1285: connection.setMountOptions(posixModel.getMountOptions().getEntity()); Line 1286: this.connection = connection; Line 1287: Line 1288: ArrayList<VdcActionType> actionTypes = new ArrayList<VdcActionType>(); http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java: Line 242: Line 243: @DefaultMessage("VM Interface Profile {0} from Network {1}") Line 244: String vnicProfileFromNetwork(String vnicProfile, String network); Line 245: Line 246: @DefaultMessage("Disk Profile {0} from Storage Domain {1}") > suggestion: add 'was' or 'has been' before the verb removed (was part of fullMsg) Line 247: String diskProfileFromStorageDomain(String diskProfile, String storageDomain); Line 248: Line 249: @DefaultMessage("Vnic {0} from Template {1}") Line 250: String vnicFromTemplate(String vnic, String template); 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") > don't we have a generic constant for that already? prefer to have a designated constant for each string. 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/presenter/tab/storage/SubTabStorageDiskProfilePresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/storage/SubTabStorageDiskProfilePresenter.java: Line 36: Line 37: @TabInfo(container = StorageSubTabPanelPresenter.class) Line 38: static TabData getTabData(ApplicationConstants applicationConstants, Line 39: SearchableDetailModelProvider<DiskProfile, StorageListModel, DiskProfileListModel> modelProvider) { Line 40: return new ModelBoundTabData(applicationConstants.diskProfilesSubTabLabel(), 9, > q: is it the right location - i.e. between which tabs is '9'? Disk Snapshots and Permissions. Line 41: modelProvider); Line 42: } Line 43: Line 44: @Inject http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/DiskProfilePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/DiskProfilePopupView.java: Line 91: @Override Line 92: public void edit(DiskProfileBaseModel object) { Line 93: driver.edit(object); Line 94: } Line 95: > setTabIndexes? (check if needed) needed, Done Line 96: @Override Line 97: public DiskProfileBaseModel flush() { Line 98: return driver.flush(); Line 99: } http://gerrit.ovirt.org/#/c/29672/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/DiskProfilePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/profile/DiskProfilePopupView.ui.xml: Line 1: <?xml version="1.0" encoding="UTF-8"?> Line 2: <!DOCTYPE ui:UiBinder SYSTEM "http://dl.google.com/gwt/DTD/xhtml.ent";> Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui" > tws Done 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: 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 = > sorting will be added in a later patch. 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
