Lior Vernia has posted comments on this change. Change subject: webadmin: Network dialog- default/custom mtu as radio buttons ......................................................................
Patch Set 9: (15 comments) http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/EditNetworkModel.java: Line 51: } Line 52: Line 53: @Override Line 54: protected void initMtu() { Line 55: boolean isCustomMtu = getNetwork().getMtu() != 0; How about adding a method Network.isMtuCustom() and using it here instead? Alternatively, you can add a static constant somewhere (Network? NetworkModel?) DEFAULT_MTU instead of using this "magic number" everywhere. This approach would be better with respect to MtuRenderer (that doesn't have access to the network entity anymore). Line 56: getMtuSelector().setSelectedItem(isCustomMtu ? MtuSelector.customMtu : MtuSelector.defaultMtu); Line 57: getMtu().setEntity(isCustomMtu() ? getNetwork().getMtu() : null); Line 58: } Line 59: http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NetworkModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/NetworkModel.java: Line 126: }); Line 127: Line 128: ListModel<MtuSelector> mtuSelector = new ListModel<MtuSelector>(); Line 129: mtuSelector.setItems(Arrays.asList(MtuSelector.values())); Line 130: setMtuSelector(mtuSelector); You've already set mtuSelector, the call isn't necessary and neither is the setter method itself. Line 131: mtuSelector.getSelectedItemChangedEvent().addListener(new IEventListener() { Line 132: Line 133: @Override Line 134: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 467: && subnetValid && profilesValid && getNetworkLabel().getIsValid(); Line 468: } Line 469: Line 470: protected boolean isCustomMtu() { Line 471: return MtuSelector.customMtu.equals(getMtuSelector().getSelectedItem()); Note that enums can be used with ==, but as you prefer. Line 472: } Line 473: Line 474: public void syncWithBackend() { Line 475: final StoragePool dc = getSelectedDc(); Line 535: Line 536: String label = getNetworkLabel().getSelectedItem(); Line 537: network.setLabel(label == null || !label.isEmpty() ? label : null); Line 538: Line 539: network.setMtu(0); Similarly to the comment in EditNetworkModel, maybe encapsulate the logic of defaultMtu == 0 inside Network? Something like Network.resetMtuToDefault(). Or in the alternative approach, swap zero for the constant value. Line 540: if (getMtu().getIsChangable() && isCustomMtu()) Line 541: { Line 542: network.setMtu(Integer.parseInt(getMtu().getEntity().toString())); Line 543: } Line 536: String label = getNetworkLabel().getSelectedItem(); Line 537: network.setLabel(label == null || !label.isEmpty() ? label : null); Line 538: Line 539: network.setMtu(0); Line 540: if (getMtu().getIsChangable() && isCustomMtu()) If I'm not mistaken, if getMtu().getIsChangable() then necessarily isCustomMtu()?... I think the second check is redundant. Line 541: { Line 542: network.setMtu(Integer.parseInt(getMtu().getEntity().toString())); Line 543: } Line 544: Line 725: getMtu().setChangeProhibitionReason(prohibitionReason); Line 726: } Line 727: Line 728: getMtuSelector().setIsChangable(isChangeable); Line 729: getMtu().setIsChangable(isChangeable); Shouldn't this be getMtu().setIsChangable(isChangeable && isCustomMtu())? This also applies for the prohibition reason above. Line 730: } Line 731: Line 732: protected void updateMtuSelectorsChangeability() { Line 733: Line 760: private MtuSelector(String description) { Line 761: this.description = description; Line 762: } Line 763: Line 764: public String getDescription() { I don't think this getter is needed. Line 765: return description; Line 766: } Line 767: Line 768: @Override http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIMessages.java: Line 2: Line 3: import java.util.Date; Line 4: import java.util.List; Line 5: Line 6: import com.google.gwt.i18n.client.Messages.DefaultMessage; I think this was accidental and can be removed. Line 7: Line 8: public interface UIMessages extends com.google.gwt.i18n.client.Messages { Line 9: Line 10: @DefaultMessage("One of the parameters isn''t supported (available parameter(s): {0})") http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/AbstractNetworkPopupPresenterWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/AbstractNetworkPopupPresenterWidget.java: Line 77: getView().getQosButton().getCommand().execute(); Line 78: } Line 79: }); Line 80: Line 81: getView().addMtuEditor(); I don't think this is required, you can add the editor from within the view. Line 82: } Line 83: Line 84: @Override Line 85: protected void onReveal() { http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/AbstractNetworkPopupView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/AbstractNetworkPopupView.java: Line 444: } Line 445: Line 446: public void addMtuEditor() { Line 447: FlowPanel panel = mtuSelectorEditor.asRadioGroup().getPanel(MtuSelector.customMtu); Line 448: if (panel != null) { When can this be null? Line 449: panel.add(mtuEditor); Line 450: } Line 451: } Line 452: Line 445: Line 446: public void addMtuEditor() { Line 447: FlowPanel panel = mtuSelectorEditor.asRadioGroup().getPanel(MtuSelector.customMtu); Line 448: if (panel != null) { Line 449: panel.add(mtuEditor); This can probably be called either from the constructor after initWidget(), or from onLoad(). It doesn't need to be triggered from the presenter. Line 450: } Line 451: } Line 452: Line 453: interface WidgetStyle extends CssResource { http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/AbstractNetworkPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/AbstractNetworkPopupView.ui.xml: Line 117: <g:HorizontalPanel addStyleNames="{style.dependentField}"> Line 118: <ge:EntityModelCheckBoxEditor ui:field="isVmNetworkEditor" /> Line 119: <g:Image resource="{resources.networkVm}" addStyleNames="{style.vmNetworkImage}" /> Line 120: </g:HorizontalPanel> Line 121: <g:HorizontalPanel addStyleNames="{style.dependentField}"> I think you can remove the encompassing HorizontalPanel. Line 122: <e:ListModelRadioGroupEditor ui:field="mtuSelectorEditor" /> Line 123: </g:HorizontalPanel> Line 124: <w:EntityModelWidgetWithInfo ui:field="networkLabelWithInfo" addStyleNames="{style.dependentField} {style.propertyWidth} anpv_networkLabelPanel_pfly_fix" /> Line 125: <g:HorizontalPanel verticalAlignment="ALIGN_MIDDLE" visible="false"> http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/ItemInfoPopup.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/host/panels/ItemInfoPopup.java: Line 44: SafeHtml notInSyncImage = Line 45: SafeHtmlUtils.fromTrustedString(AbstractImagePrototype.create(resources.networkNotSyncImage()).getHTML()); Line 46: SafeHtml alertImage = Line 47: SafeHtmlUtils.fromTrustedString(AbstractImagePrototype.create(resources.alertImage()).getHTML()); Line 48: int defaultMtu = (Integer) AsyncDataProvider.getConfigValuePreConverted(ConfigurationValues.DefaultMtu); Can't this be a static member and only initialized once? Line 49: Line 50: public ItemInfoPopup() { Line 51: super(true); Line 52: contents.setCellPadding(5); Line 131: Line 132: // Mtu Line 133: if (!entity.isExternal()) { Line 134: addRow(constants.mtuItemInfo(), Line 135: entity.getMtu() == 0 ? messages.defaultMtu(defaultMtu) : String.valueOf(entity.getMtu())); Again, I would use here some method such as Network.isMtuCustom(), or alternatively the zero swapped for a constant value. Line 136: } Line 137: } Line 138: Line 139: private void showNic(NetworkInterfaceModel nic) { http://gerrit.ovirt.org/#/c/28099/9/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/renderer/MtuRenderer.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/widget/renderer/MtuRenderer.java: Line 15: private final ApplicationMessages messages = GWT.create(ApplicationMessages.class); Line 16: Line 17: @Override Line 18: public String render(Integer mtu) { Line 19: return mtu == 0 ? messages.defaultMtu(defaultMtu) : mtu.toString(); See previous comments concerning this logic, e.g. in EditNetworkModel. Line 20: } Line 21: -- To view, visit http://gerrit.ovirt.org/28099 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f813fd292b56407c97ae69c1580061bbc0eef04 Gerrit-PatchSet: 9 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@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
