Ramesh N has posted comments on this change. Change subject: gluster: Add support for gluster brick creation ......................................................................
Patch Set 17: (13 comments) https://gerrit.ovirt.org/#/c/36031/17/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/CreateBrickModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/CreateBrickModel.java: Line 41: new > Please retain this immutable itself if it is only serving the ListModelList Done Line 46: setIsChangable > I see that the mapped sizeEditor is anyways a label editor Not required. Removed Line 85: getSelectedItems > can you please add a comment about why we added this I will add Line 126: public void setSize(EntityModel<String> size) { Line 127: this.size = size; Line 128: } Line 129: Line 130: public void setSelectedDevices(List<StorageDevice> selectedDevices) { > Can this be protected. I assume only the ListModel is using it Done Line 131: getStorageDevices().setSelectedItems(selectedDevices); Line 132: } Line 133: Line 134: public void setStorageDevices(ListModel<StorageDevice> storageDevices) { https://gerrit.ovirt.org/#/c/36031/17/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/HostGlusterStorageDevicesListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/HostGlusterStorageDevicesListModel.java: Line 125: == > can this happen? I see it handled already in updateActionAvailability. Right. Logically this case will never happen. I can remove the check. Line 178: setIsDefault > Line 177 and 178 not required as 176 already does it Done Line 177: okCommand.setTitle(ConstantsManager.getInstance().getConstants().ok()); Line 178: okCommand.setIsDefault(true); Line 179: lvModel.getCommands().add(okCommand); Line 180: Line 181: UICommand cancelCommand = UICommand.createDefaultCancelUiCommand("Cancel", this); //$NON-NLS-1$ > Kindly change this to UICommand.createCancelUiCommand Done Line 182: cancelCommand.setTitle(ConstantsManager.getInstance().getConstants().cancel()); Line 183: cancelCommand.setIsCancel(true); Line 184: lvModel.getCommands().add(cancelCommand); Line 185: } Line 179: lvModel.getCommands().add(okCommand); Line 180: Line 181: UICommand cancelCommand = UICommand.createDefaultCancelUiCommand("Cancel", this); //$NON-NLS-1$ Line 182: cancelCommand.setTitle(ConstantsManager.getInstance().getConstants().cancel()); Line 183: cancelCommand.setIsCancel(true); > 182 and 183 not required Done Line 184: lvModel.getCommands().add(cancelCommand); Line 185: } Line 186: Line 187: private void syncStorageDevices() { Line 235: Line 236: private void postCreateBrick() { Line 237: CreateBrickModel model = (CreateBrickModel) getWindow(); Line 238: model.stopProgress(); Line 239: setWindow(null); > can you please check if this closes the error dialog also in case of error. It shows the error dialog in case of vdsm/back end errors. Line 240: } Line 241: Line 242: @Override Line 243: public void executeCommand(UICommand command) { Line 243: public void executeCommand(UICommand command) { Line 244: super.executeCommand(command); Line 245: if (command.equals(getCreateBrickCommand())) { Line 246: showCreateBrickDialog(); Line 247: } else if (command.getName().equals("OnCreateBrick")) { //$NON-NLS-1$ > lower case first Done Line 248: createBrick(); Line 249: } else if (command.getName().equals("Cancel")) { //$NON-NLS-1$ Line 250: setWindow(null); Line 251: } else if (command.equals(getSyncStorageDevicesCommand())) { Line 245: if (command.equals(getCreateBrickCommand())) { Line 246: showCreateBrickDialog(); Line 247: } else if (command.getName().equals("OnCreateBrick")) { //$NON-NLS-1$ Line 248: createBrick(); Line 249: } else if (command.getName().equals("Cancel")) { //$NON-NLS-1$ > can we have it renamed to closeWindow. Just a thought bcoz we might later g Done Line 250: setWindow(null); Line 251: } else if (command.equals(getSyncStorageDevicesCommand())) { Line 252: syncStorageDevices(); Line 253: } https://gerrit.ovirt.org/#/c/36031/17/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/LocalizedEnums.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/LocalizedEnums.java: Line 508: String VdcActionType___RecoveryStoragePool(); Line 509: Line 510: String VdcActionType___UpdateVmInterface(); Line 511: Line 512: String VdcActionType___MigrateVmToServer(); > please make sure you have the corresponding VdcActionType entry as well. ot Done Line 513: Line 514: String VdcActionType___UpdateDisplayToVdsGroup(); Line 515: Line 516: String VdcActionType___ChangeDisk(); https://gerrit.ovirt.org/#/c/36031/17/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/CreateBrickPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/CreateBrickPopupView.java: Line 165: deviceTable.flush(); Line 166: return driver.flush(); Line 167: } Line 168: Line 169: public String formatSize(double size) { > Just thinking if we can give this a better place I have also used in good n May be we can add this also as part of SizeConvertor. But what if u want in a different format?. Line 170: return NumberFormat.getFormat("#.##").format(size);//$NON-NLS-1$ Line 171: } Line 172: Line 173: @Override -- To view, visit https://gerrit.ovirt.org/36031 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If43b6503dd8362d2a0907ac9648335a750828427 Gerrit-PatchSet: 17 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Ramesh N <rnach...@redhat.com> Gerrit-Reviewer: Sahina Bose <sab...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
