Tomas Jelinek has posted comments on this change. Change subject: userportal/webadmin: cluster parameters override - UI enhancements ......................................................................
Patch Set 3: (13 comments) https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelTypeAheadChangeableListBox.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelTypeAheadChangeableListBox.java: Line 51: super.showAllSuggestions(); Line 52: suggestBox.setText(lastText); Line 53: } Line 54: Line 55: protected void setNullReplacementString(String nullReplacementText) { please delete this - look at AbstractVmPopupWidget Line 56: this.nullReplacementText = nullReplacementText; Line 57: } Line 58: Line 59: protected String getNullReplacementString() { Line 55: protected void setNullReplacementString(String nullReplacementText) { Line 56: this.nullReplacementText = nullReplacementText; Line 57: } Line 58: Line 59: protected String getNullReplacementString() { same Line 60: return nullReplacementText; Line 61: } https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelTypeAheadChangeableListBoxEditor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/editor/ListModelTypeAheadChangeableListBoxEditor.java: Line 49: return data; // for string objects the replacement value is the object itself (null safe) Line 50: } Line 51: } Line 52: Line 53: public void setNullReplacementString(String nullReplacementString) { please delete this - look at comment in AbstractVmPopupWidget Line 54: super.getContentWidget().setNullReplacementString(nullReplacementString); Line 55: } Line 56: Line 57: public String getNullReplacementString() { Line 53: public void setNullReplacementString(String nullReplacementString) { Line 54: super.getContentWidget().setNullReplacementString(nullReplacementString); Line 55: } Line 56: Line 57: public String getNullReplacementString() { same Line 58: return super.getContentWidget().getNullReplacementString(); Line 59: } https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/AbstractVmPopupWidget.java: Line 1273: Line 1274: @Override Line 1275: public String getDisplayStringNullSafe(String data) { Line 1276: if (data == null || data.trim().isEmpty()) { Line 1277: data = emulatedMachine.getNullReplacementString(); this getting the string from inside the editor and than passing it back is not a good design. The really correct approach would be to create a constructor in ListModelTypeAheadChangeableListBox which would not get the renderer from outside and would create it's own which would do this logic since it is same for every instance of it. But if not this, please use the previous approach of passing the constants.clusterDefaultOption() Line 1278: } Line 1279: return typeAheadNameTemplateNullSafe(data); Line 1280: } Line 1281: }, Line 1288: Line 1289: @Override Line 1290: public String getDisplayStringNullSafe(String data) { Line 1291: if (data == null || data.trim().isEmpty()) { Line 1292: data = customCpu.getNullReplacementString(); same Line 1293: } Line 1294: return typeAheadNameTemplateNullSafe(data); Line 1295: } Line 1296: }, https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 4163: new VdsIdParametersBase(hostId), Line 4164: aQuery); Line 4165: } Line 4166: Line 4167: public void verifyClusterSupportsVmCpu(AsyncQuery aQuery, Guid vdsGroupId, String newCpuName) { this is strangely named - from the name I would expect it to return a Boolean and not a Map. Can you please consider renaming it? Line 4168: aQuery.converterCallback = new IAsyncConverter() { Line 4169: @Override Line 4170: public Object Convert(Object source, AsyncQuery _asyncQuery) Line 4171: { Line 4167: public void verifyClusterSupportsVmCpu(AsyncQuery aQuery, Guid vdsGroupId, String newCpuName) { Line 4168: aQuery.converterCallback = new IAsyncConverter() { Line 4169: @Override Line 4170: public Object Convert(Object source, AsyncQuery _asyncQuery) Line 4171: { in new code please use java formatting Line 4172: if (source != null) Line 4173: { Line 4174: return source; Line 4175: } https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/help/HelpTag.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/help/HelpTag.java: Line 261: migrate_virtual_machine("migrate_virtual_machine", HelpTagType.WEBADMIN, "VMs Tab > Migrate Virtual Machine(s)"), //$NON-NLS-1$ //$NON-NLS-2$ Line 262: Line 263: edit_next_run_configuration("edit_next_run_configuration", HelpTagType.COMMON, "'VMs' main tab -> 'Edit VM' dialog [replaces the old 'Edit Desktop' (edit_desktop) and 'Edit Server' (edit_server)]"), //$NON-NLS-1$ //$NON-NLS-2$ Line 264: Line 265: edit_unsupported_cpu("edit_unsupported_cpu", HelpTagType.COMMON, "'VMs' main tab -> 'Edit VM' dialog [replaces the old 'Edit Desktop' (edit_desktop) and 'Edit Server' (edit_server)]"), //$NON-NLS-1$ //$NON-NLS-2$ the comment seems to be quite incorrect :) Line 266: Line 267: monitor("monitor", HelpTagType.UNKNOWN), //$NON-NLS-1$ Line 268: Line 269: move_disk("move_disk", HelpTagType.WEBADMIN, "VMs Tab > Virtual Disks Sub-Tab > Move Virtual Disk"), //$NON-NLS-1$ //$NON-NLS-2$ https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterListModel.java: Line 707: } else { Line 708: message.append(ConstantsManager.getInstance() Line 709: .getConstants() Line 710: .changeCpuLevelWarningMessage()); Line 711: cpuLevelConfirmationWindow(message.toString()); it takes two server calls until you get here which may take long on slow network and you have no idea that you have to confirm something. You even can be on a different tab. Could you please consider to have a window with progress bar having shown until you get here? Line 712: } Line 713: } Line 714: }; Line 715: AsyncDataProvider.getInstance() https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolListModel.java: Line 458: onSavePhase2 this does not signify what does this do... https://gerrit.ovirt.org/#/c/38407/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/templates/TemplateListModel.java: Line 629: .getConstants() Line 630: .nameMustBeUniqueInvalidReason()); Line 631: model.getName().setIsValid(false); Line 632: model.setValidTab(TabName.GENERAL_TAB, false); Line 633: return; this return is not needed here since it is the last line of the method Line 634: } Line 635: Line 636: } Line 637: }), name); Line 635: Line 636: } Line 637: }), name); Line 638: } Line 639: String selectedCpu = model.getCustomCpu().getSelectedItem(); ...and this part is going to be executed in parallel to the name uniqueness check ignoring it's results Line 640: if (selectedCpu != null && !selectedCpu.isEmpty() && !model.getCustomCpu().getItems().contains(selectedCpu)) { Line 641: ConfirmationModel confirmModel = new ConfirmationModel(); Line 642: confirmModel.setTitle(ConstantsManager.getInstance().getConstants().vmUnsupportedCpuTitle()); Line 643: confirmModel.setMessage(ConstantsManager.getInstance().getConstants().vmUnsupportedCpuMessage()); -- To view, visit https://gerrit.ovirt.org/38407 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2e1c458dc4e4b6e61d2a1b92327287166a12afa2 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <[email protected]> Gerrit-Reviewer: Eldan Shachar <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Betak <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
