Kobi Ianko has posted comments on this change. Change subject: webadmin: Adding Quota column to the Disk tab ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/25068/2/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/SystemTreeModel.java: Line 439: dataCenterItem.setApplicationMode(ApplicationMode.VirtOnly); Line 440: dataCenterItem.setTitle(getDataCenters().get(count).getName()); Line 441: dataCenterItem.setEntity(getDataCenters().get(count)); Line 442: dataCentersItem.addChild(dataCenterItem); Line 443: treeItemById.put(getDataCenters().get(count).getId(), dataCenterItem); > Since there are three rows here calling getDataCenters().get(count), consid since it's an arraylist, it will not make any performance impact, but I agree it's nicer. Done Line 444: Line 445: SystemTreeItemModel storagesItem = new SystemTreeItemModel(); Line 446: storagesItem.setType(SystemTreeItemType.Storages); Line 447: storagesItem.setApplicationMode(ApplicationMode.VirtOnly); http://gerrit.ovirt.org/#/c/25068/2/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabDiskPresenter.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/tab/MainTabDiskPresenter.java: Line 78: } Line 79: Line 80: Event systemTreeSelectedItemChangedEvent = Line 81: CommonModel.getInstance().getSystemTree().getSelectedItemChangedEvent(); Line 82: > Unnecessary newline, and I would just inline (i.e. CommonModel.getInstance( removed the new line, I'd like to keep it as 2 lines, it's easier to find problems like NPE that way. Line 83: systemTreeSelectedItemChangedEvent.addListener(systemTreeChangedEventListener); Line 84: Line 85: super.onReveal(); Line 86: getView().handleQuotaColumnVisibility(); Line 88: Line 89: @Override Line 90: protected void onHide() { Line 91: super.onHide(); Line 92: Event entityChangedEvent = getModel().getDiskViewType().getEntityChangedEvent(); > This isn't the listener I was worried about. This should be fine, as the ma oops :) copied the wrong event... will fix it. Done. Line 93: entityChangedEvent.removeListener(getView().getDiskTypeChangedEventListener()); Line 94: } Line 95: Line 96: final IEventListener systemTreeChangedEventListener = new IEventListener() { Line 92: Event entityChangedEvent = getModel().getDiskViewType().getEntityChangedEvent(); Line 93: entityChangedEvent.removeListener(getView().getDiskTypeChangedEventListener()); Line 94: } Line 95: Line 96: final IEventListener systemTreeChangedEventListener = new IEventListener() { > Please move up, usually members appear before methods. Also, I would shorte Done Line 97: @Override Line 98: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 99: getView().handleQuotaColumnVisibility(); Line 100: } -- To view, visit http://gerrit.ovirt.org/25068 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2332da722396a16aca9545b9ef0532ebc84d8d5e Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[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
