Ayal Baron has posted comments on this change.
Change subject: webadmin: Import VM dialog redesign (#845947)
......................................................................
Patch Set 2: (14 inline comments)
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ImportVmModel.java
Line 148: super.setSelectedItem(value);
Line 149: OnEntityChanged();
Line 150: }
Line 151:
Line 152: public boolean hasVmInSystem() {
need to get rid of this.
Line 153: if (alreadyInSystemVmMap == null ||
alreadyInSystemVmMap.size() == 0) {
Line 154: return false;
Line 155: }
Line 156: return true;
Line 173: @Override
Line 174: public void OnSuccess(Object model, Object returnValue) {
Line 175: ArrayList<storage_pool> pools =
(ArrayList<storage_pool>) returnValue;
Line 176: storage_pool dataCenter = null;
Line 177: if (pools != null && pools.size() > 0) {
I meant == 1
Line 178: dataCenter = pools.get(0);
Line 179: } else {
Line 180: //TODO internal client ERROR
Line 181: }
Line 270: setMessage(isDefaultStorageApplicableForAllDisks ? "" :
Line 271:
ConstantsManager.getInstance().getConstants().importNotApplicableForDefaultStorage());
Line 272: }
Line 273:
Line 274: private void checkForProblematicItems() {
This method name is meaningless. Need to give it a proper name that actually
indicates what this method does.
in this case:
checkDestFormatCompatibility
Line 275: for (Object item : getItems()) {
Line 276: VM vm = (VM) item;
Line 277: if (vm.getDiskMap() != null) {
Line 278: for (Map.Entry<Guid, Disk> pair :
vm.getDiskMap().entrySet()) {
Line 282: && getDiskImportData(disk.getId()) != null
Line 283: &&
(getDiskImportData(disk.getId()).getSelectedStorageDomain()
Line 284: .getstorage_type() ==
StorageType.ISCSI ||
Line 285:
getDiskImportData(disk.getId()).getSelectedStorageDomain()
Line 286: .getstorage_type() ==
StorageType.FCP)) {
getstorage_type supports isBlock
should be used here
Line 287: getProblematicItems().add(vm);
Line 288: break;
Line 289: }
Line 290: }
Line 283: &&
(getDiskImportData(disk.getId()).getSelectedStorageDomain()
Line 284: .getstorage_type() ==
StorageType.ISCSI ||
Line 285:
getDiskImportData(disk.getId()).getSelectedStorageDomain()
Line 286: .getstorage_type() ==
StorageType.FCP)) {
Line 287: getProblematicItems().add(vm);
need to convert name to: disksToConvert
Line 288: break;
Line 289: }
Line 290: }
Line 291: }
Line 290: }
Line 291: }
Line 292: }
Line 293: if (getProblematicItems().size() > 0) {
Line 294: if (getProblematicItems().size() ==
Linq.Count(getItems())) {
I don't see any reason for this added complexity, you can get rid of this 'if'
Line 295: // All items are problematic.
Line 296: getCollapseSnapshots().setEntity(true);
Line 297: getCollapseSnapshots().setIsChangable(false);
Line 298:
Line 311:
Line 312: private void initDisksStorageDomainsList() {
Line 313: for (Object item : getItems()) {
Line 314: VM vm = (VM) item;
Line 315: if (!NGuid.Empty.equals(vm.getvmt_guid())) {
this is needlessly confusing.
order should be is object of interest equals Empty
i.e.
vm.getvmt_guid().equals(NGuid.Empty)
Line 316: templateDiskMap.put(vm.getvmt_guid(), new
ArrayList<Disk>(vm.getDiskMap().values()));
Line 317: } else {
Line 318: for (Disk disk : vm.getDiskMap().values()) {
Line 319: DiskImage diskImage = (DiskImage) disk;
Line 312: private void initDisksStorageDomainsList() {
Line 313: for (Object item : getItems()) {
Line 314: VM vm = (VM) item;
Line 315: if (!NGuid.Empty.equals(vm.getvmt_guid())) {
Line 316: templateDiskMap.put(vm.getvmt_guid(), new
ArrayList<Disk>(vm.getDiskMap().values()));
what happens if vm.getvmt_guid is null?
Line 317: } else {
Line 318: for (Disk disk : vm.getDiskMap().values()) {
Line 319: DiskImage diskImage = (DiskImage) disk;
Line 320: setDiskImportData(diskImage.getId(),
Line 317: } else {
Line 318: for (Disk disk : vm.getDiskMap().values()) {
Line 319: DiskImage diskImage = (DiskImage) disk;
Line 320: setDiskImportData(diskImage.getId(),
Line 321: filteredStorageDomain,
diskImage.getvolume_type());
s/filteredStorageDomain/filteredStorageDomains/
Line 322: }
Line 323: }
Line 324: }
Line 325: if (!templateDiskMap.isEmpty()) {
Line 490: List<VM> vmList =
Line 491: (List<VM>) ((VdcQueryReturnValue)
returnValue).getReturnValue();
Line 492: alreadyInSystemVmMap = new HashMap<Guid,
VM>();
Line 493: for (VM vm : vmList) {
Line 494: alreadyInSystemVmMap.put(vm.getId(), vm);
This should be removed (no need for this column as we agreed)
Line 495: }
Line 496:
Line 497: setSuperItems(value);
Line 498: }
Line 518: public boolean isObjectInSetup(Object vm) {
Line 519: if (alreadyInSystemVmMap == null) {
Line 520: return false;
Line 521: }
Line 522: return alreadyInSystemVmMap.containsKey(((VM) vm).getId());
need to get rid of this
Line 523: }
Line 524:
Line 525: public boolean isQuotaEnabled() {
Line 526: return getStoragePool() != null
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmImportDiskListModel.java
Line 51:
Line 52: public void setDiskStorageMap(HashMap<Guid, ArrayList<Guid>> value)
Line 53: {
Line 54: diskStorageMap = value;
Line 55: raiseDiskEvent();
raiseDiskEvent sounds like an exception.
should be something else (the name)
Line 56: }
Line 57:
Line 58: public void raiseDiskEvent() {
Line 59: OnPropertyChanged(new
PropertyChangedEventArgs("DiskStorageMap")); //$NON-NLS-1$
....................................................
File
frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/Constants.java
Line 1481:
Line 1482: @DefaultStringValue("Unassigned Logical Networks panel")
Line 1483: String unassignedLogicalNetworksPanel();
Line 1484:
Line 1485: @DefaultStringValue("Some Templates' disks which are missing.
Suggested solutions: "
I don't have time right now, but need to revise this message. Let's discuss.
Line 1486: + "1. Preserving Template-Based structure - "
Line 1487: + "Make sure the relevant Templates exist on the
relevant Data-Center and Import the VMs again."
Line 1488: + "2. Using non-Template-Based structure (less optimal
storage-wise) - "
Line 1489: + "Export the VMs again using the Collapse Snapshots
option.")
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/storage/backup/ImportVmPopupView.java
Line 177: @Override
Line 178: public void onSelection(SelectionEvent<Integer>
event) {
Line 179: if (object != null) {
Line 180:
object.setActiveDetailModel(object.getDetailModels().get(event.getSelectedItem()));
Line 181: if (event.getSelectedItem() == 2) {
== 2?
1. This should be named
2. Why disks by default? (not that I mind, but still)
Line 182: generalView.setMainTabSelectedItem((VM)
object.getSelectedItem());
Line 183: }
Line 184: }
Line 185: }
--
To view, visit http://gerrit.ovirt.org/7313
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1b6551b235c66e6a32db0f8842cd8e410d6e0318
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Gilad Chaplik <[email protected]>
Gerrit-Reviewer: Greg Padgett <[email protected]>
Gerrit-Reviewer: Haim Ateya <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
