Tomas Jelinek 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..) I have written it for this case: - open the attach disk popup from new/edit VM popup - select something - close it - open it again as a result, nothing has been selected. The model contained the value selected properly, it just was not rendered. And this set to null and back was done to force the rendering. But now, when I wanted to try it again to make sure the exact flow, it seems it is not needed - the content is rendered properly. It is because this case is already handled in AttachDiskModel. So, deleting. 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? It is about which of the two radio buttons on top of the dialog will be selected - the "internal" or the "external". Previously, when you have opened a new disk dialog, the "internal" was selected. This was the "disk.getIsNew() || ..." part. Now, when you open the new disk dialog and have no disk configured (e.g. disk.getDisk == null) the internal has to be selected. Than you press OK. Than you open this same dialog again. Again the new disk dialog will be opened for you, but the disk will already be selected. So we have to check, if this new disk was internal or external. E.g. now you can open the new dialog more times, because the OK does not mean that it will be sent to server directly - it means just to save the current configuration and possibly change it later on. The send to server happens only after pressing the OK on the new/edit VM dialog. 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 ok, generalized and moved 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 behav well I did not realized that also the storage domains are using it :) Thanx for the catch. I have verified and it works (I had to do one little change in SanStorageModel.findSelectedItems() only). 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.. tested and worked 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? yes - for the case when: open the dialog from edit dialog, select something, press ok and than open it again - this makes the selection rendered properly. 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? well, this handles the lun disks - making sure that if the specific disk has been selected, it will get re-selected also when you close and open the dialog again. In create internal disk you don't have this problem and in attach internal - it is handled in VmDiskAttachPopupWidget 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 did not find anything, but moved it there and generalized. 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? to re-select all the prev selected ones. 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? not specific - it is used to load the attachable disks for both attach disk from disks subtab and for the load them from the new/edit dialog. It is shared for this two cases - did you have some other case in mind which could re-use it? 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? it re-selects the already selected things. If you select some disk to attach, than press ok and you will get back to the new/edit VM dialog and than open this same attach disk dialog, this makes sure that your selection will be preserved. 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 similar than the previous but for the inside of the disk - e.g. if you select a disk and for it you select readOnly, than press OK and than re-open this dialog, the disks from server don't have your selection (since it has not been submitted yet). So I replace the selected disk from server by the one one which contains all your selections (for example the readOnly). 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
