Lior Vernia has posted comments on this change. Change subject: webadmin: Add button for MAC pool creation in DC dialog ......................................................................
Patch Set 5: (7 comments) http://gerrit.ovirt.org/#/c/29230/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterListModel.java: Line 360: @Override Line 361: protected void postSave(Guid macPoolId) { Line 362: super.postSave(macPoolId); Line 363: ArrayList<MacPool> macPools = new ArrayList<MacPool>(dcModel.getMacPoolListModel().getItems()); Line 364: macPools.add(1, getEntity()); > I don't see any reason to add it as the second element of the list. IMO, it Done Line 365: dcModel.getMacPoolListModel().setItems(macPools); Line 366: dcModel.getMacPoolListModel().setSelectedItem(getEntity()); Line 367: } Line 368: }; http://gerrit.ovirt.org/#/c/29230/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/datacenters/DataCenterModel.java: Line 221: new VdcQueryParametersBase(), Line 222: new AsyncQuery(new INewAsyncCallback() { Line 223: @Override Line 224: public void onSuccess(Object model, Object returnValue) { Line 225: ArrayList<MacPool> allMacPools = new ArrayList<MacPool>((Collection<MacPool>) ((VdcQueryReturnValue) returnValue).getReturnValue()); > Please use interface List in the left side Note that this doesn't matter when it's just a local variable, this is important mainly when passing around collections between different classes/methods. Not relevant anyway due to your next comment. Line 226: Collections.sort(allMacPools, new Linq.SharedMacPoolComparator()); Line 227: getMacPoolListModel().setItems(allMacPools); Line 228: initSelectedMacPool(); Line 229: stopProgress(); Line 222: new AsyncQuery(new INewAsyncCallback() { Line 223: @Override Line 224: public void onSuccess(Object model, Object returnValue) { Line 225: ArrayList<MacPool> allMacPools = new ArrayList<MacPool>((Collection<MacPool>) ((VdcQueryReturnValue) returnValue).getReturnValue()); Line 226: Collections.sort(allMacPools, new Linq.SharedMacPoolComparator()); > 1. As I wrote in DataCenterListModel. I think the items should always be or 1. Done, though SortedListModel is broken at the moment (on purpose, see http://gerrit.ovirt.org/#/c/28392/), but should be fixed in the future. 2. Done. Line 227: getMacPoolListModel().setItems(allMacPools); Line 228: initSelectedMacPool(); Line 229: stopProgress(); Line 230: } Line 343: Collection<MacPool> allMacPools = getMacPoolListModel().getItems(); Line 344: StoragePool dc = getEntity(); Line 345: if (allMacPools != null && dc != null) { Line 346: Guid macPoolId = dc.getMacPoolId(); Line 347: if (macPoolId != null) { > 1. And what if it it is null? Shouldn't you choose the default pool? 1. Actually, shouldn't be null. Fixed. 2. Done. Line 348: for (MacPool macPool : allMacPools) { Line 349: if (macPoolId.equals(macPool.getId())) { Line 350: getMacPoolListModel().setSelectedItem(macPool); Line 351: break; Line 352: } Line 353: } Line 354: } Line 355: } Line 356: } > please format Already formatted?... Line 357: Line 358: public boolean validate() Line 359: { Line 360: getName().validateEntity(new IValidation[] { http://gerrit.ovirt.org/#/c/29230/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/NewSharedMacPoolModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/NewSharedMacPoolModel.java: Line 17: Line 18: @Override Line 19: protected void postSave(Guid macPoolId) { Line 20: super.postSave(macPoolId); Line 21: getEntity().setId(macPoolId); > I think you should set the whole entity returned from the engine and not ju The engine returns just the ID. Also, note that I moved this down to the anonymous subclass in DataCenterListModel, as it's the only one that needs this functionality. Line 22: } Line 23: http://gerrit.ovirt.org/#/c/29230/5/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/datacenter/DataCenterPopupPresenterWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/datacenter/DataCenterPopupPresenterWidget.java: Line 16: public class DataCenterPopupPresenterWidget extends AbstractModelBoundPopupPresenterWidget<DataCenterModel, DataCenterPopupPresenterWidget.ViewDef> { Line 17: Line 18: public interface ViewDef extends AbstractModelBoundPopupPresenterWidget.ViewDef<DataCenterModel> { Line 19: void updateMacPool(MacPoolModel macPoolModel); Line 20: UiCommandButton getMacPoolButton(); > It is enough for the return value to be HasUiCommandClickHandlers Done Line 21: } Line 22: Line 23: @Inject Line 24: public DataCenterPopupPresenterWidget(EventBus eventBus, ViewDef view) { -- To view, visit http://gerrit.ovirt.org/29230 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I585ab2d39bb5e0f88285d16e69eeae1c818391bc Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <[email protected]> Gerrit-Reviewer: Alona Kaplan <[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
