Lior Vernia has posted comments on this change. Change subject: webadmin: Adding Quota column to the Disk tab ......................................................................
Patch Set 1: (15 comments) http://gerrit.ovirt.org/#/c/25068/1/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/disks/DisksViewColumns.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/disks/DisksViewColumns.java: Line 53: public static final TextColumnWithTooltip<Disk> qoutaColumn = new TextColumnWithTooltip<Disk>() { Line 54: @Override Line 55: public String getValue(Disk object) { Line 56: Line 57: String value = null; Is null rendered as the empty string (which would be great), or as "null"? Line 58: if (object.getDiskStorageType() == DiskStorageType.IMAGE) { Line 59: DiskImage diskImage = (DiskImage) object; Line 60: value = diskImage.getQuotaName(); Line 61: } Line 56: Line 57: String value = null; Line 58: if (object.getDiskStorageType() == DiskStorageType.IMAGE) { Line 59: DiskImage diskImage = (DiskImage) object; Line 60: value = diskImage.getQuotaName(); I've noticed that a disk may have more than one quota associated with it. Are you sure you want to only display the first one? Line 61: } Line 62: return value; Line 63: } Line 64: }; http://gerrit.ovirt.org/#/c/25068/1/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 446: storagesItem.setApplicationMode(ApplicationMode.VirtOnly); Line 447: storagesItem.setTitle(ConstantsManager.getInstance().getConstants().storageTitle()); Line 448: storagesItem.setEntity(getDataCenters().get(count)); Line 449: dataCenterItem.addChild(storagesItem); Line 450: treeItemById.put(getDataCenters().get(count).getId(), storagesItem); I think you may have wanted to put dataCenterItem in the map rather than storagesItem, in which case I would correct this and move it under line 442. Line 451: Line 452: if (storages != null && storages.size() > 0) Line 453: { Line 454: // sort by name first http://gerrit.ovirt.org/#/c/25068/1/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 773: // Update the Quota at the corresponding DC object at the system tree. Line 774: // The DC Quota value from the tree is used at MainTabDiskView. Line 775: SystemTreeItemModel itemModel = CommonModel.getInstance().getSystemTree().getItemById(dataCenter.getId()); Line 776: StoragePool storagePool = (StoragePool) itemModel.getEntity(); Line 777: storagePool.setQuotaEnforcementType(dataCenter.getQuotaEnforcementType()); Instead of updating this specific field of the entity stored in the tree item, I would just replace the whole entity, i.e. itemModel.setEntity(dataCenter). Does that cause any issues? Line 778: Line 779: // Otherwise use async action in order to close dialog immediately. Line 780: Frontend.getInstance().runMultipleAction(VdcActionType.UpdateStoragePool, Line 781: new ArrayList<VdcActionParametersBase>(Arrays.asList( http://gerrit.ovirt.org/#/c/25068/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/ApplicationConstants.java: Line 2680: // Disk Line 2681: @DefaultStringValue("ID") Line 2682: String idDisk(); Line 2683: Line 2684: @DefaultStringValue("Qouta") Typo. Line 2685: String qoutaDisk(); Line 2686: Line 2687: @DefaultStringValue("Volume Format") Line 2688: String volumeFormatDisk(); Line 2681: @DefaultStringValue("ID") Line 2682: String idDisk(); Line 2683: Line 2684: @DefaultStringValue("Qouta") Line 2685: String qoutaDisk(); Typo. Line 2686: Line 2687: @DefaultStringValue("Volume Format") Line 2688: String volumeFormatDisk(); Line 2689: http://gerrit.ovirt.org/#/c/25068/1/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 43: Line 44: public interface ViewDef extends AbstractMainTabWithDetailsPresenter.ViewDef<Disk> { Line 45: Line 46: IEventListener getDiskTypeChangedEventListener(); Line 47: IEventListener systemTreeChangedEventListener(); Unneeded in my opinion, see bottom comment. Line 48: Line 49: } Line 50: Line 51: @TabInfo(container = MainTabPanelPresenter.class) Line 79: Line 80: Event systemTreeSelectedItemChangedEvent = Line 81: CommonModel.getInstance().getSystemTree().getSelectedItemChangedEvent(); Line 82: if (!systemTreeSelectedItemChangedEvent.getListeners().contains(getView().systemTreeChangedEventListener())) { Line 83: systemTreeSelectedItemChangedEvent.addListener(getView().systemTreeChangedEventListener()); This looks unsafe to me; when this view is destroyed, SystemTreeModel still exists and has the listener registered for events. Then when a different node is picked, I'll expect some exception to be thrown. Could you verify? I would also override onHide() and there remove this listener. This will also render the check whether the listeners contain this listener unnecessary - it'll never be there onReveal(), because it'll always be removed onHide(). Line 84: } Line 85: Line 86: super.onReveal(); Line 87: ((MainTabDiskView) getView()).handleQuotaColumnVisibility(); Line 83: systemTreeSelectedItemChangedEvent.addListener(getView().systemTreeChangedEventListener()); Line 84: } Line 85: Line 86: super.onReveal(); Line 87: ((MainTabDiskView) getView()).handleQuotaColumnVisibility(); A cleaner way to do this would be to include handleQuotaColumnVisibility() in the ViewDef interface. Then you wouldn't really need systemTreeChangedEventListener() - it could be defined in the presenter, and just call handleQuotaColumnVisibility(). Line 88: } http://gerrit.ovirt.org/#/c/25068/1/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDiskView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabDiskView.java: Line 81: public IEventListener getDiskTypeChangedEventListener() { Line 82: return diskTypeChangedEventListener; Line 83: } Line 84: Line 85: final IEventListener systemTreeChangedEventListener = new IEventListener() { I'd move this to the presenter. Line 86: @Override Line 87: public void eventRaised(Event ev, Object sender, EventArgs args) { Line 88: handleQuotaColumnVisibility(); Line 89: } Line 89: } Line 90: }; Line 91: Line 92: @Override Line 93: public IEventListener systemTreeChangedEventListener() { Not needed in my opinion, see comment in presenter. Line 94: return systemTreeChangedEventListener; Line 95: } Line 96: Line 97: private boolean isQuotaVisible; Line 93: public IEventListener systemTreeChangedEventListener() { Line 94: return systemTreeChangedEventListener; Line 95: } Line 96: Line 97: private boolean isQuotaVisible; It's probably preferable to move the member further up, together with the other members. Line 98: public void handleQuotaColumnVisibility() { Line 99: isQuotaVisible = false; Line 100: SystemTreeItemModel treeItem = Line 101: (SystemTreeItemModel) CommonModel.getInstance().getSystemTree().getSelectedItem(); Line 99: isQuotaVisible = false; Line 100: SystemTreeItemModel treeItem = Line 101: (SystemTreeItemModel) CommonModel.getInstance().getSystemTree().getSelectedItem(); Line 102: if (treeItem != null Line 103: && SystemTreeItemType.DataCenter.equals(treeItem.getType())) { As far as I know, enums can be used with the ==/!= operators. It's a matter of style, but I prefer them (since an instance of an enum is really the same referenced entity as the enum constant). However, you can do whatever you like, either is fine by me. Line 104: StoragePool storagePool = (StoragePool) treeItem.getEntity(); Line 105: if (!QuotaEnforcementTypeEnum.DISABLED.equals(storagePool.getQuotaEnforcementType())) { Line 106: isQuotaVisible = true; Line 107: } Line 101: (SystemTreeItemModel) CommonModel.getInstance().getSystemTree().getSelectedItem(); Line 102: if (treeItem != null Line 103: && SystemTreeItemType.DataCenter.equals(treeItem.getType())) { Line 104: StoragePool storagePool = (StoragePool) treeItem.getEntity(); Line 105: if (!QuotaEnforcementTypeEnum.DISABLED.equals(storagePool.getQuotaEnforcementType())) { Same comment. Line 106: isQuotaVisible = true; Line 107: } Line 108: } Line 109: onDiskViewTypeChanged(); Line 180: DisksViewColumns.lunProductIdColumn, constants.productIdSanStorage(), luns, Line 181: "100px"); //$NON-NLS-1$ Line 182: Line 183: getTable().ensureColumnPresent( Line 184: DisksViewColumns.qoutaColumn, constants.qoutaDisk(), (all || images || luns) && isQuotaVisible, "120px"); //$NON-NLS-1$ If I correctly understood the logic in DisksViewColumns, (all || images || luns) isn't consistent with it and should be replaced by (all || images). Which leads me to another question: what then would you want to present in "all" for LUNs? Just an empty entry, or some notion of non-relevant? If non-relevant, then you can get rid of all the system tree logic... Line 185: Line 186: getTable().ensureColumnPresent( Line 187: DisksViewColumns.descriptionColumn, constants.descriptionDisk(), all || images || luns, Line 188: "90px"); //$NON-NLS-1$ -- 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: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Kobi Ianko <k...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
