Lior Vernia 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), consider just saving that into a StoragePool object earlier and referring to that instead. 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().getSystemTree().getSelectedItemChangedEvent().addListener(systemTreeChangedEventListener)). 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 main tab model is probably destructed soon after its view. What I worried about was the systemTree listener, because the system tree stays there and will keep trying to send events to the disk main tab view (which will not exist anymore). So basically, I think instead all is required is CommonModel.getInstance().getSystemTree().getSelectedItemChangedEvent().removeListener(systemTreeChangedEventListener). 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 shorten the name to something like systemTreeListener :) 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
