Alona Kaplan 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? This patch is about changing the mtu from checkbox with text box to radio boxes and showing the default mtu value. Your comment is not related to this patch. It can be done in a separate cleanup patch. 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 The mtuSelector I set is not this.mtuSelector, it is the method's inner member. So, I have to use the set to init the class d.m 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. Done 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 o Same a the response on EditNetworkMode :) 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 isCustom Done 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())? T Done 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. Done 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. Done 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 Please see the comment in 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? Done 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(), 1. Cannot be called after initWidget(). It should be called after the driver initialization which is done in each child separately. 2. I guess it can be called from onLoad() but I"m not sure how it is better then from the presenter. The call to the function should be done after the model init. So calling it from presenter.init() seems fine. 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. Done 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? Done 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 alter Again, same answer. There are many more places that have to be refactored in case your suggestion is applied. It is not in the scope of this patch. 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. same 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
