Tomas Jelinek has posted comments on this change. Change subject: frontend: Add VM HostDevice subtab ......................................................................
Patch Set 41: (10 comments) overall a good patch - some small comments https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/hostdev/AddVmHostDevicesModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/hostdev/AddVmHostDevicesModel.java: Line 43: Line 44: private VM vm; Line 45: private Set<String> alreadyAttachedDevices = new HashSet<>(); Line 46: Line 47: @Inject do you need this annotation? Line 48: public AddVmHostDevicesModel() { Line 49: setPinnedHost(new ListModel<VDS>()); Line 50: setCapability(new ListModel<String>()); Line 51: setAllAvailableHostDevices(new ListModel<EntityModel<HostDeviceView>>()); Line 85: initCapabilities(); Line 86: fetchExistingDevices(); Line 87: } Line 88: Line 89: private void postExistingDeviceInit() { no need for this method - please use the initHosts() directly Line 90: initHosts(); Line 91: } Line 92: Line 93: private void fetchExistingDevices() { Line 245: super.executeCommand(command); Line 246: } Line 247: } Line 248: Line 249: private void addDevice() { please delete Line 250: //Window.alert("Device added"); //$NON-NLS-1$ Line 251: } Line 252: Line 253: private void removeDevice() { Line 249: private void addDevice() { Line 250: //Window.alert("Device added"); //$NON-NLS-1$ Line 251: } Line 252: Line 253: private void removeDevice() { please delete Line 254: //Window.alert("Device removed"); //$NON-NLS-1$ Line 255: } https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/hostdev/VmHostDeviceListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/hostdev/VmHostDeviceListModel.java: Line 151: model.startProgress(null); Line 152: if (getEntity().getDedicatedVmForVds() == null || !getEntity().getDedicatedVmForVds().equals(model.getPinnedHost().getSelectedItem().getId())) { Line 153: pinVmToHost(model.getPinnedHost().getSelectedItem().getId(), new IFrontendActionAsyncCallback() { Line 154: @Override Line 155: public void executed(FrontendActionAsyncResult result) { here you should continue only if result.getReturnValue().getSucceeded() Line 156: doAttachDevices(model.getSelectedHostDevices().getItems()); Line 157: } Line 158: }); Line 159: } else { https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/AddVmHostDevicePopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/AddVmHostDevicePopupView.java: Line 147: private String renderCommaSeparated(Collection<String> strings) { Line 148: if (strings == null || strings.size() == 0) { Line 149: return ""; //$NON-NLS-1$ Line 150: } Line 151: Iterator<String> iterator = strings.iterator(); You can use StringUtils.join from org.ovirt.engine.ui.uicompat.external for this task Line 152: if (strings.size() == 1) { Line 153: return iterator.next(); Line 154: } Line 155: StringBuilder builder = new StringBuilder(iterator.next()); https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/AddVmHostDevicePopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/AddVmHostDevicePopupView.ui.xml: Line 10: <ui:with field="constants" type="org.ovirt.engine.ui.webadmin.ApplicationConstants" /> Line 11: Line 12: <d:SimpleDialogPanel width="1200px" height="800px"> Line 13: <d:content> Line 14: <g:VerticalPanel> If it will not break horribly please replace this VerticalPanel (e.g. <Table>) by FlowPanel (e.g. <div>) Line 15: <g:FlowPanel> Line 16: <e:ListModelListBoxEditor ui:field="pinnedHostEditor" /> Line 17: </g:FlowPanel> Line 18: <e:ListModelListBoxEditor ui:field="capabilityEditor" label="{constants.capability}" /> https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/VmRepinHostPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/VmRepinHostPopupView.java: Line 41: this.driver = driver; Line 42: driver.initialize(this); Line 43: } Line 44: Line 45: private void initEditors() { you can remove this Line 46: } Line 47: Line 48: @Override Line 49: public void edit(VmHostDeviceListModel object) { https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/VmRepinHostPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/vm/VmRepinHostPopupView.ui.xml: Line 10: </ui:style> Line 11: Line 12: <d:SimpleDialogPanel> Line 13: <d:content> Line 14: <g:VerticalPanel> Please replace this VerticalPanel (e.g. <Table>) by FlowPanel (e.g. <div>) Line 15: <e:ListModelListBoxEditor ui:field="pinnedHostEditor" addStyleNames="{style.pinnedHostEditor}" /> Line 16: </g:VerticalPanel> Line 17: </d:content> Line 18: </d:SimpleDialogPanel> https://gerrit.ovirt.org/#/c/37620/41/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/hostdev/HostDeviceTable.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/hostdev/HostDeviceTable.java: Line 1: package org.ovirt.engine.ui.webadmin.widget.hostdev; Line 2: Line 3: public class HostDeviceTable { I don't see this class used - please remove -- To view, visit https://gerrit.ovirt.org/37620 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8f2648e3c1073f630b9637aea3a6acc6f800cf6c Gerrit-PatchSet: 41 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
