Daniel Erez has posted comments on this change. Change subject: webadmin: Changes in disk popup needed to reuse in VM popup ......................................................................
Patch Set 11: (12 comments) http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskAttachPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskAttachPopupWidget.java: Line 424: Line 425: diskTable.setVisible(true); Line 426: ListModel<EntityModel<DiskModel>> disksListModel = disk.getAttachableDisksMap().get(disk.getDiskStorageType().getEntity()); Line 427: Line 428: // The setting of null and setting back the original value is done to force rendering the selected items. in which scenario is it needed? (i.e. what behavior it fixes..) Line 429: // The reason it does not work is that the HasDataListModelEditorAdapter.edit() only renders it after the Line 430: // change events are fired Line 431: EntityModel<DiskModel> selectedModel = disksListModel.getSelectedItem(); Line 432: List<EntityModel<DiskModel>> selectedModels = disksListModel.getSelectedItems(); http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmDiskPopupWidget.java: Line 329: }); Line 330: Line 331: diskTypePanel.addRadioButton( Line 332: constants.internalDisk(), Line 333: disk.getDisk() == null || disk.getDisk().getDiskStorageType() == DiskStorageType.IMAGE, why is this change needed? Line 334: disk.getIsNew(), Line 335: new ClickHandler() { Line 336: @Override Line 337: public void onClick(ClickEvent event) { Line 413: revealStorageView(disk); Line 414: revealDiskPanel(disk); Line 415: } Line 416: Line 417: private <T extends IStorageModel> T byType(Collection<IStorageModel> models, Class<T> specific) { seems worth moving to Linq Line 418: for (IStorageModel model : models) { Line 419: if (model.getClass().equals(specific)) { Line 420: return (T) model; Line 421: } http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/storage/SanStorageLunToTargetList.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/storage/SanStorageLunToTargetList.java: Line 126: Line 127: if (!lunModel.getIsSelected()) { Line 128: table.getSelectionModel().setSelected(lunModel, false); Line 129: } else { Line 130: table.getSelectionModel().setSelected(lunModel, true); is it done for edit mode? have you verified the storage domain dialog behavior after this change? (as it's common to storage and disk dialogs). Line 131: } Line 132: table.redraw(); Line 133: } Line 134: } Line 284: } Line 285: Line 286: table.setRowData(items); Line 287: Line 288: Object selectedItem = leafModel.getSelectedItem(); again, not sure these changes play well with storage dialog.. Line 289: leafModel.setSelectedItem(null); Line 290: table.asEditor().edit(leafModel); Line 291: leafModel.setSelectedItem(selectedItem); Line 292: http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/storage/SanStorageTargetToLunList.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/storage/SanStorageTargetToLunList.java: Line 242: }, constants.serialSanStorage(), "120px"); //$NON-NLS-1$ Line 243: Line 244: table.setRowData(items); Line 245: Object selectedItem = leafModel.getSelectedItem(); Line 246: leafModel.setSelectedItem(null); is it needed just for refresh purposes? Line 247: table.asEditor().edit(leafModel); Line 248: leafModel.setSelectedItem(selectedItem); Line 249: Line 250: table.setWidth("100%", true); //$NON-NLS-1$ http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/SanStorageModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/storage/SanStorageModel.java: Line 144: public void onSuccess(Object target, Object returnValue) { Line 145: SanStorageModel model = (SanStorageModel) target; Line 146: VdcQueryReturnValue response = (VdcQueryReturnValue) returnValue; Line 147: if (response.getSucceeded()) { Line 148: model.applyData((ArrayList<LUNs>) response.getReturnValue(), false, prevSelected); is it relevant only for lun disk? Line 149: model.setGetLUNsFailure(""); //$NON-NLS-1$ Line 150: } Line 151: else { Line 152: model.setGetLUNsFailure( Line 159: new GetDeviceListQueryParameters(host.getId(), getType()), Line 160: asyncQuery); Line 161: } Line 162: Line 163: private List<EntityModel<?>> findSelectedItems() { iirc, there's something similar in Linq Line 164: List<EntityModel<?>> res = new ArrayList<>(); Line 165: Line 166: for (EntityModel<?> item : (Collection<EntityModel<?>>) getItems()) { Line 167: if (item.getIsSelected()) { Line 251: lunModel.setSize(a.getDeviceSize()); Line 252: lunModel.setIsAccessible(a.getAccessible()); Line 253: lunModel.setStatus(a.getStatus()); Line 254: lunModel.setIsIncluded(isIncluded); Line 255: lunModel.setIsSelected(containsLun(lunModel, selectedItems, isIncluded)); why is it needed? Line 256: lunModel.setEntity(a); Line 257: Line 258: // Add LunModel Line 259: newItems.add(lunModel); http://gerrit.ovirt.org/#/c/36061/11/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AttachDiskModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AttachDiskModel.java: Line 65: new AsyncQuery(this, new GetDisksCallback(DiskStorageType.LUN, prevSelectedDisk), Line 66: getHash()), null, getVm().getId()); Line 67: } Line 68: Line 69: public void loadAttachableDisks(int os, Version compatibilityVersion, final Disk prevSelectedDisk) { is this logic specific for instance images? perhaps it can be extracted? Line 70: // Get internal attachable disks Line 71: AsyncDataProvider.getInstance().getFilteredAttachableDisks( Line 72: new AsyncQuery(this, new GetDisksCallback(DiskStorageType.IMAGE, prevSelectedDisk), Line 73: getHash()), getVm().getStoragePoolId(), getVm().getId(), os, compatibilityVersion); Line 100: getAttachableDisksMap().get(diskStorageType).setItems(entities, selectedOrNull(entities, prevSelectedDisk, diskStorageType)); Line 101: } Line 102: } Line 103: Line 104: private EntityModel<DiskModel> selectedOrNull(List<EntityModel<DiskModel>> list, Disk prevSelected, DiskStorageType diskStorageType) { not sure i follow... why is it needed? Line 105: if (prevSelected == null) { Line 106: return null; Line 107: } Line 108: Line 118: Line 119: return null; Line 120: } Line 121: Line 122: private List<Disk> replaceIfSelected(List<Disk> disksFromServer, Disk prevSelected, DiskStorageType diskStorageType) { same Line 123: if (prevSelected == null) { Line 124: return disksFromServer; Line 125: } Line 126: -- To view, visit http://gerrit.ovirt.org/36061 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I52c5d5c7b83dd53cea3a9419656015e6a51bd179 Gerrit-PatchSet: 11 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Ala Hino <[email protected]> Gerrit-Reviewer: Amit Aviram <[email protected]> Gerrit-Reviewer: Candace Sheremeta <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Freddy Rolland <[email protected]> Gerrit-Reviewer: Idan Shaby <[email protected]> Gerrit-Reviewer: Tal Nisan <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[email protected]> Gerrit-Reviewer: Vered Volansky <[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
