Lior Vernia has posted comments on this change.
Change subject: webadmin: Ability to add external subnet
......................................................................
Patch Set 1:
(19 comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java
Line 3444: };
Line 3445:
Frontend.getInstance().runQuery(VdcQueryType.GetExternalSubnetsOnProviderByNetwork,
new IdQueryParameters(networkId), aQuery);
Line 3446: }
Line 3447:
Line 3448: public static List<IpVersion> getExternalSubnetIpVerionList() {
I don't think this is the place for such a method, as it doesn't retrieve
anything from the backend. In fact, I don't think this functionality merits a
method at all.
Line 3449: return Arrays.asList(IpVersion.values());
Line 3450: }
Line 3451:
Line 3452: public static void getNumberOfActiveVmsInCluster(AsyncQuery
aQuery, Guid clusterId) {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/networks/NetworkExternalSubnetListModel.java
Line 56: }
Line 57:
Line 58: NewExternalSubnetModel model = new
NewExternalSubnetModel(this);
Line 59: setWindow(model);
Line 60: initNetworkInModel(model);
Is there any reason to do that here rather than encapsulate the logic in the
constructor of NewExternalSubnetModel? Especially when as far as I can see, it
has to be called whenever the dialog is instantiated.
Line 61: }
Line 62:
Line 63: private void initNetworkInModel(NewExternalSubnetModel model) {
Line 64: model.getNetwork().setItems(Arrays.asList(getEntity()));
Line 135:
Line 136: private void updateActionAvailability() {
Line 137: NetworkView network = getEntity();
Line 138:
Line 139: getNewCommand().setIsExecutionAllowed(network != null &&
network.isExternal());
I thought the subtab wasn't supposed to even be visible if the network wasn't
external?... And without this check you could just inline the getEntity().
Line 140: getRemoveCommand().setIsExecutionAllowed((getSelectedItems()
!= null && getSelectedItems().size() > 0));
Line 141: }
Line 142:
Line 143: @Override
Line 166: super.executeCommand(command);
Line 167:
Line 168: if (command == getNewCommand()) {
Line 169: newSubnet();
Line 170: }
Formatting.
Line 171: else if (command == getRemoveCommand()) {
Line 172: remove();
Line 173: }
Line 174: else if ("Cancel".equals(command.getName())) { //$NON-NLS-1$
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/providers/NewExternalSubnetModel.java
Line 19: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 20: import org.ovirt.engine.ui.uicompat.FrontendActionAsyncResult;
Line 21: import org.ovirt.engine.ui.uicompat.IFrontendActionAsyncCallback;
Line 22:
Line 23: @SuppressWarnings("rawtypes")
Please remove this.
Line 24: public class NewExternalSubnetModel extends Model {
Line 25:
Line 26: private EntityModel<String> name;
Line 27: private EntityModel<String> cidr;
Line 24: public class NewExternalSubnetModel extends Model {
Line 25:
Line 26: private EntityModel<String> name;
Line 27: private EntityModel<String> cidr;
Line 28: private ListModel<NetworkView> network;
Why is network a ListModel? Shouldn't it be just a private Network member, used
perhaps to display the network name in the dialog? Do you want to be able to
create a subnet for a network different from the one chosen in the main tab?
Line 29: private ListModel<IpVersion> ipVersion;
Line 30: private final EntityModel sourceModel;
Line 31: private ExternalSubnet subnet = null;
Line 32:
Line 26: private EntityModel<String> name;
Line 27: private EntityModel<String> cidr;
Line 28: private ListModel<NetworkView> network;
Line 29: private ListModel<IpVersion> ipVersion;
Line 30: private final EntityModel sourceModel;
This should most likely be SearchableListModel rather than EntityModel, so you
can refresh it when a new subnet is added.
Line 31: private ExternalSubnet subnet = null;
Line 32:
Line 33: public NewExternalSubnetModel(EntityModel sourceModel) {
Line 34: this.sourceModel = sourceModel;
Line 27: private EntityModel<String> cidr;
Line 28: private ListModel<NetworkView> network;
Line 29: private ListModel<IpVersion> ipVersion;
Line 30: private final EntityModel sourceModel;
Line 31: private ExternalSubnet subnet = null;
Isn't null the default value?
Line 32:
Line 33: public NewExternalSubnetModel(EntityModel sourceModel) {
Line 34: this.sourceModel = sourceModel;
Line 35:
Line 60: public EntityModel<String> getName() {
Line 61: return name;
Line 62: }
Line 63:
Line 64: public void setName(EntityModel<String> name) {
I see no reason for the setters to be public.
Line 65: this.name = name;
Line 66: }
Line 67:
Line 68: public EntityModel<String> getCidr() {
Line 89: this.ipVersion = ipVersion;
Line 90: }
Line 91:
Line 92: private void onSave()
Line 93: {
Parentheses formatting.
Line 94: if (getProgress() != null)
Line 95: {
Line 96: return;
Line 97: }
Line 90: }
Line 91:
Line 92: private void onSave()
Line 93: {
Line 94: if (getProgress() != null)
I think this isn't required.
Line 95: {
Line 96: return;
Line 97: }
Line 98:
Line 124: true);
Line 125: }
Line 126:
Line 127: public void flush() {
Line 128: if (subnet == null) {
Why not initiate the subnet to new ExternalSubnet() and get rid of this check?
Line 129: subnet = new ExternalSubnet();
Line 130: }
Line 131: subnet.setName(getName().getEntity());
Line 132: Network network = getNetwork().getSelectedItem();
Line 129: subnet = new ExternalSubnet();
Line 130: }
Line 131: subnet.setName(getName().getEntity());
Line 132: Network network = getNetwork().getSelectedItem();
Line 133: subnet.setExternalNetwork(network != null ?
network.getProvidedBy() : null);
Why should a null network be possible?
Line 134: subnet.setCidr(getCidr().getEntity());
Line 135: subnet.setIpVersion(getIpVersion().getSelectedItem());
Line 136: }
Line 137:
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java
Line 463: @DefaultStringValue("VNIC Profiles")
Line 464: String vnicProfilesTitle();
Line 465:
Line 466: @DefaultStringValue("External Subnet")
Line 467: String externalSubnetTitle();
Shouldn't this be "New External Subnet"?
Line 468:
Line 469: @DefaultStringValue("External Subnets")
Line 470: String externalSubnetsTitle();
Line 471:
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/provider/ExternalSubnetPopupPresenterWidget.java
Line 16: super(eventBus, view);
Line 17: }
Line 18:
Line 19: @Override
Line 20: public void init(NewExternalSubnetModel model) {
This isn't required.
Line 21: super.init(model);
Line 22: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/provider/ExternalSubnetPopupView.java
Line 47:
Line 48: @UiField(provided = true)
Line 49: @Path("ipVersion.selectedItem")
Line 50: @WithElementId("ipVersion")
Line 51: protected ListModelListBoxEditor<IpVersion> ipVersionEditor;
Can the GWT binder reach this when it's protected? Either way, there's no
reason why it would be different from the others...
Line 52:
Line 53: @UiField(provided = true)
Line 54: @Path("network.selectedItem")
Line 55: ListModelListBoxEditor<Object> networkEditor;
Line 51: protected ListModelListBoxEditor<IpVersion> ipVersionEditor;
Line 52:
Line 53: @UiField(provided = true)
Line 54: @Path("network.selectedItem")
Line 55: ListModelListBoxEditor<Object> networkEditor;
As I commented on NewExternalSubnetModel, I don't think this should be a list
box, maybe a label.
Line 56:
Line 57: private final Driver driver = GWT.create(Driver.class);
Line 58:
Line 59: @Inject
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/provider/ExternalSubnetPopupView.ui.xml
Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder"
Line 4: xmlns:g="urn:import:com.google.gwt.user.client.ui"
xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog"
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor"
Line 6:
xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"
Line 7:
xmlns:k="urn:import:org.ovirt.engine.ui.common.widget.form.key_value" >
This isn't required, as no widget here is imported from the referenced package.
Line 8:
Line 9: <ui:with field='constants'
type='org.ovirt.engine.ui.webadmin.ApplicationConstants' />
Line 10:
Line 11: <d:SimpleDialogPanel width="350px" height="220px">
Line 5: xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor"
Line 6:
xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"
Line 7:
xmlns:k="urn:import:org.ovirt.engine.ui.common.widget.form.key_value" >
Line 8:
Line 9: <ui:with field='constants'
type='org.ovirt.engine.ui.webadmin.ApplicationConstants' />
This isn't required, as nothing uses 'constants' here.
Line 10:
Line 11: <d:SimpleDialogPanel width="350px" height="220px">
Line 12: <d:content>
Line 13: <g:FlowPanel>
--
To view, visit http://gerrit.ovirt.org/22689
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I7315f9ffe29f72d06deb56dcd637c53fba47b116
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Mike Kolesnik <mkole...@redhat.com>
Gerrit-Reviewer: Lior Vernia <lver...@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
