Alona Kaplan 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 364: macPools.add(1, getEntity()); I don't see any reason to add it as the second element of the list. IMO, it should be added to its natural place according to the SharedMacPoolComparator. 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 225: ArrayList Please use interface List in the left side Line 226: Collections 1. As I wrote in DataCenterListModel. I think the items should always be ordered. In this case you can use SortedListModel for macPoolList and you won't need this code here. 2. This code is not related to the current patch and should be part of the previous patch. 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? 2. Should be part of the previous patch. 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 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 just the id. I know that for now besides the id there is no difference, but maybe in the future the engine will fix the overlapping ranges or there will be some other difference. The most correct data is the MacPool in the result. 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 20: UiCommandButton It is enough for the return value to be HasUiCommandClickHandlers -- 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: [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
