Lior Vernia has posted comments on this change. Change subject: webadmin: adding netwrok-out-of-sync column to Hosts's Netwrok Interfaces ......................................................................
Patch Set 2: (10 comments) https://gerrit.ovirt.org/#/c/38291/2//COMMIT_MSG Commit Message: Line 9: Line 10: adding sync column inidcating network sync status Line 11: Line 12: Change-Id: Id200f9c135f1097572feac300e255a438cfb295a Line 13: Bug-Url: https://bugzilla.redhat.com/?????? WTF? https://gerrit.ovirt.org/#/c/38291/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/host/SubTabHostInterfaceView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/host/SubTabHostInterfaceView.java: Line 88: Line 89: // Vlan Panel Line 90: table.addColumn(new EmptyColumn(), constants.vlanInterface(), "200px"); //$NON-NLS-1$ Line 91: table.addColumn(new EmptyColumn(), constants.networkNameInterface(), "200px"); //$NON-NLS-1$ Line 92: table.addColumn(new EmptyColumn(), constants.hostOutOfSync(), "75px"); //$NON-NLS-1$ I would put this column to the left of the network name column. Line 93: table.addColumn(new EmptyColumn(), constants.addressInterface(), "120px"); //$NON-NLS-1$ Line 94: Line 95: // Statistics Panel Line 96: table.addColumn(new EmptyColumn(), constants.macInterface(), "120px"); //$NON-NLS-1$ https://gerrit.ovirt.org/#/c/38291/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/VLanPanel.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/host/VLanPanel.java: Line 63: Grid createRow(final HostVLan hostVLan) { Line 64: Grid row = new Grid(1, 4); Line 65: row.getColumnFormatter().setWidth(0, VLanPanel.CHECK_BOX_COLUMN_WIDTH); Line 66: row.getColumnFormatter().setWidth(1, VLanPanel.NETWORK_NAME_COLUMN_WIDTH); Line 67: row.getColumnFormatter().setWidth(2, VLanPanel.OUT_OF_SYNC_WIDTH); This would change accordingly, as well as several other spots in this file... Line 68: row.getColumnFormatter().setWidth(3, VLanPanel.ADDRESS_COLUMN_WIDTH); Line 69: Line 70: row.setWidth("100%"); //$NON-NLS-1$ Line 71: row.setHeight("100%"); //$NON-NLS-1$ Line 110: Grid row = new Grid(1, 4); Line 111: row.getColumnFormatter().setWidth(0, VLanPanel.CHECK_BOX_COLUMN_WIDTH); Line 112: row.getColumnFormatter().setWidth(1, VLanPanel.NETWORK_NAME_COLUMN_WIDTH); Line 113: row.getColumnFormatter().setWidth(2, VLanPanel.OUT_OF_SYNC_WIDTH); Line 114: row.getColumnFormatter().setWidth(3, VLanPanel.ADDRESS_COLUMN_WIDTH); This code duplication doesn't bother you? I'd create a preceding patch extracting the common logic among createRow() and createBlankRow() to a separate method, and then only change this once. Line 115: Line 116: row.setWidth("100%"); //$NON-NLS-1$ Line 117: row.setHeight("100%"); //$NON-NLS-1$ Line 118: Line 129: Line 130: row.setWidget(0, 1, networkName); Line 131: Line 132: // Network sync status Line 133: VdsNetworkInterface iface = lineModel.getIsBonded()? lineModel.getInterface() : lineModel.getInterfaces().get(0).getInterface(); This isn't formatted. As usual. Line 134: VdsNetworkInterface.NetworkImplementationDetails ifaceNetworkImpDetails = (iface != null) ? iface.getNetworkImplementationDetails() : null; Line 135: boolean isSYnc = (ifaceNetworkImpDetails != null) ? ifaceNetworkImpDetails.isInSync() : true; Line 136: Line 137: row.setWidget(0, 2, getSyncPanel(isSYnc)); Line 130: row.setWidget(0, 1, networkName); Line 131: Line 132: // Network sync status Line 133: VdsNetworkInterface iface = lineModel.getIsBonded()? lineModel.getInterface() : lineModel.getInterfaces().get(0).getInterface(); Line 134: VdsNetworkInterface.NetworkImplementationDetails ifaceNetworkImpDetails = (iface != null) ? iface.getNetworkImplementationDetails() : null; Is it possible for iface to be null? Line 135: boolean isSYnc = (ifaceNetworkImpDetails != null) ? ifaceNetworkImpDetails.isInSync() : true; Line 136: Line 137: row.setWidget(0, 2, getSyncPanel(isSYnc)); Line 138: Line 131: Line 132: // Network sync status Line 133: VdsNetworkInterface iface = lineModel.getIsBonded()? lineModel.getInterface() : lineModel.getInterfaces().get(0).getInterface(); Line 134: VdsNetworkInterface.NetworkImplementationDetails ifaceNetworkImpDetails = (iface != null) ? iface.getNetworkImplementationDetails() : null; Line 135: boolean isSYnc = (ifaceNetworkImpDetails != null) ? ifaceNetworkImpDetails.isInSync() : true; 1. isSYnc --> isSync? 2. Can network implementation details be null? Line 136: Line 137: row.setWidget(0, 2, getSyncPanel(isSYnc)); Line 138: Line 139: // Address Line 141: Line 142: return row; Line 143: } Line 144: Line 145: private HorizontalPanel getSyncPanel(final boolean isSync) { This method name isn't good - it doesn't fetch an existing panel, it creates a new one. Line 146: HorizontalPanel output = new HorizontalPanel() { Line 147: { Line 148: if (isSelectionAvailable) { Line 149: add(getCheckBox()); Line 142: return row; Line 143: } Line 144: Line 145: private HorizontalPanel getSyncPanel(final boolean isSync) { Line 146: HorizontalPanel output = new HorizontalPanel() { Why are you creating an anonymous inner class instead of instantiating a HorizontalPanel and then setting its attributes? Line 147: { Line 148: if (isSelectionAvailable) { Line 149: add(getCheckBox()); Line 150: } Line 144: Line 145: private HorizontalPanel getSyncPanel(final boolean isSync) { Line 146: HorizontalPanel output = new HorizontalPanel() { Line 147: { Line 148: if (isSelectionAvailable) { Why is this needed? Line 149: add(getCheckBox()); Line 150: } Line 151: Line 152: if (!isSync) { -- To view, visit https://gerrit.ovirt.org/38291 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id200f9c135f1097572feac300e255a438cfb295a Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eliraz Levi <[email protected]> Gerrit-Reviewer: Lior Vernia <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
