Tomas Jelinek has posted comments on this change. Change subject: frontend: created instance image widget ......................................................................
Patch Set 7: (7 comments) http://gerrit.ovirt.org/#/c/36063/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AddRemoveRowWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/AddRemoveRowWidget.java: Line 230: new ClickHandler() { Line 231: Line 232: @Override Line 233: public void onClick(ClickEvent event) { Line 234: if (vetoRemoveWidget(item, value, widget)) { > nice naming :) heh, yeah, better fitting to our culture would be "-1removeWidget()" but it was not a valid method name :D Line 235: return; Line 236: } Line 237: Line 238: doRemoveItem(item, value, widget); http://gerrit.ovirt.org/#/c/36063/7/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/instanceimages/InstanceImageLineEditor.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/instanceimages/InstanceImageLineEditor.java: Line 72: createEditButton.setCommand(model.getCreateEditCommand()); Line 73: attachButton.setCommand(model.getAttachCommand()); Line 74: Line 75: // memory leak maybe? Line 76: createEditButton.addClickHandler(new ClickHandler() { > maybe you can move it to the view's presenter if not too complicated? not that simple - it does not have a presenter - the closest presenter to this one is the AbstractVmBasedPopupPresenterWidget - but the buttons here are created and removed on the fly and the presenter widget does not know about them. The same approach is used in for example. AddRemoveRowWidget or ViewRadioGroup (e.g. if you are too fast from the presenter). But removing the comment - it was a note to myself and I have forgotten it there. Line 77: @Override Line 78: public void onClick(ClickEvent event) { Line 79: createEditButton.getCommand().execute(); Line 80: } Line 123: } Line 124: Line 125: @Override Line 126: public void setEnabled(boolean b) { Line 127: > why prevent it? right, done Line 128: } Line 129: Line 130: @Override Line 131: public HandlerRegistration addValueChangeHandler(ValueChangeHandler<InstanceImageLineModel> valueChangeHandler) { http://gerrit.ovirt.org/#/c/36063/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/InstanceImageLineModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/InstanceImageLineModel.java: Line 28: Line 29: // if the disk already exists in the engine or is just created here but not yet submitted Line 30: private boolean diskExists; Line 31: Line 32: // a hack because the diskModel from inside of this method might not have been inited when the onSave is called > suggestion - maybe you can just create the models beforehand? (I mean as fi not all of them are needed - if you create a new disk, only the NewDiskModel is created and not the others. And they are not re-created all the time - look at showPreviouslyShownDialog() which is reusing the already created ones. But I have removed the tmpDiskModel - it was really not needed... Line 33: private AbstractDiskModel tmpDiskModel; Line 34: Line 35: private EntityModel<String> name = new EntityModel<>(); Line 36: Line 70: String diskName = disk.getDiskAlias(); Line 71: String size = Long.toString(disk.getSize()); Line 72: Line 73: if (disk.getDiskStorageType() == Disk.DiskStorageType.IMAGE) { Line 74: size = Long.toString(disk.getSize() / (1024 * 1024 * 1024)); > you can use 'DiskImageBase -> getSizeInGigabytes()' instead ok, but it required to move the getSizeInGigabytes() from DiskImageBase to Disk - added a new patch before this. Line 75: } Line 76: Line 77: String type; Line 78: if (diskExists) { Line 306: return null; Line 307: } Line 308: Line 309: Disk disk = diskModel.getDisk(); Line 310: if (disk == null) { > just check against 'disk.getDiskStorageType()' Done Line 311: disk = diskModel.getDiskImage(); Line 312: } Line 313: Line 314: if (disk == null) { http://gerrit.ovirt.org/#/c/36063/7/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/InstanceImagesModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/InstanceImagesModel.java: Line 80: callback.removeApproved(false); Line 81: } Line 82: Line 83: private void hideRemoveDiskAndShowEditVm() { Line 84: getParentListModel().setWindow(null); > needed just to invoke the event? It is a regular setWindow(null) - e.g. hides the currently shown dialog which is the create/attach disk dialog. The next setWindow(getUnitVmModel()) shows the new/edit VM dialog again. It is needed because one dialog (create/attach disk) is opened from a different dialog (new/edit vm). Other option would be to abuse the setConfirmWindow() to have both windows visible, but I don't really like it - the setConfirmWindow() is not meant for showing normal windows. Line 85: getParentListModel().setWindow(getUnitVmModel()); Line 86: } Line 87: Line 88: @Override -- To view, visit http://gerrit.ovirt.org/36063 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4bcaa1e40841aa12e64708ec82afdb9daec9fb7f Gerrit-PatchSet: 7 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Amit Aviram <aavi...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
