anmolbabu has posted comments on this change. Change subject: gluster: Add support for gluster brick creation ......................................................................
Patch Set 17: (13 comments) Some minor 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 ListModelListBoxEditor Line 46: setIsChangable I see that the mapped sizeEditor is anyways a label editor then why is this required? Line 85: getSelectedItems can you please add a comment about why we added this 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 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. Even if it happens, just by a very rare coincidence, I was just thinking can you please show a error dialog? Line 178: setIsDefault Line 177 and 178 not required as 176 already does it 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 as 2 commands both simulataneously shouldn't be default actions 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 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. 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 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 get a confirmWindow also to use somewhere in here and then cancel might just turn ambiguous 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. otherwise it keeps showing a warning in the debug mode :)) 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 number of places. And in your patch also I see it used in atleast 2 or 3 places. 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
