Tomas Jelinek has posted comments on this change. Change subject: userportal, webadmin: VM console key management ......................................................................
Patch Set 20: (9 comments) https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/PublicKeyPopupWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/PublicKeyPopupWidget.ui.xml: Line 3: <ui:UiBinder xmlns:ui="urn:ui:com.google.gwt.uibinder" xmlns:g="urn:import:com.google.gwt.user.client.ui" Line 4: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"> Line 5: Line 6: <ui:style type="org.ovirt.engine.ui.common.widget.uicommon.popup.vm.PublicKeyPopupWidget.Style"> Line 7: .content { why do you have all of this empty styles here? Id say you could just delete all this since it is empty anyway... Than do the corresponding deletes also inside the PublicKeyPopupWidget.java as well Line 8: Line 9: } Line 10: Line 11: .publicKeyEditor { Line 20: Line 21: } Line 22: </ui:style> Line 23: Line 24: <g:VerticalPanel verticalAlignment="ALIGN_MIDDLE" addStyleNames="{style.content}"> please use FlowPanel instead - VerticalPanel renders as <table> and we are trying to avoid it. And especially since you have only one widget in it it should look all the same. Line 25: <ge:StringEntityModelTextAreaEditor ui:field="publicKeyEditor" addStyleNames="{style.publicKeyEditor}" /> Line 26: </g:VerticalPanel> Line 27: https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/PublicKeyModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/PublicKeyModel.java: Line 1: package org.ovirt.engine.ui.uicommonweb.models; Line 2: Line 3: public class PublicKeyModel extends Model { Line 4: Line 5: EntityModel<String> textInput; private EntityModel... Line 6: Line 7: public EntityModel<String> getTextInput() { Line 8: return textInput; Line 9: } https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java: Line 545: model.setHelpTag(HelpTag.edit_public_key); Line 546: Line 547: model.getCommands().add(UICommand.createDefaultOkUiCommand("OnSetConsoleKey", this)); //$NON-NLS-1$ Line 548: model.getCommands().add(UICommand.createCancelUiCommand("Cancel", this)); //$NON-NLS-1$ Line 549: here you should call "model.startProgress(null)" (to show the loading indicator inside the window). Line 550: AsyncDataProvider.getInstance().getUserProfile(new AsyncQuery(model, Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 550: AsyncDataProvider.getInstance().getUserProfile(new AsyncQuery(model, Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 554: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); I'd say this is NPE vulnerable since the profile could be null Line 555: model.getTextInput().setEntity(profile.getSshPublicKey()); Line 556: } Line 557: })); Line 558: } Line 551: new INewAsyncCallback() { Line 552: @Override Line 553: public void onSuccess(Object target, Object returnValue) { Line 554: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 555: model.getTextInput().setEntity(profile.getSshPublicKey()); and here "model.stopProgress()" Line 556: } Line 557: })); Line 558: } Line 559: Line 559: Line 560: private void onSetConsoleKey() { Line 561: Line 562: final PublicKeyModel model = (PublicKeyModel) getWindow(); Line 563: here should be a check if the something is not going on in the model on background, e.g.: if (model.getProgress() != null) { return; } Line 564: final String publicKey = model.getTextInput().getEntity(); Line 565: Line 566: model.startProgress(null); Line 567: Line 569: new INewAsyncCallback() { Line 570: @Override Line 571: public void onSuccess(Object target, Object returnValue) { Line 572: UserProfile profile = ((VdcQueryReturnValue) returnValue).getReturnValue(); Line 573: VdcActionType action = (profile != null) ?VdcActionType.UpdateUserProfile :VdcActionType.AddUserProfile; just a nit - please add space between "?" and "VdcAc..". The same with ":" Line 574: Line 575: Frontend.getInstance().runAction(action, new UserProfileParameters(publicKey), Line 576: new IFrontendActionAsyncCallback() { Line 577: @Override https://gerrit.ovirt.org/#/c/35810/20/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java: Line 1751: } Line 1752: }, model); Line 1753: } Line 1754: Line 1755: private void editConsoleKey() { well, this is copy / pasted from UserPortalListModel. Please move this logic inside the PublicKeyModel so both here and in UserPortalListModel you can just call setWindow(new PublicKeyModel(this)); and it can set itself up, listen on itself and than destroy itself (that is why it needs the "this" to be able to call "cancel()" on it. Line 1756: Line 1757: final PublicKeyModel model = new PublicKeyModel(); Line 1758: Line 1759: setWindow(model); -- To view, visit https://gerrit.ovirt.org/35810 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id2364cafc687ba6dee2504322234067ff98dc00c Gerrit-PatchSet: 20 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Vitor de Lima <vdel...@redhat.com> Gerrit-Reviewer: Alon Bar-Lev <alo...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Martin Betak <mbe...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
