Daniel Erez has posted comments on this change. Change subject: webadmin: Allow to configure ISCSI multipathing in the dc context ......................................................................
Patch Set 5: (13 comments) partial review http://gerrit.ovirt.org/#/c/22953/5/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/pool/IscsiBondPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/pool/IscsiBondPopupWidget.java: Line 62: Line 63: private final Driver driver = GWT.create(Driver.class); Line 64: Line 65: public IscsiBondPopupWidget(CommonApplicationConstants constants) { Line 66: initWidget(ViewUiBinder.uiBinder.createAndBindUi(this)); extract the setLabel statements to localize method Line 67: nameEditor.setLabel(constants.name()); Line 68: descriptionEditor.setLabel(constants.description()); Line 69: ViewIdHandler.idHandler.generateAndSetIds(this); Line 70: initLogicalNetworksPanel(); Line 74: driver.initialize(this); Line 75: } Line 76: Line 77: private void initLogicalNetworksPanel() { Line 78: VerticalPanel panel = new VerticalPanel(); why do you need the panel wrapper? scrolling issues? Line 79: networksTable = new ListModelObjectCellTable<Network, ListModel>(true); Line 80: panel.add(networksTable); Line 81: logicalNetworksPanel.setWidget(panel); Line 82: } Line 81: logicalNetworksPanel.setWidget(panel); Line 82: } Line 83: Line 84: private void initStorageConnectionsPanel() { Line 85: VerticalPanel panel = new VerticalPanel(); same Line 86: connectionsTable = new ListModelObjectCellTable<StorageServerConnections, ListModel>(true); Line 87: panel.add(connectionsTable); Line 88: storageTargetsPanel.setWidget(panel); Line 89: } Line 92: networksTable.enableColumnResizing(); Line 93: Line 94: TextColumnWithTooltip<Network> nameColumn = new TextColumnWithTooltip<Network>() { Line 95: @Override Line 96: public String getValue(Network ntw) { s/ntw/network Line 97: return ntw.getName(); Line 98: } Line 99: }; Line 100: networksTable.addColumn(nameColumn, constants.name(), "40%"); //$NON-NLS-1$ Line 96: public String getValue(Network ntw) { Line 97: return ntw.getName(); Line 98: } Line 99: }; Line 100: networksTable.addColumn(nameColumn, constants.name(), "40%"); //$NON-NLS-1$ width should be specified in pixels (needed for column resizing support) Line 101: Line 102: TextColumnWithTooltip<Network> descriptionColumn = new TextColumnWithTooltip<Network>() { Line 103: @Override Line 104: public String getValue(Network ntw) { Line 104: public String getValue(Network ntw) { Line 105: return ntw.getDescription(); Line 106: } Line 107: }; Line 108: networksTable.addColumn(descriptionColumn, constants.description(), "60%"); //$NON-NLS-1$ same Line 109: Line 110: networksTable.setWidth("100%", true); //$NON-NLS-1$ Line 111: } Line 112: Line 112: Line 113: private void initConnectionsTable(CommonApplicationConstants constants) { Line 114: connectionsTable.enableColumnResizing(); Line 115: Line 116: TextColumnWithTooltip<StorageServerConnections> iqnColumn = new TextColumnWithTooltip<StorageServerConnections>() { same comment for this table Line 117: @Override Line 118: public String getValue(StorageServerConnections conn) { Line 119: return conn.getiqn(); Line 120: } http://gerrit.ovirt.org/#/c/22953/5/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/pool/IscsiBondPopupWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/pool/IscsiBondPopupWidget.ui.xml: Line 13: border-bottom: 1px #C0C0C0 solid; Line 14: margin-top: 5px; Line 15: padding: 0px; Line 16: } Line 17: just use the same styles: i.e. * .logicalNetworksPanel, * .storageTargetsPanel Line 18: .storageTargetsPanel { Line 19: border-top: 1px #C0C0C0 solid; Line 20: border-bottom: 1px #C0C0C0 solid; Line 21: margin-top: 5px; Line 22: padding: 0px; Line 23: } Line 24: Line 25: .textBox { Line 26: width: 90%; does 100% cause any issues? Line 27: } Line 28: Line 29: .labelStyle { Line 30: font-size: 14px; http://gerrit.ovirt.org/#/c/22953/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ListModel.java: Line 311: } Line 312: } Line 313: } Line 314: Line 315: public boolean isSelectionsEmpty() Not sure we should introduce the api this way, since getSelectedItem could contain a value - might be a bit confusing? Line 316: { Line 317: return getSelectedItems() == null || getSelectedItems().isEmpty(); Line 318: } Line 319: http://gerrit.ovirt.org/#/c/22953/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterIscsiBondListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterIscsiBondListModel.java: Line 139: setWindow(model); Line 140: Line 141: model.getLatch().setEntity(false); Line 142: Line 143: ArrayList<String> items = new ArrayList<String>(); consider extracting to a method Line 144: for (Object item : getSelectedItems()) { Line 145: IscsiBondModel iscsiBondModel = new IscsiBondModel(); Line 146: iscsiBondModel.setIscsiBond((IscsiBond) item); Line 147: iscsiBondModel.setStoragePool(getEntity()); Line 213: @Override Line 214: public void onSuccess(Object model, Object ret) Line 215: { Line 216: ArrayList<IscsiBond> items = ((VdcQueryReturnValue) ret).getReturnValue(); Line 217: SearchableListModel searchableListModel = (SearchableListModel) model; s/searchableListModel/dataCenterIscsiBondListModel Line 218: searchableListModel.setItems(items); Line 219: } Line 220: }; Line 221: Line 237: } Line 238: Line 239: private void cancel() { Line 240: setWindow(null); Line 241: Frontend.getInstance().unsubscribe(); no need - didn't call Frontend's subscribe() Line 242: } -- To view, visit http://gerrit.ovirt.org/22953 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0b9effc07b9a52873f5a4507d882ff76ee091026 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> 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
