Kobi Ianko 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"? cell renderer will skip null, and an empty cell will be displayed 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. A not sure that this is true, cannot assign two quota with the add disk ui screen can you direct me to how to add a secondary quota to a disk? 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 st Done 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 it Done 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. Done 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. Done 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. Done 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 not sure how to simulate the view destruction, once its up it stays up, doesn't it? only show and hide functions are activated I'll take your suggestion anyway cause its cleaner... Done. 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() Done 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. Done 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. Done 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 o Done 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 Done 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. Done 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 || To make it simple we wanna follow this rule: If Quota is enabled for selected DC show column else hide it. For the sake of simplicity we should consider showing the column for LUN too (when Lun radio button is selected). If All is selected we might have a mixture of Image and Lun, which means we must show the column, an empty cell for Image or Lun will indicate that a Quota was enabled for this DC but this disk(row) has no Quota assign to it 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 <[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
