Tomas Jelinek has posted comments on this change.
Change subject: frontend: Use console popup dialog in webadmin
......................................................................
Patch Set 1: (4 inline comments)
....................................................
File
frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/presenter/popup/ConsolePopupPresenterWidget.java
Line 91: this.model = model;
Line 92: initView(model);
Line 93: initListeners(model);
Line 94:
Line 95: String vmName = model.getModel().getVM().getName();
I understand that this popup works only for VM and not for pool, so the fact
that model.getModel().getVM() returns null for pool is not handled here. But
still, it is an invariant for this class that it works only for VM and it
should guard it. Maybe for easier debugging in the future I would start this
init method with something like if(model.getModel().isPool()) { throw new
IllegalArgumentException("The console popup can not be used with pool, only
with VM") } or something like this.
Line 96: getView().setVmName(vmName);
Line 97:
Line 98: super.init(model);
Line 99: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/ConsoleAwareModel.java
Line 2:
Line 3: import org.ovirt.engine.core.common.businessentities.VM;
Line 4: import org.ovirt.engine.ui.uicommonweb.models.vms.ConsoleModel;
Line 5:
Line 6: public interface ConsoleAwareModel {
Hmm, agree with Daniel [even if I recall correctly I was suggesting this name
to franta - sry:) ]. The "aware" is common in the Spring world meaning that
spring will inject something (typically this interface has one setter method).
This is clearly not the case.
The Has* seems much more appropriate - like HasHorizontalAlignment.
Line 7:
Line 8: public boolean isPool();
Line 9:
Line 10: public ConsoleProtocol getSelectedProtocol();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalItemModel.java
Line 23: import org.ovirt.engine.ui.uicompat.Event;
Line 24: import org.ovirt.engine.ui.uicompat.EventArgs;
Line 25: import org.ovirt.engine.ui.uicompat.PropertyChangedEventArgs;
Line 26:
Line 27: public class UserPortalItemModel extends EntityModel implements
ConsoleAwareModel
Well, I don't see an issue here. It is an interface which can be implemented in
various places. As soon as you keep the contract of the interface, you are fine.
Line 28: {
Line 29:
Line 30: @Override
Line 31: public ConsoleProtocol getSelectedProtocol() {
....................................................
File
frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/tab/MainTabVirtualMachineView.java
Line 229: resources.consoleImage(),
resources.consoleDisabledImage()) {
Line 230: @Override
Line 231: protected UICommand resolveCommand() {
Line 232:
Line 233: UICommand command = new UICommand("ConsoleConnect",
new BaseCommandTarget() { // $NON-NLS-1$
What about the command would indeed be created in the model the same way as any
other commands. If executed, it would fire some event. The
MainTabVirtualMachinePresenter would listen to this event and as a reaction
would do the connect logic (this presenter would finally have some useful
logic:) )
In this way we would keep the common infrastructure but at the same time make
use of the extracted, managed logic in the managed code (but in the correct
place - e.g. presenter instead of view).
What do you think?
Line 234: @Override
Line 235: public void ExecuteCommand(UICommand uiCommand) {
Line 236: String errorMessage =
consoleManager.connectToConsole(getMainModel());
Line 237: if (errorMessage != null) {
--
To view, visit http://gerrit.ovirt.org/12877
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I00158bbd8bd488dc527821b5f088fe598c9c2713
Gerrit-PatchSet: 1
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Frank Kobzik <[email protected]>
Gerrit-Reviewer: Tomas Jelinek <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
